-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: Replace default Go user-agent with oauth2-proxy and version #2570
Conversation
Something's not right here, when I look at the files view, or directly on your branch, it is showing as pkg.Version in the Makefile still, can you double check? |
Strange. If I look locally, there are no |
UI still showing pkg.Version in the makefile for me, tried different browsers and the GitHub app, perhaps rebase and force push? |
Done. The UI shows |
It has changed now, but shouldn't it be pkg/version.VERSION? |
No, it should be import path:
I also tested this. |
The import path is |
Ah yes, you are right. I thought you only meant the package name. I will fix it. Edit: fixed and tested it. |
@@ -58,7 +58,7 @@ func (r *builder) WithMethod(method string) Builder { | |||
|
|||
// WithHeaders replaces the request header map with the given header map. | |||
func (r *builder) WithHeaders(header http.Header) Builder { | |||
r.header = header | |||
r.header = header.Clone() |
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.
Why is this change necessary?
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.
Ah, this is a remnant of an earlier attempt to add it in the Builder
interface. In the current code base I don't think it is strictly necessary, but if you combine WithHeaders
and SetHeader
you are mutating the original header you passed. I left it because it look strange to me, but I can change it if that is preferred.
@JoelSpeed @tuunit anything needed to get this PR to merged? |
This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed. |
Still waiting for @JoelSpeed and @tuunit |
…uth2-proxy#2570) * feat: Replace default Go user-agent with oauth2-proxy and version * Add to CHANGELOG * Make userAgentTransport configurable and composable * Use correct naming convention for DefaultHTTPClient * Move version to own package and use named arguments * Update version path in Makefile * Fix import path in Makefile * Change importpath in dist.sh * Minor style issues
Changing the default user agent from
Go-http-client/1.1
tooauth2-proxy/x.y.z
.Description
I moved the
VERSION
topkg
and use it to replace the default user agent from the Go standard library with one that specifiesoauth2-proxy
with the current version. This is done by deriving a httpClient from the default one and add aTransport
that sets the user-agent if not already set. Then using this httpClient in the requests package and setting it on the context for the oauth2 and oidc libraries.Motivation and Context
We have an authentication service that is behind Akamai which blocks generic user agents, like
Go-http-client/1.1
.Using a more specific one fixes this. Also it can help to detect faulty clients when running an IdP related to oauth2-proxy or specific versions. This is a reason that Let's Encrypt have a strong policy to have people use an actual user agent instead of the standard library's one.
I thought about making the default user agent also configurable, but that is a bit more work if globals are not an option.
Also tried first to add default user-agent via the http builder, but then found out that if done also in the oauth2 and oidc library you need to customize it via the httpClient.
How Has This Been Tested?
There is already a test that test this specific case, so I left it.
I did roll out a patched version on our environment and it fixed out issues.
If needed, I could add some more tests for the oidc case.
Checklist: