Skip to content
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

Merged
merged 9 commits into from
Jul 14, 2024
Merged

feat: Replace default Go user-agent with oauth2-proxy and version #2570

merged 9 commits into from
Jul 14, 2024

Conversation

middagj
Copy link
Contributor

@middagj middagj commented Mar 25, 2024

Changing the default user agent from Go-http-client/1.1 to oauth2-proxy/x.y.z.

Description

I moved the VERSION to pkg and use it to replace the default user agent from the Go standard library with one that specifies oauth2-proxy with the current version. This is done by deriving a httpClient from the default one and add a Transport 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:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.
  • I have written tests for my code changes.

providers/oidc.go Outdated Show resolved Hide resolved
pkg/requests/http.go Outdated Show resolved Hide resolved
pkg/requests/http.go Outdated Show resolved Hide resolved
pkg/version.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@JoelSpeed
Copy link
Member

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?

@middagj
Copy link
Contributor Author

middagj commented Mar 30, 2024

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 pkg.VERSION anymore. Also on GitHub UI not: https://github.com/oauth2-proxy/oauth2-proxy/pull/2570/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52

@JoelSpeed
Copy link
Member

UI still showing pkg.Version in the makefile for me, tried different browsers and the GitHub app, perhaps rebase and force push?

@middagj
Copy link
Contributor Author

middagj commented Mar 30, 2024

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 pkg.version.VERSION, but the pkg.version is green highlighted. Could it be that?

@JoelSpeed
Copy link
Member

It has changed now, but shouldn't it be pkg/version.VERSION?

Makefile Outdated Show resolved Hide resolved
@middagj
Copy link
Contributor Author

middagj commented Mar 31, 2024

It has changed now, but shouldn't it be pkg/version.VERSION?

No, it should be import path:

-X importpath.name=value
Set the value of the string variable in importpath named name to value.
This is only effective if the variable is declared in the source code either uninitialized

https://pkg.go.dev/cmd/link

I also tested this.

@JoelSpeed
Copy link
Member

The import path is pkg/version not pkg.version no?

@middagj
Copy link
Contributor Author

middagj commented Mar 31, 2024

The import path is pkg/version not pkg.version no?

Ah yes, you are right. I thought you only meant the package name. I will fix it.

Edit: fixed and tested it.

Makefile Show resolved Hide resolved
@@ -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()
Copy link
Member

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?

Copy link
Contributor Author

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.

pkg/requests/http.go Outdated Show resolved Hide resolved
providers/oidc.go Outdated Show resolved Hide resolved
@middagj
Copy link
Contributor Author

middagj commented May 6, 2024

@JoelSpeed @tuunit anything needed to get this PR to merged?

Copy link
Contributor

github-actions bot commented Jul 6, 2024

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.

@github-actions github-actions bot added the Stale label Jul 6, 2024
@middagj
Copy link
Contributor Author

middagj commented Jul 9, 2024

Still waiting for @JoelSpeed and @tuunit

@github-actions github-actions bot removed the Stale label Jul 10, 2024
@JoelSpeed JoelSpeed merged commit 3045392 into oauth2-proxy:master Jul 14, 2024
6 checks passed
@middagj middagj deleted the add-user-agent branch July 15, 2024 07:22
jjlakis pushed a commit to jjlakis/oauth2-proxy that referenced this pull request Oct 19, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants