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.

Sign up for GitHub

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: relay considered sensitive #1005

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

fix: relay considered sensitive #1005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions relay/cmd/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,10 @@ func NewConfig(ctx *cli.Context) (Config, error) {
}
return config, nil
}

// SanitizedConfig returns a copy of the Config with sensitive information removed or obfuscated.
func (c Config) SanitizedConfig() Config {
sanitized := c
sanitized.RelayConfig.AuthenticationKeyCacheSize = 0 // Obfuscate sensitive information
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm this doesn't seem like a secret. It's just a cache size right? Maybe codeQL is just parsing "AuthenticationKey" and is not smart enough to understand the suffix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is aws SecretAccessKey but it isn't triggering anything for codeQL. None other config seems to be a secret. maybe we just add shutup?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s not log secretaccesskey and shut up the rest.
Still, for best practices going forward, I’m now a big fan of separating configs and secrets into two separate structs, and only logging configs on startup. This I feel is a safer way to not in the future add more secrets that would be logged and not picked up by the now shut up codeQL.

return sanitized
}
2 changes: 1 addition & 1 deletion relay/cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func RunRelay(ctx *cli.Context) error {
if err != nil {
return fmt.Errorf("failed to create logger: %w", err)
}
logger.Info(fmt.Sprintf("Relay configuration: %#v", config))
logger.Info(fmt.Sprintf("Relay configuration: %#v", config.SanitizedConfig()))
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make sure that none of the configs printed (including awsConfig, ethConfig, etc) don't leak any secrets, and if nothing is leaked maybe there's a comment we can add to shutup codeQL on this one if its a false positive

Copy link
Contributor

@ian-shim ian-shim Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this logging was meant for debugging. Maybe we can just log few specific (non-sensitive) fields instead of the whole struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which fields are useful for debugging? we got

RelayIDs []v2.RelayKey, 
GRPCPort int
MaxGRPCMessageSize int
MetadataCacheSize int
MetadataMaxConcurrency int
BlobCacheBytes uint64
BlobMaxConcurrency int
ChunkCacheSize uint64
ChunkMaxConcurrency int
MaxKeysPerGetChunksRequest int
RateLimits limiter.Config
AuthenticationKeyCacheSize int <- codeQL doesn't like this one
AuthenticationTimeout time.Duration
AuthenticationDisabled bool
Timeouts TimeoutConfig
OnchainStateRefreshInterval time.Duration
MetricsPort int


dynamoClient, err := dynamodb.NewClient(config.AWS, logger)
if err != nil {
Expand Down
Loading