-
-
Notifications
You must be signed in to change notification settings - Fork 668
fix: secure token persistence #664
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: secure token persistence #664
Conversation
Summary of ChangesHello @jroth1111, 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 significantly enhances the security posture of token persistence by introducing a dedicated 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request significantly improves the security of token persistence. It introduces a new securefile package that provides helpers for atomic file writes with locking, and encryption for authentication data. The file permissions for logs and other sensitive files have been hardened. The stores (gitstore, objectstore, postgresstore, filestore) have been updated to use these new secure helpers, which is a great improvement. Additionally, sensitive information is now masked in request logs, and a warning is issued if the authentication directory is located within a Git repository, preventing accidental credential exposure.
Overall, this is an excellent set of changes that hardens the application's security posture. I have one suggestion to improve the error handling logic when decoding authentication data from the PostgreSQL store.
internal/store/postgresstore.go
Outdated
| decoded := raw | ||
| if plaintext, _, errDecode := securefile.DecodeAuthJSON(raw, securefile.CurrentAuthEncryption()); errDecode == nil { | ||
| decoded = plaintext | ||
| } else if errDecode != nil { | ||
| log.WithError(errDecode).Warnf("postgres store: auth %s decode failed; treating payload as raw json", id) | ||
| } | ||
| if err = json.Unmarshal(decoded, &metadata); err != nil { |
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.
The current error handling for DecodeAuthJSON is a bit convoluted. If decryption fails, it logs a misleading message ("treating payload as raw json") and then relies on the subsequent json.Unmarshal to fail, which logs another error. This can make debugging more difficult.
A cleaner approach is to handle the decryption error directly and skip the record. This simplifies the logic, avoids double-logging, and provides a more accurate error message.
decoded, _, errDecode := securefile.DecodeAuthJSON(raw, securefile.CurrentAuthEncryption())
if errDecode != nil {
log.WithError(errDecode).Warnf("postgres store: auth %s decode failed, skipping", id)
continue
}
if err = json.Unmarshal(decoded, &metadata); err != nil {|
This PR broke some management APIs and hidden functions (like the proxy_url and prefix in OAuth files), so I've closed it. |
|
Rebased on upstream/main; management auth file handling now reads/decrypts secure auth JSON and preserves fields like proxy_url/prefix. Uploads re-secure files. @luispater could you reopen and re-review when you have a moment? |
|
Opened a replacement PR: #673 (links back to #664). Improvements included:\n- Management auth endpoints now read/decrypt secure auth JSON so fields like proxy_url/prefix are preserved.\n- Auth file uploads are re-written via securefile to enforce encryption when enabled.\n- Log files are created with 0600 permissions. |
Summary
Motivation
Testing