httpwg / http-core

Core HTTP Specifications
https://httpwg.org/http-core/
466 stars 43 forks source link

WWW-Authenticate parsing #136

Closed annevk closed 4 years ago

annevk commented 6 years ago

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 commented 6 years ago

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
 ] )

:confused:

reschke commented 6 years ago

What exactly do you mean by "but"?

agebhar1 commented 6 years ago

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 commented 6 years ago

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

agebhar1 commented 6 years ago

Thank you and sorry for the noise.

annevk commented 6 years ago

Basic manual test at https://github.com/web-platform-tests/wpt/pull/13189 including links to browser bugs (all browsers fail).

reschke commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

asankah commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

How exactly would those be affected?

annevk commented 6 years ago

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 commented 6 years ago

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

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

asankah commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

royfielding commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

royfielding commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

bagder commented 5 years ago

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 commented 4 years ago

I believe this issue should be closed with no action.

mnot commented 4 years ago

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

bagder commented 4 years ago

ah yes, exactly.

mnot commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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