-
Notifications
You must be signed in to change notification settings - Fork 498
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
Allow Athens to Propagate Authentication to Mod Download #1650
Conversation
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.
This looks pretty good. One thought, instead of a boolean, PropagateAuth
should potentially be a slice of globs similar to GOPRIVATE
to allow users to configure which domains to propagate authentication to, rather than propagating to all domains and leaking credentials to third parties
@twexler this feature is most likely accompanied by a ValidationHook or an auth proxy, which can do that part of the work for us. Do you think it's worth including here as well? Happy to do so ✌️ |
Right, but since you're using the |
So each call gets its own $HOME variable with a .netrc file. This means no two calls would ever share the same .netrc file. This ensures one call's credentials is never leaked to another call's go mod download. See this block for where it happens for the download part (same with the lister). That said, your suggestion is definitely a nice insurance to make sure a user's auth header is never propagated to the wrong Hostname even if the Athens admin has this feature turned on. @twexler I added the pattern matching to the latest commit. Let me know if it looks good to you, thanks! 🙌 |
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.
Looking great! A couple more comments.
config.dev.toml
Outdated
# for more details. | ||
# | ||
# Env override: ATHENS_PROPAGATE_AUTH_PATTERNS | ||
PropagateAuthPatterns = ["*"] |
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'm not sure if I like the default here, from a security perspective. Perhaps just ["localhost"]
?
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.
@twexler you're right. I removed this whole functionality in favor of static hosts. This should eliminate the possibility of accidentally leaking credentials to any host other host than the pre declared one.
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.
One more thing, and then this looks good
Co-authored-by: Ted Wexler <ted@stuckinacan.com>
…)" This reverts commit dfb7887.
gomods#1650 was a big change to the authentication/authorization code, which we have decided to pull out and potentially move into a separate process/project Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
* Reverting PR 1650 #1650 was a big change to the authentication/authorization code, which we have decided to pull out and potentially move into a separate process/project Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net> * removing commented, unused code Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net> * removing more commented, unused code Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net> * removing more unused code Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
This PR allows Athens to create dynamic authentication for "go mod downloads" so that a server can serve multiple modules visible to only those who request it.
Fixes #1649