set the Secure flag on auth cookies when appropriate #6930
Conversation
jmazzitelli
force-pushed
the
6912-cookie-secure
branch
from
e3f8017
to
a4d14c0
Compare
config/config.go
Outdated
| @@ -873,6 +873,11 @@ func (conf *Config) AllNamespacesAccessible() bool { | |||
| return conf.Deployment.ClusterWideAccess | |||
| } | |||
|
|
|||
| // IsServerHttps returns true if the server endpoint should use HTTPS. If false, only plaintext HTTP is supported. | |||
| func (conf *Config) IsServerHttps() bool { | |||
There was a problem hiding this comment.
Very minor nit that you can ignore if you like: but note that if you accept this suggestion you'll probably have to update it everywhere else.
| func (conf *Config) IsServerHttps() bool { | |
| func (conf *Config) IsServerHTTPS() bool { |
There was a problem hiding this comment.
I originally had it that way, and then the Code editor made the suggestion to change it to camel case. What is our typical code convention here? I was just following the editor blindly :)
There was a problem hiding this comment.
I think we're inconsistent but it's a generally accepted style for abbreviations in go: https://github.com/golang/go/wiki/CodeReviewComments#initialisms.
There was a problem hiding this comment.
done. See latest commit
| @@ -372,6 +372,7 @@ func (c OpenIdAuthController) redirectToAuthServerHandler(w http.ResponseWriter, | |||
| nonceCookie := http.Cookie{ | |||
| Expires: expirationTime, | |||
| HttpOnly: true, | |||
| Secure: conf.IsServerHttps(), | |||
There was a problem hiding this comment.
Is setting this to false the same as not setting this? In other words:
http.Cookie{} == http.Cookie{Secure: false}
I think so because the default value is false.
There was a problem hiding this comment.
Yes, I almost think it has to be because the "Secure" flag is just that -- a flag in the cookie. So the set-cookie header doesn't have "Secure=true" or "Secure=false" its just "Secure" or not specified.
And yes the default is false.
see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#syntax and https://http.dev/set-cookie#secure
jmazzitelli
force-pushed
the
6912-cookie-secure
branch
from
626a1a7
to
495c6c8
Compare
|
I pushed a rebase to fix the conflict in server/server.go |
|
as per @nrfox ... If there is a proxy in front that uses https (but the kiali endpoint doesn't use https), we still want to set the Secure flag. |
| @@ -367,12 +367,14 @@ func (c OpenIdAuthController) redirectToAuthServerHandler(w http.ResponseWriter, | |||
| return | |||
| } | |||
|
|
|||
| guessedKialiURL := httputil.GuessKialiURL(r) | |||
| secureFlag := conf.IsServerHTTPS() || strings.HasPrefix(guessedKialiURL, "https:") | |||
There was a problem hiding this comment.
Since this is repeated a bunch of places now can it just be its own function? Something like IsSecureCookie(...)?
fixes: #6912