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

WWW-Authenticate parsing #136

Closed
annevk opened this issue Sep 21, 2018 · 35 comments · Fixed by #438
Closed

WWW-Authenticate parsing #136

annevk opened this issue Sep 21, 2018 · 35 comments · Fixed by #438

Comments

@annevk
Copy link
Contributor

annevk commented Sep 21, 2018

These are probably just implementation bugs, but it's somewhat curious that no browser handles
WWW-Authenticate: Newauth realm="apps", type=1, title="Login to \"apps\"", Basic realm="simple" from the standard correctly. They all do handle

WWW-Authenticate: Newauth realm="apps", type=1, title="Login to \"apps\""
WWW-Authenticate: Basic realm="simple"

correctly on the other hand.

I'll create some manual tests for web-platform-tests, unless someone has a good idea on how to automate this...

@agebhar1
Copy link

At 4.1 WWW-Authenticate it's defined by

WWW-Authenticate = 1#challenge

and challenge by

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

with

auth-scheme    = token
auth-param     = token BWS "=" BWS ( token / quoted-string )
atoken68        = 1*( ALPHA / DIGIT /
                       "-" / "." / "_" / "~" / "+" / "/" ) *"=" 

But in the collected section it is:

WWW-Authenticate = *( "," OWS ) challenge *( OWS "," [ OWS challenge
 ] )

😕

@reschke
Copy link
Contributor

reschke commented Sep 22, 2018

What exactly do you mean by "but"?

@agebhar1
Copy link

You're right. My fault. I didn't read the 1# correctly. Where is the alternative variable repetition defined? Thank you for your assistance.

@reschke
Copy link
Contributor

reschke commented Sep 22, 2018

See https://httpwg.org/specs/rfc7235.html#notation and the links from there.

@agebhar1
Copy link

Thank you and sorry for the noise.

@annevk
Copy link
Contributor Author

annevk commented Sep 24, 2018

Basic manual test at web-platform-tests/wpt#13189 including links to browser bugs (all browsers fail).

@reschke
Copy link
Contributor

reschke commented Sep 24, 2018

When I wrote tests around seven years ago, Konqueror was fixed to pass most of them. That seems to indicate that it's possible to implement the spec.

@annevk
Copy link
Contributor Author

annevk commented Sep 24, 2018

Oooh, can we convert those to web-platform-tests somehow, even if only manual for now? (I got some pushback from at least one Firefox developer on fixing this and given it's HTTP authentication I suspect most people won't be too enthused, but it also seems worth fixing as having separate parser requirements for any header between a single comma-separated and two headers is just bad and not really compatible with how HTTP is defined.)

@reschke
Copy link
Contributor

reschke commented Sep 24, 2018

Feel free to steal from http://test.greenbytes.de/tech/tc/httpauth ...

@asankah
Copy link

asankah commented Sep 25, 2018

While it's possible to implement the spec, given that none of the browsers implement it and doing so won't really make anything possible that isn't possible now, could we limit to one challenge per header?

@reschke
Copy link
Contributor

reschke commented Sep 25, 2018

In HTTP, intermediaries are allowed to coalesce multiple field instances into a single one, no matter what the field name is. So if you have two header instances, it's legal to combine them into a single one.

RFC 7230 made one exception for cookies, because they break otherwise. We looked at WWW-Authenticate and verified that no exception is needed here.

So my position remains: implement the spec, or otherwise prove that it's impossible/would break existing use cases.

@nwgh
Copy link

nwgh commented Sep 25, 2018

I'm the one Anne talked to. He's right, I'm not particularly excited about the idea of making this change - none of the browsers handle it right now, and with the ambiguity around commas in the grammar, making this change is on the higher side of the "likely to introduce terrible bugs" scale.

Not to mention there may be intermediaries somewhere that depend on the current behaviour. Changing this could also result in interop issues there, which will be much more difficult to fix than upgrading clients.

Given all that, this seems like a case where changing the spec would be more appropriate.

@reschke
Copy link
Contributor

reschke commented Sep 25, 2018

Not to mention there may be intermediaries somewhere that depend on the current behaviour.

How exactly would those be affected?

@annevk
Copy link
Contributor Author

annevk commented Sep 25, 2018

Even if you change the standard, some browsers might still have to change their code as https://bugs.chromium.org/p/chromium/issues/detail?id=872772#c18 indicates that, e.g., WWW-Authenticate: Basic realm="CUPS", Local parses differently across browsers. That doesn't really seem like an acceptable outcome whatever we do.

@reschke
Copy link
Contributor

reschke commented Sep 25, 2018

FWIW, that's a parse error according to the ABNF.

EDIT: in the meantime I realized that the syntax is ok.

@asankah
Copy link

asankah commented Sep 25, 2018

@reschke : Currently one can embed a comma somewhere in the auth challenge. These will break if browsers change behavior.

Given how long this bug has been around, it's difficult to estimate the effect of such a change. Both FF and Chrome have telemetry, but at least from the perspective of Chrome, we don't have much insight into many corporate or closed networks where middle boxes tend to be much more prevalent.

The change itself is fairly trivial from a code perspective. And I understand that either option isn't great. However IMHO the reward doesn't seem to justify the risk of changing browser behavior here.

@reschke
Copy link
Contributor

reschke commented Sep 25, 2018

@reschke : Currently one can embed a comma somewhere in the auth challenge. These will break if browsers change behavior.

No, it doesn't, if the parsing is done correctly. Please give an example if you think otherwise.

@nwgh
Copy link

nwgh commented Sep 25, 2018

Not to mention there may be intermediaries somewhere that depend on the current behaviour.

How exactly would those be affected?

I don't know, and that's what scares me. Intermediaries have all kinds of busted behaviour already, and changing the way the WWW-Authenticate header is parsed in practice (spec notwithstanding) could have unintended consequences (dropped as a malformed header is the first possibility that comes to mind).

@reschke
Copy link
Contributor

reschke commented Sep 25, 2018

It would be helpful if you gave at least one concrete example.

@royfielding
Copy link
Member

HTTP is not defined by browser behavior after receipt of a valid message. Even if five major browsers caught fire and imploded upon receipt of multiple challenges, that would only increase the need for those parsers to be fixed (somehow). We can't control the bits received. The easiest fix is to simply parse the field as specified, since that's what the non-browser UAs have been doing for ages.

@nwgh
Copy link

nwgh commented Sep 25, 2018

@reschke unfortunately I can't, as I have neither the time nor the resources to test even the most common intermediaries. I've just been around long enough to not trust them to not be doing something horrible.

@royfielding your hyperbole is not at all useful here, thanks. I'm glad you're confident in all the major browser vendors being so perfect as to not accidentally introduce some corner-case parsing bug that is potentially catastrophic for the security of WWW-Authenticate. I would prefer to be conservative, and recognize the fact that this is the way the internet has operated for years (decades?) and we're probably better off taking the safe course of changing the spec over changing every single browser on the planet's handling of a security-sensitive header.

@royfielding
Copy link
Member

@nwgh What hyperbole? Changing the spec is not a "safe course", no matter how you look at it. There exist cases where multiple schemes are in use today. There exist many intermediaries that will, without question, merge those multiple field names into a single field value regardless of sender's choice to send multiple fields. There exist many clients (e.g., API clients) that currently support multiple auth schemes and choose one, which is the primary audience for this bit of the protocol. The only question here is what should a browser do given the same bits received. Regardless of that answer, the spec will not be changed.

@nwgh
Copy link

nwgh commented Sep 25, 2018

OK, if the answer I give doesn't matter, and the spec won't be changed, then no point engaging.

@royfielding
Copy link
Member

We cannot change HTTP (for which there exist thousands of independent implementations) just because a few browser implementations choose to ignore a valid header field value. We can only make a change when all deployed implementations do the same thing, or when failing to implement it correctly is either impossible or known to be insecure.

Servers compensate by not delivering the same options to bug-present user agents that they do for API clients, or rely on a TLS connection being sufficient to prevent most intermediaries from joining field values (which can lead to gateway/CDN issues, etc.).

This has never been good for the security of browsers. They get stuck with Basic while the rest of the HTTP universe is using better auth schemes. Nevertheless, we can't change the spec to prevent header field joining, which deployed implementations have always done, nor can we disallow multiple auth schemes that current APIs depend upon. Hence, an implementation can choose to magically ignore some parts of the field and occasionally fail to support certain (usually more secure) auth schemes when an intermediary is present, or we can get past the FUD and just implement the field as specified. That is an implementation choice, not a spec choice.

@reschke
Copy link
Contributor

reschke commented Sep 26, 2018

FWIW, I believe there is some confusion about what a conforming client is supposed to do. Hint: it's not "splitting where a comma is". That's why I'm asking for examples.

@agebhar1
Copy link

agebhar1 commented Sep 26, 2018

@asankah if I'm not totally wrong a , is not allowed within a token

  token          = 1*tchar
  tchar          = "!" / "#" / "$" / "%" / "&" / "'" / "*"
                 / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" 
                 / DIGIT / ALPHA
                 ; any VCHAR, except delimiters

only in a (double) quoted string

  quoted-string  = DQUOTE *( qdtext / quoted-pair ) DQUOTE
  qdtext         = HTAB / SP /%x21 / %x23-5B / %x5D-7E / obs-text
  obs-text       = %x80-FF

So parsing is not straight forward "splitting where a comma is".

@reschke
Copy link
Contributor

reschke commented Sep 26, 2018

@agebhar1 - that is true, but it is allowed in a parameter value (when using quoted-string syntax).

FWIW, this syntax is common across many many HTTP header fields. A common parser library should do the trick.

@agebhar1
Copy link

agebhar1 commented Sep 26, 2018

@reschke definitely, do parsing not by yourself. But I've no idea how many libraries/frameworks does it correctly beside of browsers.

@bagder
Copy link

bagder commented Nov 20, 2018

no browser handles

Let me just gently remind the audience here that authentication if anything is possibly even more widely implemented and used in and by non-browser HTTP user-agents.

Then, I could add that curl also doesn't parse these headers "correctly" and assumes multi-line. And I don't think we've ever had a bug report about it...

@reschke
Copy link
Contributor

reschke commented Jan 7, 2020

I believe this issue should be closed with no action.

@mnot mnot added the discuss label Feb 2, 2020
@mnot
Copy link
Member

mnot commented Aug 3, 2020

@bagder does "assumes multi-line" mean "only supports multiple challenges if they're on different lines"?

@bagder
Copy link

bagder commented Aug 3, 2020

ah yes, exactly.

@mnot
Copy link
Member

mnot commented Aug 7, 2020

I think the options available to us are:

  1. Do nothing (i.e., close this issue without action)
  2. Include text advising that putting more than one WWW-Authenticate on a header field line may not be interoperable
  3. Create a formal exception for WWW-Authenticate (like Cookies)

@reschke
Copy link
Contributor

reschke commented Aug 13, 2020

I don't see why a formal exception is needed; the situation is different from Cookies (cookies do not work without the exception, but WWW-A does).

My preference is (1), but I could be persuaded to do (2).

@mnot
Copy link
Member

mnot commented Aug 13, 2020

Does anyone object to (2)? If so, why?

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

Successfully merging a pull request may close this issue.

8 participants