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

According to RFC 2617 #1.2, the "Bearer" keyword should be case-insensitive #6150

Merged
merged 1 commit into from
Nov 29, 2018
Merged

According to RFC 2617 #1.2, the "Bearer" keyword should be case-insensitive #6150

merged 1 commit into from
Nov 29, 2018

Conversation

nlebas
Copy link
Contributor

@nlebas nlebas commented Nov 27, 2018

According to RFC 6750 item 3, the "WWW-Authenticate" header field uses the framework defined by HTTP/1.1 RFC2617.
There item 1.2 states that the auth-scheme (i.e, the "Bearer" keyword) is case insensitive.

Hope this helps,

@pivotal-issuemaster
Copy link

@nlebas Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@nlebas Thank you for signing the Contributor License Agreement!

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @nlebas! I left just one code comment, and would you also mind changing the commit message to follow what's in the contribution guidelines and also squash your commits when you are done?

@@ -87,7 +89,7 @@ public void setAllowUriQueryParameter(boolean allowUriQueryParameter) {

private static String resolveFromAuthorizationHeader(HttpServletRequest request) {
String authorization = request.getHeader(HttpHeaders.AUTHORIZATION);
if (StringUtils.hasText(authorization) && authorization.startsWith("Bearer")) {
if (StringUtils.hasText(authorization) && authorization.toLowerCase().startsWith("bearer")) {
Copy link
Contributor

@jzheaux jzheaux Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more efficient if we didn't lowercase the entire string just to compare the first few characters.

Could we instead do

Suggested change
if (StringUtils.hasText(authorization) && authorization.toLowerCase().startsWith("bearer")) {
if (StringUtils.startsWithIgnoreCase(authorization, "bearer")) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've switched to StringUtils.startsWithIgnoreCase as suggested.

The Authorization header was matched for OAuth2
against the "Bearer" keyword in a case sensitive
fashion.
According to RFC 2617, it should be case insensitive
and some oauth clients (including some earlier
versions of spring-security) expect it so.
@jzheaux jzheaux merged commit 63f2b60 into spring-projects:master Nov 29, 2018
@jzheaux
Copy link
Contributor

jzheaux commented Nov 29, 2018

Thanks for the PR, @nlebas! This is now merged into master.

@jzheaux jzheaux added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Nov 29, 2018
@jzheaux jzheaux added this to the 5.2.0.M1 milestone Nov 29, 2018
@vpavic
Copy link
Contributor

vpavic commented Nov 29, 2018

I don't think this should've been merged.

First of all, DefaultBearerTokenResolver extracts the value of Authorization header, which is really different thing from WWW-Authenticate which was used as argument for this change. The RFC 6750 is quite explicit about the syntax of Authorization header in section 2.1:

b64token    = 1*( ALPHA / DIGIT /
                       "-" / "." / "_" / "~" / "+" / "/" ) *"="
credentials = "Bearer" 1*SP b64token

It also states that Unless otherwise noted, all the protocol parameter names and values are case sensitive in section 1.1.

Regarding WWW-Authenticate itself, section 3 quite explicitly says that All challenges defined by this specification MUST use the auth-scheme value "Bearer".

To my understanding, the RFC 2617 uses case-insensitivity in terms of identifying auth scheme on the client, which isn't really a subject of Resource Server support.

@vpavic
Copy link
Contributor

vpavic commented Nov 29, 2018

I'm also pretty sure that Google will reject a protected resource request if bearer (lowercase) is presented as auth scheme in Authorization header.

@nlebas
Copy link
Contributor Author

nlebas commented Nov 29, 2018

I'll acknowledge the formulation "Unless otherwise noted" creates some ambiguity, since "uses the framework defined in RFC2617" can be interpreted as "otherwise noted", or not. All auth-schemes are always Capitalized in all specs, and use the word "MUST"...

Now from a practical perspective, I suggest the Bearer keyword should always be provided case-sensitive (capital B) by both servers and clients, but should be accepted as case insensitive, if only for backwards compatibility.

My specific issue is that I have to deal with a client that uses spring-security-oauth2 prior to this fix: spring-attic/spring-security-oauth#1346

@jgrandja
Copy link
Contributor

These are all valid points from @vpavic.

I agree we cannot compare WWW-Authenticate and Authorization as they serve different purposes.

If you look at 2.1. Authorization Request Header Field

The syntax of the "Authorization" header field for this scheme
follows the usage of the Basic scheme defined in Section 2 of
[RFC2617]. Note that, as with Basic, it does not conform to the
generic syntax defined in Section 1.2 of [RFC2617]
but is compatible
with the general authentication framework being developed for
HTTP 1.1 [HTTP-AUTH],

The key point is highlighted in bold. Although the generic syntax does state case-insensitivity for the auth-scheme, it seems the Basic scheme is case-sensitive as outlined in the syntax.

For Basic, the framework above is utilized as follows:
challenge = "Basic" realm
credentials = "Basic" basic-credentials

@nlebas
Copy link
Contributor Author

nlebas commented Nov 29, 2018

I think the statement "Note that, as with Basic, it does not conform to the
generic syntax defined in Section 1.2 of [RFC2617]" refets to the fact that auth-param is expected to include an = sign. Neither Basic nor Bearer do, as opposed to Digest for instance.

As to that, please note that the current implementation of DefaultBearerTokenResolver tolerates and ignores an eventual = sign.

@jzheaux
Copy link
Contributor

jzheaux commented Nov 29, 2018

A similar change was recently made for Basic authentication: #5586 on the same grounds.

I believe it is a reasonable interpretation that the case-insensitivity applies to the Authorization header for Bearer as it does with Basic.

@jgrandja
Copy link
Contributor

jgrandja commented Nov 29, 2018

@vpavic

I'm also pretty sure that Google will reject a protected resource request if bearer (lowercase) is presented as auth scheme in Authorization header.

Although I feel Bearer should be case-sensitive for the Authorization header based on what I'm reading/interpreting in the spec. I also feel that being more flexible is a better alternative. I'm curious though, are there any concerns if we keep things as-is? I know you mention Google but what other concerns are there (if any) that we should be aware of? This will help in deciding whether to revert or keep as-is.

@rwinch What are your thoughts on this?

@tnwang
Copy link

tnwang commented Nov 30, 2018

We've started seeing this become an issue as well. Happy to see there's been work done on this. The Cloud Foundry ecosystem is another place where the case insensitivity has existed in the past and we saw the change caused issues to show up, essential causing backwards compatibility issues for APIs that upgraded Spring Security or new APIs using the latest Spring Security.

From what I've seen via quick Google search, this is definitely an interoperability issue that comes up a lot with many implementations also using lower case.

My two-cents is that clients (the ones issuing calls using the Authorization header) should try and adhere to spec and user Bearer when they make calls, whereas resource servers can be more flexible and not reject lower case bearers.

@vpavic
Copy link
Contributor

vpavic commented Nov 30, 2018

IMO the spec is the law - and the spec's quite explicit here. Why should a security based framework reasonably add default behavior that tolerates clients not adhering to the spec? There are two options:

  • Fix the client (libraries) - seriously, how big of a problem is to send Bearer instead of bearer?
  • Provide a custom BearerTokenResolver implementation.

With changes like this also take into account:

  • What happens when someone else makes another request that strays from the spec?
  • How do I build a resource server that's fully spec compliant? With this change you're forcing me to replace the default BearerTokenResolver implementation which is just silly.

I used Google as an example that supports spec interpretation that it should be case-sensitive. I believe that anything outside of the spec doesn't belong in DefaultBearerTokenResolver.

@tnwang
Copy link

tnwang commented Nov 30, 2018

I do agree that the right way to get around all this is for clients to just use Bearer if only to avoid this case sensitivity interoperability nonsense, but many related specs do explicitly call out case-insensitivity.

If you look at 2.1. Authorization Request Header Field

The syntax of the "Authorization" header field for this scheme
follows the usage of the Basic scheme defined in Section 2 of
[RFC2617]. Note that, as with Basic, it does not conform to the
generic syntax defined in Section 1.2 of [RFC2617] but is compatible
with the general authentication framework being developed for
HTTP 1.1 [HTTP-AUTH],

This appears to refer to HTTP-AUTH spec that reiterates the same scheme is insensitive line as RFC 2617:

2.1. Challenge and Response
HTTP provides a simple challenge-response authentication framework
that can be used by a server to challenge a client request and by a
client to provide authentication information. It uses a case-
insensitive token as a means to identify the authentication scheme
,
followed by additional information necessary for achieving authentication via that scheme.

The updated Basic authentication spec found at IANA's "Hypertext Transfer Protocol (HTTP) Authentication Scheme Registry" also explicitly calls out case insensitivity for Basic:

Note that both scheme and parameter names are matched case-insensitively.

Another area where bearer pops up is RFC6749 which mentions for authorization servers that token_type is case insensitive, which makes returning token_type: bearer valid from authorization servers. Many client libraries appear to pick up and use the value which leads to the incompatibility issue in some cases. Not saying this is correct, but this has been a reason for the issue to show up based on similar issues out there via Google search.

So I'm on the same page as @nlebas here in terms of spec ambiguity.

@jgrandja
Copy link
Contributor

@tnwang Thanks for your feedback here and for those further references. After further reading the spec references I do agree with you

My two-cents is that clients (the ones issuing calls using the Authorization header) should try and adhere to spec and user Bearer when they make calls, whereas resource servers can be more flexible and not reject lower case bearers

I'm inclined to leave things as-is at this point.

@jgrandja
Copy link
Contributor

@nlebas @jzheaux The same change needs to be applied to the reactive counterpart. I added #6195

@nlebas nlebas deleted the case_sensitive_auth_header branch December 1, 2018 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants