loopdb: add Secret type for reading password from file#1131
loopdb: add Secret type for reading password from file#1131kaldun-tech wants to merge 1 commit intolightninglabs:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the security posture of the application by allowing sensitive database credentials to be read from external files rather than being passed as plain-text command-line arguments. By implementing a custom unmarshaler, the configuration system now supports a file-reference syntax, ensuring that secrets remain protected from accidental exposure in system logs or process monitoring tools. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new Secret type that allows database passwords to be read from files when prefixed with '@', enhancing security by preventing secrets from appearing in process listings. The feedback suggests restoring a gosec linter suppression to avoid CI failures, implementing the fmt.Stringer interface to prevent accidental logging of sensitive data, and broadening the whitespace trimming logic to include spaces and tabs when reading from secret files.
| Port int `long:"port" description:"Database server port."` | ||
| User string `long:"user" description:"Database user."` | ||
| Password string `long:"password" description:"Database user's password."` //nolint:gosec | ||
| Password Secret `long:"password" description:"Database user's password. Use @/path/to/file to read from a file."` |
There was a problem hiding this comment.
The //nolint:gosec directive was removed in this change. Since the field name contains "Password", it is likely to be flagged by security linters (like gosec G101) as a potential hardcoded credential. It's recommended to keep the suppression to avoid CI failures.
| Password Secret `long:"password" description:"Database user's password. Use @/path/to/file to read from a file."` | |
| Password Secret `long:"password" description:"Database user's password. Use @/path/to/file to read from a file."` //nolint:gosec |
There was a problem hiding this comment.
I think this can be ignored: The gosec G101 rule looks for hardcoded credentials in string literals. Since Secret is a type (not a hardcoded value), it shouldn't trigger.
Running the linter showed no warnings in this file via golangci-lint run -v
| // Secret is a string type that can unmarshal values from files when prefixed | ||
| // with '@'. This allows sensitive values like passwords to be stored in files | ||
| // rather than directly in configuration. | ||
| type Secret string |
There was a problem hiding this comment.
It is a best practice for sensitive types like Secret to implement the fmt.Stringer interface. This prevents the actual secret value from being accidentally exposed in logs if the configuration struct or the Secret value itself is printed using %v or %+v.
type Secret string
// String implements the fmt.Stringer interface to prevent the secret from
// being leaked in logs.
func (s Secret) String() string {
if s == "" {
return ""
}
return "xxxxxxxx"
}There was a problem hiding this comment.
This is a deliberate design decision made because:
- DSN() already handles password hiding explicitly via the hidePassword parameter
- Automatic redaction could mask debugging issues
- This keeps the Secret type simple and predictable
Introduce a Secret type that implements go-flags Unmarshaler to support reading sensitive values from files using @/path/to/file syntax. This allows the postgres password to be stored in a file rather than passed directly on the command line, avoiding exposure in process listings and shell history. Trailing whitespace (spaces, tabs, newlines) is automatically stripped from file contents. Fixes lightninglabs#1088 Signed-off-by: kaldun-tech <tsmereka@protonmail.com>
d6beab0 to
7529656
Compare
Summary
Secrettype that implementsgo-flags.Unmarshalerto support reading sensitive values from files using@/path/to/filesyntaxPostgresConfig.Passwordfield fromstringtoSecretUsage
Trailing whitespace (spaces, tabs, newlines) is automatically stripped from file contents.
Design notes
fmt.Stringerredaction:PostgresConfig.DSN(hidePassword bool)already handles password hiding explicitly. Automatic redaction could mask debugging issues.MarshalFlag: The resolved secret should not be written back to config files. Users should store the@/pathsyntax, not the resolved value.Test plan
loopd --postgres.password=@/path/to/secret.txtparses successfullyFixes #1088
🤖 Generated with Claude Code