Ensure redirect URI always has a scheme #1045
Conversation
| @@ -902,6 +903,11 @@ func (p *OAuthProxy) getOAuthRedirectURI(req *http.Request) string { | |||
| rd.Host = requestutil.GetRequestHost(req) | |||
| rd.Scheme = requestutil.GetRequestProto(req) | |||
|
|
|||
| // If there's no scheme in the request, we should still include one | |||
There was a problem hiding this comment.
Interesting -- good find. I blanked on the possibility that Scheme could be empty from http.Requests when trying to replace the previous logic that determined HTTPS vs HTTP purely if CookieSecure was true or false.
There was a problem hiding this comment.
@JoelSpeed I wonder... is this backwards compatibility we want to support -- or should we use this opportunity for forced security by default?
This authorization endpoint response will have the OAuth authorization_code for the token endpoint. That redirect REALLY should be HTTPS (and I actually think some OIDC providers will not allow this to be set to HTTP in their allowlist).
I wonder instead should we force this to be https if none of the other mechanisms (X-Forwarded-Proto/Scheme detection) made it http for some reason.
Then a user needs to consciously decide they want to be insecure and set it explicitly to http://... in the redirect_uri config flag?
There was a problem hiding this comment.
The accidentally insecure scenario I want to protect against: users that think they are using HTTPS - but have HTTP -> HTTPS redirect either in OAuth2-Proxy directly or a proxy in front of it.
BUT forgot to set CookieSecure for some reason -- leading to a scenario where the auto-OAuth redirect_uri is computed as http and the authorization_code in the querystring to the token redeem endpoint is over HTTP the first time before an auto-redirect to the HTTPS version.
There was a problem hiding this comment.
I think defaulting to https to make this more secure could make sense, would infer to users that we are expecting them to use it securely and perhaps prompt them to think about why.
Just wondering how many people that would break though 🤔 @hoffrocket @grimurd Since I know this bug is affecting you both, do you host your OAuth2 Proxy ultimately as HTTPS or HTTP?
There was a problem hiding this comment.
Do we potentially want a warning message on startup that trips during the validation stage?
If we don't see an explicit --redirect-url, --https-address, --cookie-secure=true or --reverse-proxy config that would make us confident in our settings: give a warning that we can't dynamically determine the redirect_url scheme accurately, so we are defaulting to HTTPS - if you want HTTP, set it explicitly in --redirect-url
There was a problem hiding this comment.
I found this when I was trying to test locally with the proxy running on http://localhost.... The actual deployment is HTTPS. I understand the challenge here-- you don't want a surprising outcome for the automatic redirect creation that downgrades the security. But there are other controls against that danger to consider-- Google, at least, will fail if the redirect url does match what's in the app configuration.
I think a waterfall of configured redirect url -> X-FORWARDED-PROTO -> oauth2proxy is TLS -> http could make sense.
JoelSpeed
force-pushed
the
fix-missing-redirect-scheme
branch
from
817ab3f
to
9dbd5f5
Compare
| @@ -175,6 +177,9 @@ func Validate(o *options.Options) error { | |||
| var redirectURL *url.URL | |||
| redirectURL, msgs = parseURL(o.RawRedirectURL, "redirect", msgs) | |||
| o.SetRedirectURL(redirectURL) | |||
| if o.RawRedirectURL == "" && !o.Cookie.Secure && !o.ReverseProxy { | |||
Description
Ensure that the OAuth redirect URI always has a scheme associated with it
Motivation and Context
Fixes #1044 and restores the behaviour to pre-v7
How Has This Been Tested?
None yet, tests to be added
Checklist: