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

HTTP 401 MUST return a WWW-Authenticate header #65

Open
danmarg opened this issue Jul 26, 2024 · 16 comments
Open

HTTP 401 MUST return a WWW-Authenticate header #65

danmarg opened this issue Jul 26, 2024 · 16 comments

Comments

@danmarg
Copy link
Contributor

danmarg commented Jul 26, 2024

We initially wrote here that the HTTP request that triggers a challenge/response should serve an HTTP 401 response code and a Sec-Session-Challenge header.

But per https://datatracker.ietf.org/doc/html/rfc2616#section-10.4.2, a 401 MUST return a WWW-Authenticate header.

Without having put any thought into this, I can see a few obvious options:

  1. Add a new HTTP authentication method, and replace the Sec-Session-Challenge header with a WWW-Authenticate header.
  2. Just return a different response code.

I see the point of doing (1) just to avoid violating the spec--(2) seems like a better choice for that. And I could see some value in reusing HTTP authentication if we think that the DBSC-style challenge/response authn will be useful on its own (and thus is worth fleshing out "standalone", without the whole refresh/session-management stuff).

But I am not sure the value there is worth trying to make this fit the existing HTTP authentication paradigm; option (2) thus seems better to me.

Curious to hear what others think! Should we just return a 403 instead?

@MattMenke2
Copy link

Worth noting that while RFC 2616 is of course obsolete, the current RFC has similar language (https://datatracker.ietf.org/doc/html/rfc7235).

@mattjm
Copy link
Contributor

mattjm commented Jul 26, 2024

@MattMenke2 thanks for the reminder about updated RFCs--based on the obsolete one I was going to say 403 might not be appropriate, but per updated RFCs it should be OK:

"The client SHOULD NOT automatically repeat the request with the same credentials. The client MAY repeat the request with new or different credentials. " citation

I agree it seems cumbersome to define a new authentication scheme just for this.

I feel like I've seen (don't ask me for examples) a few cases where a service returned 403 when a 401 might have made more sense. Now I'm wondering how often that was because the engineer didn't want to bother returning the WWW-authenticate header but needed to comply with the spec. 😂

@MattMenke2
Copy link

I think a 403 would be reasonable, though I'm no expert on how 403 are typically used (or intended to be used). Another option would be to return a 200 with a no-cache, no-store header. Main advantage of the 4xx status code is that it informs any intermediaries that this is an error and should not be cached, and potentially shows up in any applicable logs as an error.

@MattMenke2
Copy link

MattMenke2 commented Jul 26, 2024

Separating it out from an error code would also allow redirecting browsers that don't support to, e.g., an alternative login flow or whatnot, rather than having to sniff browser version to determine compatibility. So return a redirect + challenge. If browser knows how to handle the request, it send credentials. Otherwise, it follows the redirect. That's how Sec-Session-Storage (?) works.

I guess as specified, the 401s aren't actually normal loads, but rather browser-initiated loads in response to other requests, so I that capability may not be useful here.

@danmarg
Copy link
Contributor Author

danmarg commented Jul 26, 2024

Uh oh. As I read this, I think I've stepped into a can of worms.

Three observations:

  1. The explainer also contains this: The server can also serve challenges ahead of time attached to any response as an optimization, for example...1
  2. The refresh endpoint can choose to not require a new challenge/response--it can set the required cookies right away if, for example, the IP address hasn't changed (or whatever). It can also choose to not set the required cookies, of course (like, if the signature is broken).
  3. We are only talking about requests to the refresh endpoint, so browsers that don't understand DBSC should never even make such requests.

So this leaves me a bit confused! On the one hand, I think point 3 above matches your point, @MattMenke2, that these aren't normal page loads, so the advantage of serving existing error codes is limited.

On the other hand, point 2 suggests to me that it's important that unexpected errors from refresh trigger DBSC session termination: it's important that the browser stop throttling requests that require some DBSC-managed cookie if it tried and failed to refresh the session; it's those throttled requests--which are "normal" page loads--that will then be treated as unauthenticated and result in the user being redirected to a sign-in or error page.

But, point (1) makes me think the server can choose to merely return a Sec-Session-Challenge header on refresh and the right thing will happen, regardless of error code.

Whew!

Now, I sort of think the state machine for client-side behavior is something like:

  • If a response has a Sec-Session-Challenge, store that challenge as "latest challenge"
  • If a pending request matches a DBSC session and is missing a required cookie, call the refresh endpoint
  • If calling the refresh endpoint and there's an existing "latest challenge", sign it and add the signature to the request

All of that suggests to me that we can just get rid of specifying HTTP response codes entirely on calls to refresh and the above algorithm just does The Right Thing.

Sorry for the very long response. Does that reasoning make sense?

Footnotes

  1. I think this is unclear. Reading the text, it seems like it's saying that that should immediately trigger a refresh ("The browser replies to that response with a Sec-Session-Response header, containing a signed JWT..."), but I believe the intent was that the browser should pre-cache that challenge and use it in the future whenever it decides to do a refresh.

