weaveworks
Auth security improvements
During a security review of the auth code we identified the following improvements:
- [x]
JWTAuthorizationHeaderPrincipalGetteris not currently used and may be removed - [x]
offlinescope is not needed as we don't refresh the tokens at the moment - [ ] /oauth2/userinfo should return 401 when no auth
- [x] Reduce the validation secret cookie TTL as it doesn't need to be set to 1h
- [x] Ensure all cookies are set to HttpOnly/Secure
@yiannistri where do you see that JWTAuthorizationHeaderPrincipalGetter is not used? I can see we are adding it here. Do you mean that we do not want to add it to the "chain" anymore?
Reduce the validation secret cookie TTL as it doesn't need to be set to 1h
What should the new value be?
That's correct, we don't make any requests from the UI using the auth header just the cookie is used for now.
If/when we start using the auth header we will need to add it of course but for now it's not used.
The validation secret cookie can be set to a smaller value i.e. ~5m as it only needs to be valid while the OIDC flow takes place. @bigkevmcd do you think 5m is ok for this?
/oauth2/userinfo should return 401 when no auth
Could you clarify where we are not returning the correct status here.
The UserInfo func is "fall-through", so if we get to the end the user is authenticated.
From what I see we are writing the following codes in these "leave early" situations:
StatusMethodNotAllowedwhen notGET, which is obviously correct.StatusBadRequestwhen the IDTokenCookie is not found. This feels correct to me, but at a stretch I guess could be changed toStatusUnauthorized, although it doesn't really fit the RFC.StatusUnauthorizedwhen the UserInfo is not found on the Provider. This is correct.- And
StatusInternalServerErrorwhenever some struct cannot be marshalled. Again this is right.
I know the comment on the func declaration says we return 401 status in any other case, so just checking we actually mean to do that and change all the above status codes to StatusUnauthorized. I guess if any point fails the user is not authenticated so 🤷♀️ .
I would argue that when the user (or the user's browser) does not supply the IDToken cookie, the request lacks valid authentication credentials and therefore it should return a StatusUnauthorized. But it can be interpreted both ways so I won't insist if you think a StatusBadRequest is more suitable (we don't send back a WWW-Authenticate header anyway).
I won't insist on changing the other status codes either although I do think auth endpoints in general do not need to be super helpful with their status codes. Unless of course you're okta or auth0.
@bigkevmcd let us know if you have any thoughts on this.
WWW-Authenticate is for basic auth, so it wouldn't be appropriate.
Oh, and I think for the cli-based auth, we'll want JWTAuthorizationHeaderPrincipalGetter.
Otherwise, the response codes look good to me.
And, I think it's ok to say "bad request" to an endpoint that does the authentication as opposed to "requires authentication" but I don't feel very strongly about it, so if there was a good upstream to copy from, I'd be happy.
@yiannistri and @ozamosi what is the status of the final one issue that isn't marked above?
/oauth2/userinfo should return 401 when no auth
Is that completed? This is part of a milestone for the next release.
@davidstauffer - @yiannistri has confirmed that the last point isn't complete, we currently return 400 rather than 401... not ideal but probably not a critical fix, just something to be aware of that could be updated if you happen to be in the code.
