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

proposal: net/http: add methods for setting and getting Bearer tokens #70522

Open
jub0bs opened this issue Nov 22, 2024 · 5 comments
Open

proposal: net/http: add methods for setting and getting Bearer tokens #70522

jub0bs opened this issue Nov 22, 2024 · 5 comments
Labels
Milestone

Comments

@jub0bs
Copy link

jub0bs commented Nov 22, 2024

Proposal Details

I propose the addition of the following two methods:

// SetBearerAuth, if the provided token is valid, sets the request's
// Authorization header to use the Bearer authentication scheme with that token
// and returns true.
// Otherwise, it leaves the request unchanged and returns false.
// See RFC 6750, Section 2.1.
func (*Request) SetBearerAuth(token string) bool

// BearerAuth returns the token provided in the request's
// Authorization header, if the request uses the Bearer authentication scheme.
func (r *Request) BearerAuth() (token string, ok bool)

Those methods parallel ones related to HTTP Basic Authentication already exported by net/http:

At first, you may think that the logic for getting/setting a Bearer token is so trivial that it doesn't deserve its own methods in the standard library. However, I've come to realise that many implementations out there suffer from correctness issues and/or performance issues; here are two examples (among others):

Moreover, and despite my lack of data to back up the following claim, I believe that Bearer is one of the most popular authentication scheme nowadays (likely even more popular than Basic), given the prominent role it plays in OAuth 2.x and OpenID Connect. Therefore, the logic required for parsing a request's Authorization header that uses Bearer arguably deserves to be enshrined in the standard library.

This playground contains a standalone implementation as well as a test suite. For convenience, SetBearerAuth and BearerAuth are presented there as package-level functions rather than as *Request methods.

@gopherbot gopherbot added this to the Proposal milestone Nov 22, 2024
@seankhliao
Copy link
Member

I feel like this belongs in x/oauth2.
We shouldn't encourage ad-hoc auth handling.

@mvdan
Copy link
Member

mvdan commented Nov 22, 2024

Bearer token auth was introduced as part of oauth2, but it isn't strictly tied to oauth2.

The x/oauth2 package already provides the "set token" side in a way via https://pkg.go.dev/golang.org/x/oauth2#Config.Client, but in some cases a client needs more flexibility than that, for example when writing integration tests where one needs to be fully in control of which token is used.

And, particularly for the server side, there is no API in net/http nor x/oauth2 to extract a bearer token from a request.

@seankhliao
Copy link
Member

seankhliao commented Nov 22, 2024

you can control the token directly with a static token and https://pkg.go.dev/golang.org/x/oauth2#Token.SetAuthHeader

the proposal points to the oauth2 spec saying bearer is case insensitive, but that doesn't mean any other usage of bearer is similarly case insensitive. There's no reason why we can't have the API to extract the token in x/oauth2

@mvdan
Copy link
Member

mvdan commented Nov 22, 2024

I guess oauth2.Token.SetAuthHeader is OK as a setter, although as a method hanging off of Token it's not exactly as flexible as what is proposed here. But it does support different token types, which is nice.

We would still need the getter method, which IMO is what is most important about this proposal, given that it's non-trivial to get right. If we want to double down on the oauth2.Token API I'd be fine for that func or method to live there as well. How about:

ParseAuthHeader(r *http.Request) (*Token, error)

@earthboundkid
Copy link
Contributor

I feel like I have seen bearer tokens in the wild that weren't OAuth tokens. Refusing to set a token that isn't base32 is surprising behavior. Moving it to x/oauth makes sense if we want to enforce that the tokens are valid OAuth tokens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants