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

BearerTokenExtractor not RFC7230 compliant #894

Closed
jbspeakr opened this Issue Oct 28, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@jbspeakr

jbspeakr commented Oct 28, 2016

The current implementation of the BearerTokenExtractor is not fully RFC7230 compliant, as it expects the Authorization-header to start with the Bearer-token identifier.

As defined in RFC7230 (Section 3.2.2, Field Order) this is a valid request and should be supported:

GET /presence/alice HTTP/1.1 
Host: server.example.com
Authorization: Basic YXNkZnNhZGZzYWRmOlZLdDVOMVhk, Bearer mF_9.B5f-4.1JqM

RFC7230, section 3.2.2, Field Order


A sender MUST NOT generate multiple header fields with the same field name in a message 
unless either the entire field value for that header 
field is defined as a comma-separated list [i.e., #(values)] or the header field is a well-known exception (as noted below).
...

To make the BearerTokenExtractor RFC-compliant and hence increase its resilience regarding incoming requests, I'm going to address this in a soon to be created pull request.

jbspeakr added a commit to jbspeakr/spring-security-oauth that referenced this issue Oct 28, 2016

@jgrandja jgrandja self-assigned this Jan 27, 2017

@jgrandja

This comment has been minimized.

Contributor

jgrandja commented Jan 27, 2017

@jbspeakr Apologize for the delay in reviewing this issue and #895. We do have resource constraints on this project as we are in the process of re-writing OAuth2 and integrating it into Spring Security proper. So really this project is in maintenance mode (bug fixes) and are limiting new features.

However, if the PR is a clean merge and maintains compatibility than it shouldn't be a problem. I'm working on other priorities now but give me a couple of weeks and will get to this for you.

@jgrandja

This comment has been minimized.

Contributor

jgrandja commented Feb 16, 2017

Hi @jbspeakr. I'm finally getting to this...

According to RFC 7235, only 1 authentication scheme is allowed in the Authorization header. Your example request (and PR) allows for more than one, for example, Basic and Bearer.

If you look at Section 2.1, the following is specified...

Both the Authorization field value and the Proxy-Authorization field
value contain the client's credentials for the realm of the resource
being requested...

Notice the bold highlight...."for the realm" which indicates that the credentials being set in the Authorization header is for 1 realm only. So having Basic and Bearer in the Authorization header implies 2 realms which is invalid according to this spec.

Moreover, the syntax for the Authorization header in the same section is:

credentials = auth-scheme [ 1*SP ( token68 / #auth-param ) ]

Only 1 authentication scheme is allowed.

I hope this clarifies things and we really do appreciate your effort here.
However, the current logic behaves as expected so we will leave things as is.

I'm going to close this issue and associated PR.
If you have any questions please let me know and we can clarify things further.
Thanks again.

@jgrandja jgrandja closed this Feb 16, 2017

@jgrandja jgrandja added WontFix Invalid and removed WontFix labels Feb 16, 2017

@jbspeakr

This comment has been minimized.

jbspeakr commented Feb 24, 2017

hey @jgrandja, thanks for getting back and the in-depth feedback!

although I am not an expert on that field, RFC 7235 seems not to be as restrictive as you described it to be as it also states ...

When creating their values, the user agent ought to do so by selecting the challenge with what it considers to be the most secure auth-scheme that it understands, obtaining credentials from the user as appropriate.

Notice the bold highlight... "ought" which indicates that the user agent could send requests including more than one auth-scheme.

Furthermore, the token68 excludes commata, meaning a comma won't be interpreted as part of a token. Hence, providing multiple auth-schemes in list-notation would not interfere.

Also I don't see any reason why RFC 7235 should partly invalidate the more general RFC 7230#section-3.2.2.

I think the Spec is just not as clear about it as it probably should be. However, I believe my suggested change would have helped the BearerTokenExtractor to be a bit more resilient.

Thanks.

@jgrandja

This comment has been minimized.

Contributor

jgrandja commented Mar 7, 2017

Hi @jbspeakr I don't feel this is correct...

Notice the bold highlight... "ought" which indicates that the user agent could send requests including more than one auth-scheme.

...that the user-agent could send more than 1 auth-scheme.

Please let me clarify how the Challenge and Response authentication framework works.

Assuming a user-agent/client attempts to access a protected resource without passing any credentials, the server will respond with a Challenge (to authenticate).

As per spec:

A 401 (Unauthorized) response message is used by an origin server to challenge the authorization of a user agent, including a WWW-Authenticate header field containing at least one challenge applicable to the requested resource.

Notice that the server can respond with more than 1 challenge, for example:

HTTP/1.1 401 Unauthorized
WWW-Authenticate: Basic realm="Primary Realm", Bearer realm="OAuth2 Realm"

Next comes the Response to the Challenge from the user-agent.

As per spec:

A user agent that wishes to authenticate itself with an origin server
-- usually, but not necessarily, after receiving a 401 (Unauthorized)
-- can do so by including an Authorization header field with the
request.

...

Both the Authorization field value and the Proxy-Authorization field
value contain the client's credentials for the realm of the resource
being requested, based upon a challenge received in a response
(possibly at some point in the past). When creating their values,
the user agent ought to do so by selecting the challenge with what it
considers to be the most secure auth-scheme that it understands,
obtaining credentials from the user as appropriate.

Notice this part in the spec...

the user agent ought to do so by selecting the challenge with what it considers to be the most secure auth-scheme that it understands

The user-agent chooses only 1 of the challenges (if there is more than 1 challenge) to authenticate against.

This makes sense to me. For example, if the user-agent (the client/app) is trying to access an OAuth2 protected resource then the Authorization header would be set with the Bearer scheme and OAuth2 realm (if applicable). This would be the applicable and most secure auth-scheme that the protected resource understands.

Also to comment on...

Also I don't see any reason why RFC 7235 should partly invalidate the more general RFC 7230#section-3.2.2.

RFC 7235 defines the HTTP Authentication framework and therefore overrides and supercedes the more general semantics defined for Header fields in RFC 7230.

When it comes to security, the stricter policies always override the more general.

I hope this clarifies things further.

@jbspeakr

This comment has been minimized.

jbspeakr commented Apr 28, 2017

Hey @jgrandja, thanks for taking the time to clarify again.

I am totally fine with your argumentation. In the end (I think) this discussion is more a meta discussion about the fundamental semantics of the tiny word ought (you and I mentioned above, however with variable interpretation). As it is neither defined in e.g. RFC 2119 nor am I a native english-speaker, I am certainly not in the position to question it's meaning.

Thanks anyway – I had fun diving into this topic a bit more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment