-
Notifications
You must be signed in to change notification settings - Fork 156
Default anonymous authentication #2914
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2914 +/- ##
=======================================
Coverage 56.26% 56.26%
=======================================
Files 324 324
Lines 31897 31900 +3
=======================================
+ Hits 17947 17950 +3
Misses 12420 12420
Partials 1530 1530 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
960ecd9 to
a2483ef
Compare
|
Hey, was reading through the code and spotted something weird. You have this default config that sets auth to anonymous: config := Config{
Auth: &AuthConfig{
Mode: AuthModeAnonymous,
},
}But up in the AuthMode comments it says it defaults to oauth for "security-by-default". So there's a mismatch between what the docs say and what actually happens. anyone who deploys this without thinking about auth config will be running wide open. Default to oauth and only go anonymous if someone explicitly sets it in the config? people have to consciously choose the insecure option instead of accidentally getting it. Might be missing context on why its like this though |
ChrisJBurns
left a comment
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.
Thanks for doing this @dmartinol as it was getting tiring having to manually edit the config configmap. Although I think if we do add this change we'd need to enable it on the CRD too, because if we specify default of anonymous, we aren't giving users a way of providing oauth. So we should probably allow for the full auth config if we do have it in the operator
This reminds me of another issue I've found: if you |
Hey @slyt3 , you are right: I copied only the minimal data types from the equivalent configuration types in the TL;DR: I'm not forcing anyone to merge this PR, but without this fix the registry server won't start. |
Fix #2870
Adda a default anonymous authentication until the operator consistently manages this aspect.