Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Hide other sensitive cfg values #194

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

epociask
Copy link
Collaborator

Fixes Issue

Fixes #176

Changes proposed

Screenshots (Optional)

Note to reviewers

Copy link
Collaborator

@samlaf samlaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Just one thing to fix

configJSON, err := json.MarshalIndent(cfg, "", " ")
if err != nil {
return fmt.Errorf("failed to marshal config: %w", err)
}
cfg.EigenDAConfig.EdaClientConfig.SignerPrivateKeyHex = "" // marshaling defined in client config
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you move this after the marshaling? Like this it doesn’t do anything

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does actually - the SignerPrivateKey is stored as part of the EigenDAClientConfig which is defined within layr-labs/eigenda

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log line below is

log.Info(fmt.Sprintf("Initializing EigenDA proxy server with config: %v", string(configJSON)))

which prints the already marshalled config. So changing the fields of cfg here above doesn't do anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️

store/precomputed_key/redis/redis.go Show resolved Hide resolved
@epociask epociask requested a review from samlaf October 26, 2024 18:33
Eviction time.Duration
Profile bool
Endpoint string `json:"endpoint"`
Password string `json:"-"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about this approach some more and not sure I like it, for a few reasons:

  1. if some future developer ever wants to marshal AND unmarshal the config for whatever reason he will run into a bug (can fix this by defining a custom marshalJSONHideSecrets function instead)
  2. this will print empty string, so reader won’t know if password field was hidden or just never set (we can fix this by using omitempty json tag)

Could look something like this (Claude generated so beware):

package main

import (
	"encoding/json"
	"fmt"
	"time"
)

type Config struct {
	Endpoint string        `json:"endpoint"`
	Password string        `json:"password,omitempty"`
	DB       int           `json:"database"`
	Eviction time.Duration `json:"eviction"`
}

// Custom MarshalJSON function to control what gets included in the JSON output
func (c Config) MarshalJSON() ([]byte, error) {
	type Alias Config // Use an alias to avoid recursion with MarshalJSON
	aux := struct {
		Alias
		Password string `json:"password,omitempty"`
	}{
		Alias:    (Alias)(c),
		Password: "",
	}

	// Conditionally include a masked password if it is set
	if c.Password != "" {
		aux.Password = "****" // Mask the password instead of excluding it
	}
	return json.Marshal(aux)
}

func main() {
	cfg := Config{
		Endpoint: "localhost:6379",
		Password: "supersecret",
		DB:       0,
		Eviction: time.Hour,
	}

	jsonData, _ := json.MarshalIndent(cfg, "", "  ")
	fmt.Println(string(jsonData))
}

Maybe also use a separate Marshal function to not change the default marshaling behavior?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache credentials and the private key are exposed in the logs
2 participants