-
Notifications
You must be signed in to change notification settings - Fork 187
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: relay considered sensitive #1005
base: master
Are you sure you want to change the base?
Conversation
// 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -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())) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Why are these changes needed?
https://github.com/Layr-Labs/eigenda/security/code-scanning/18
Not sure why my commit was the source it identified 🤷
Checks