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.

Sign up for GitHub

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

Jump to bottom

set the Secure flag on auth cookies when appropriate #6930

Merged
merged 4 commits into from
Dec 8, 2023
Merged

set the Secure flag on auth cookies when appropriate #6930

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b1f9873
set the Secure flag on auth cookies when appropriate
jmazzitelli Dec 6, 2023
495c6c8
rename IsServerHttps to IsServerHTTPS
jmazzitelli Dec 7, 2023
63dc900
if server itself isn't using https, check to see if the proxy in fron…
jmazzitelli Dec 7, 2023
ec9ca96
fix some tests
jmazzitelli Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 8 additions & 5 deletions business/authentication/openid_auth_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ func (c OpenIdAuthController) authenticateWithAuthorizationCodeFlow(r *http.Requ
// the callbackCleanup func cannot be called before checkOpenIdAuthorizationCodeFlowParams().
// It may sound reasonable to do a cleanup as early as possible (i.e. delete cookies), however
// if we do it, we break the "implicit" flow, because the requried cookies will no longer exist.
callbackCleanup(w).
callbackCleanup(r, w).
validateOpenIdState().
requestOpenIdToken(httputil.GuessKialiURL(r)).
parseOpenIdToken().
Expand Down Expand Up @@ -367,12 +367,14 @@ func (c OpenIdAuthController) redirectToAuthServerHandler(w http.ResponseWriter,
return
}

guessedKialiURL := httputil.GuessKialiURL(r)
secureFlag := conf.IsServerHTTPS() || strings.HasPrefix(guessedKialiURL, "https:")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is repeated a bunch of places now can it just be its own function? Something like IsSecureCookie(...)?

nowTime := util.Clock.Now()
expirationTime := nowTime.Add(time.Duration(conf.Auth.OpenId.AuthenticationTimeout) * time.Second)
nonceCookie := http.Cookie{
Expires: expirationTime,
HttpOnly: true,
Secure: conf.IsServerHTTPS(),
Secure: secureFlag,
Name: OpenIdNonceCookieName,
Path: conf.Server.WebRoot,
SameSite: http.SameSiteLaxMode,
Expand Down Expand Up @@ -405,7 +407,7 @@ func (c OpenIdAuthController) redirectToAuthServerHandler(w http.ResponseWriter,
authorizationEndpoint,
url.QueryEscape(conf.Auth.OpenId.ClientId),
responseType,
url.QueryEscape(httputil.GuessKialiURL(r)),
url.QueryEscape(guessedKialiURL),
url.QueryEscape(scopes),
url.QueryEscape(fmt.Sprintf("%x", nonceHash)),
url.QueryEscape(fmt.Sprintf("%x-%s", csrfHash, nowTime.UTC().Format("060102150405"))),
Expand Down Expand Up @@ -482,20 +484,21 @@ type openidFlowHelper struct {

// callbackCleanup deletes the nonce cookie that was generated during the redirection from Kiali to
// the OpenId server to initiate authentication (see OpenIdAuthController.redirectToAuthServerHandler).
func (p *openidFlowHelper) callbackCleanup(w http.ResponseWriter) *openidFlowHelper {
func (p *openidFlowHelper) callbackCleanup(r *http.Request, w http.ResponseWriter) *openidFlowHelper {
// Do nothing if there was an error in previous flow steps.
if p.Error != nil {
return p
}

conf := config.Get()
secureFlag := conf.IsServerHTTPS() || strings.HasPrefix(httputil.GuessKialiURL(r), "https:")

// Delete the nonce cookie since we no longer need it.
deleteNonceCookie := http.Cookie{
Name: OpenIdNonceCookieName,
Expires: time.Unix(0, 0),
HttpOnly: true,
Secure: conf.IsServerHTTPS(),
Secure: secureFlag,
Path: conf.Server.WebRoot,
SameSite: http.SameSiteStrictMode,
Value: "",
Expand Down
4 changes: 2 additions & 2 deletions business/authentication/openid_auth_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,14 +276,14 @@ QwIDAQAB
assert.Equal(t, OpenIdNonceCookieName, response.Cookies()[0].Name)
assert.True(t, clockTime.After(response.Cookies()[0].Expires))
assert.True(t, response.Cookies()[0].HttpOnly)
assert.False(t, response.Cookies()[0].Secure)
assert.True(t, response.Cookies()[0].Secure) // the test URL is https://kiali.io:44/kiali-test ; https: means it should be Secure

// Session cookie
assert.Equal(t, AESSessionCookieName, response.Cookies()[1].Name)
assert.Equal(t, expectedExpiration, response.Cookies()[1].Expires)
assert.Equal(t, http.StatusFound, response.StatusCode)
assert.True(t, response.Cookies()[1].HttpOnly)
assert.False(t, response.Cookies()[1].Secure)
assert.True(t, response.Cookies()[1].Secure) // the test URL is https://kiali.io:44/kiali-test ; https: means it should be Secure

// Redirection to boot the UI
assert.Equal(t, "/kiali-test/", response.Header.Get("Location"))
Expand Down
11 changes: 7 additions & 4 deletions business/authentication/session_persistor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/kiali/kiali/config"
"github.com/kiali/kiali/log"
"github.com/kiali/kiali/util"
"github.com/kiali/kiali/util/httputil"
)

// SessionCookieMaxSize is the maximum size of session cookies. This is 3.5K.
Expand Down Expand Up @@ -53,7 +54,7 @@ type sessionData struct {
// For improved security, the data of the session is encrypted using the AES-GCM algorithm and
// the encrypted data is what is sent in cookies. The strategy, expiresOn and payload arguments
// are all required.
func (p CookieSessionPersistor) CreateSession(_ *http.Request, w http.ResponseWriter, strategy string, expiresOn time.Time, payload interface{}) error {
func (p CookieSessionPersistor) CreateSession(r *http.Request, w http.ResponseWriter, strategy string, expiresOn time.Time, payload interface{}) error {
// Validate that there is a payload and a strategy. The strategy is required just in case Kiali is reconfigured with a
// different strategy and drop any stale session. The payload is required because it does not make sense to start a session
// if there is no data to persist.
Expand Down Expand Up @@ -116,6 +117,7 @@ func (p CookieSessionPersistor) CreateSession(_ *http.Request, w http.ResponseWr
// If the resulting session data is large, it may not fit in one cookie. So, the resulting
// session data is broken in chunks and multiple cookies are used, as is needed.
conf := config.Get()
secureFlag := conf.IsServerHTTPS() || strings.HasPrefix(httputil.GuessKialiURL(r), "https:")

sessionDataChunks := chunkString(base64SessionData, SessionCookieMaxSize)
for i, chunk := range sessionDataChunks {
Expand All @@ -137,7 +139,7 @@ func (p CookieSessionPersistor) CreateSession(_ *http.Request, w http.ResponseWr
Value: chunk,
Expires: expiresOn,
HttpOnly: true,
Secure: conf.IsServerHTTPS(),
Secure: secureFlag,
Path: conf.Server.WebRoot,
SameSite: http.SameSiteStrictMode,
}
Expand All @@ -153,7 +155,7 @@ func (p CookieSessionPersistor) CreateSession(_ *http.Request, w http.ResponseWr
Value: strconv.Itoa(len(sessionDataChunks)),
Expires: expiresOn,
HttpOnly: true,
Secure: conf.IsServerHTTPS(),
Secure: secureFlag,
Path: conf.Server.WebRoot,
SameSite: http.SameSiteStrictMode,
}
Expand Down Expand Up @@ -299,6 +301,7 @@ func (p CookieSessionPersistor) ReadSession(r *http.Request, w http.ResponseWrit
// clearing any stale cookies/session.
func (p CookieSessionPersistor) TerminateSession(r *http.Request, w http.ResponseWriter) {
conf := config.Get()
secureFlag := conf.IsServerHTTPS() || strings.HasPrefix(httputil.GuessKialiURL(r), "https:")

var cookiesToDrop []string

Expand Down Expand Up @@ -329,7 +332,7 @@ func (p CookieSessionPersistor) TerminateSession(r *http.Request, w http.Respons
Value: "",
Expires: time.Unix(0, 0),
HttpOnly: true,
Secure: conf.IsServerHTTPS(),
Secure: secureFlag,
MaxAge: -1,
Path: conf.Server.WebRoot,
SameSite: http.SameSiteStrictMode,
Expand Down