@danmarg
Copy link
Contributor Author

danmarg commented Jul 26, 2024

Oh, and to my comment

On the other hand, point 2 suggests to me that it's important that unexpected errors from refresh trigger DBSC session termination

I think this means that if there is an error code from refresh, it should trigger session termination. (Though we need to specify which codes are "fatal" and which are "transient" and require a client retry.)

@MattMenke2
Copy link

Alternatively, could just consider the session refreshed until it's refresh time again, or the server tells us otherwise, but since I don't think we tell the server in normal requests about the live session, unlike cookies, killing the session on error does seem reasonable to me.

@kmonsen
Copy link
Collaborator

kmonsen commented Jul 31, 2024

I think there are a few options:

  1. Update 401 to mandate WWW-Authenticate OR Sec-Session-Challenge, would be broken against browsers that doesn't support DBSC, but the server should only send this where it expects there to be a session. I guess one questions is what would the client do if the session has just been delete locally? We could resend the request with a "No such session" or something.
  2. Use some other 4xx/invent a new one. This sort of has the same issues as (1) I think.
  3. Use 302 as we once intended. This has a few issues, first 401 forbidden makes a lot of sense in that the content is forbidden until the client authenticates, it has not been moved. Second all the current standard compliant browsers know how to resend a request when receiving 401 after updating the authentication.

I don't feel strongly about this (I argued for 302 for a long time, primarily for backwards compatibility). Happy to update as needed to what others want. I think right now, I would propose we try (1) unless we meet large resistance.

I should say currently there is an option where the refresh requests doesn't happen, but instead the browser start signing every request and the server can reply with 401. So the client need to expect 401 on different requests, but all requests that can have a 401 response have "Sec-Session-Response: JWT" header in the request.

@wparad
Copy link

wparad commented Aug 1, 2024

302 feels right, but may cause the wrong things to happen in browsers/clients that handle this differently.

Since the DBSC process starts authentication, wouldn't it make sense that the www-authenticate header is still returned with the matching information? Is somehow using this incompatible with DBSC?

@aaronpk
Copy link

aaronpk commented Sep 26, 2024

I don't see how a 3xx code makes any sense, since the followup request is made to the same URL, so it's not a redirect.

@kmonsen
Copy link
Collaborator

kmonsen commented Sep 26, 2024

3xx can be used with a redirect to yourself (or a new url as needed) as it works with current web standards.

I agree that 401 feel more right to me.

@MattMenke2
Copy link

Redirects provide error handling, without having to send a full error page to display to the user to browsers that support the feature. With a 401, if you want an error page, you have to send it out every single time you want the cookies, even if it's supported by nearly every browser that visits your site.

@MattMenke2
Copy link

MattMenke2 commented Sep 26, 2024

And just to make sure we're on the same page:

First response, say for https://foo/cookie-request would get the challenge header and a redirect to https://foo/cookie-request-not-supported.

If the browser can handle the challenge with another network request, it does so, and then re-requests https://foo/cookie-request with any additional cookies that it set. If it does not, it follows the redirect to an error page / alternative login.

So the redirect would be for browsers that don't support the feature.

Alternatively, a server could just send a 4xx error page...Basically, the response code handling, and the header handling are entirely independent. If a browser sees the header, it sends another fetch, and then re-requests the original URL, regardless of the status code. If it does not, normal handling proceeds. So the HTTP status code and this feature have no interactions with each other. We don't need a new status code. Edit: Nor do we need to require the use of a specific pre-existing one.

@danmarg
Copy link
Contributor Author

danmarg commented Sep 26, 2024

@MattMenke2 I might be misreading you, but just to reiterate one point which may or may not be clear: as described here, the Sec-Session-Challenge header is (normally) served on refresh attempts. These are requests to the session-registered "refresh" endpoint, so should never be made by a browser that doesn't support DBSC. It's for that reason that I think the precise HTTP error code used is less important than it at first appears (though we should still do something stylistically reasonable).

@MattMenke2
Copy link

So it's been 3 months since I made the comment folks were responding to, and haven't thought about this stuff since, so I've completely lost all context.

I still think a 401 is wrong here, since this is not structured at all like HTTP auth. We also don't even need a special response code here - this is a request that's being managed by code that's looking for the challenge, all we need is the challenge header, not a special response code, if we want to make a new request. The network layer doesn't need to know what's going on at all (well, requests do need to bypass the cache, but that's just an HTTP-network fetch, already covered by the fetch spec, as opposed to an HTTP-network-or-cache fetch. So the response code is entirely irrelevant, if this is structured as multiple network requests managed outside the network layer, as opposed to the network layer retrying a network request.

@danmarg
Copy link
Contributor Author

danmarg commented Sep 26, 2024 via email

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

No branches or pull requests

6 participants