-
Notifications
You must be signed in to change notification settings - Fork 2
fix issue with hydra no openning up enrichment paths #601
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
Conversation
Summary of ChangesHello @pitabwire, 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 resolves an issue where Hydra was not correctly opening or utilizing enrichment paths by updating its core configuration. The changes primarily involve adding specific webhook endpoints for token and refresh token enrichment, alongside other detailed OAuth2 settings to bolster security and feature completeness. Additionally, minor cleanup was performed by removing redundant logging statements from the server's HTTP/2 setup. Highlights
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 updates the Ory Hydra configuration to enable token enrichment webhooks, which resolves an issue with enrichment paths not being available. The changes also include several other Hydra configuration updates and the removal of a couple of log messages from the server startup logic.
My review focuses on the security implications of the new Hydra configuration settings. I've identified a few areas of concern:
- The PBKDF2 iteration count is critically low, which significantly weakens password hashing.
- The JWT maximum time-to-live is very long, increasing the window of opportunity for misuse if a token is compromised.
- Internal errors are exposed, which is risky for production environments.
I've provided specific suggestions to address these points. While some of these settings might be acceptable for a test environment, it's crucial to follow security best practices to avoid accidentally promoting insecure configurations. The changes in server.go seem unrelated to the main purpose of this PR; it's generally better to keep pull requests focused on a single concern.
| bcrypt: | ||
| cost: 10 | ||
| pbkdf2: | ||
| iterations: 1 |
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 number of iterations for PBKDF2 is set to 1. This is critically low and provides almost no resistance to brute-force attacks, making the hashed values insecure. While this is for a test environment, using such a weak setting is a dangerous practice. It is highly recommended to use a much higher iteration count. For example, OWASP recommends values in the hundreds of thousands for production. For testing, a value like 10000 would offer a better balance between security and performance.
| iterations: 1 | |
| iterations: 10000 |
| jwt: | ||
| iat_optional: false | ||
| jti_optional: false | ||
| max_ttl: 720h |
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 max_ttl for JWT-based grants is set to 720h (30 days). This is an exceptionally long lifetime for a token, which significantly increases the security risk if the token is compromised. It is a security best practice to use short-lived tokens. Consider reducing this to a much shorter duration, such as 1h, to limit the window of exposure.
| max_ttl: 720h | |
| max_ttl: 1h |
| grace_period: 1h | ||
| refresh_token_hook: http://127.0.0.1:3000/webhook/enrich/refresh-token | ||
| token_hook: http://127.0.0.1:3000/webhook/enrich/token | ||
| expose_internal_errors: true |
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.
Setting expose_internal_errors to true can be useful for debugging in development or test environments. However, it poses a significant security risk in production by potentially leaking sensitive information like stack traces or internal application details. Please ensure this setting is disabled in any production configuration to prevent information disclosure vulnerabilities.
No description provided.