From b1f98735153d782fb47fe0c0331d3dcc0524a402 Mon Sep 17 00:00:00 2001 From: John Mazzitelli Date: Wed, 6 Dec 2023 10:22:41 -0500 Subject: [PATCH] set the Secure flag on auth cookies when appropriate fixes: https://github.com/kiali/kiali/issues/6912 --- .../authentication/openid_auth_controller.go | 6 +++- .../openid_auth_controller_test.go | 10 ++++++ business/authentication/session_persistor.go | 4 +++ .../authentication/session_persistor_test.go | 35 +++++++++++++++++++ config/config.go | 5 +++ server/server.go | 4 +-- server/server_test.go | 6 ++++ 7 files changed, 67 insertions(+), 3 deletions(-) diff --git a/business/authentication/openid_auth_controller.go b/business/authentication/openid_auth_controller.go index 510d5f0da0..3e6ada85f1 100644 --- a/business/authentication/openid_auth_controller.go +++ b/business/authentication/openid_auth_controller.go @@ -372,6 +372,7 @@ func (c OpenIdAuthController) redirectToAuthServerHandler(w http.ResponseWriter, nonceCookie := http.Cookie{ Expires: expirationTime, HttpOnly: true, + Secure: conf.IsServerHttps(), Name: OpenIdNonceCookieName, Path: conf.Server.WebRoot, SameSite: http.SameSiteLaxMode, @@ -487,12 +488,15 @@ func (p *openidFlowHelper) callbackCleanup(w http.ResponseWriter) *openidFlowHel return p } + conf := config.Get() + // Delete the nonce cookie since we no longer need it. deleteNonceCookie := http.Cookie{ Name: OpenIdNonceCookieName, Expires: time.Unix(0, 0), HttpOnly: true, - Path: config.Get().Server.WebRoot, + Secure: conf.IsServerHttps(), + Path: conf.Server.WebRoot, SameSite: http.SameSiteStrictMode, Value: "", } diff --git a/business/authentication/openid_auth_controller_test.go b/business/authentication/openid_auth_controller_test.go index 301710a5db..68c4569b88 100644 --- a/business/authentication/openid_auth_controller_test.go +++ b/business/authentication/openid_auth_controller_test.go @@ -275,11 +275,15 @@ QwIDAQAB // nonce cookie cleanup 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) // 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) // Redirection to boot the UI assert.Equal(t, "/kiali-test/", response.Header.Get("Location")) @@ -394,6 +398,8 @@ func TestOpenIdAuthControllerAuthenticatesCorrectlyWithAuthorizationCodeFlow(t * conf.LoginToken.ExpirationSeconds = 1 conf.Auth.OpenId.IssuerUri = testServer.URL conf.Auth.OpenId.ClientId = "kiali-client" + conf.Identity.CertFile = "foo.cert" // setting conf.Identity will make it look as if the endpoint ... + conf.Identity.PrivateKeyFile = "foo.key" // ... is HTTPS - this causes the cookies' Secure flag to be true config.Set(conf) // Returning some namespace when a cluster API call is made should have the result of @@ -435,11 +441,15 @@ func TestOpenIdAuthControllerAuthenticatesCorrectlyWithAuthorizationCodeFlow(t * // nonce cookie cleanup 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.True(t, response.Cookies()[0].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.True(t, response.Cookies()[1].Secure) // Redirection to boot the UI assert.Equal(t, "/kiali-test/", response.Header.Get("Location")) diff --git a/business/authentication/session_persistor.go b/business/authentication/session_persistor.go index 0b8a6c2911..330fa4b11e 100644 --- a/business/authentication/session_persistor.go +++ b/business/authentication/session_persistor.go @@ -137,6 +137,7 @@ func (p CookieSessionPersistor) CreateSession(_ *http.Request, w http.ResponseWr Value: chunk, Expires: expiresOn, HttpOnly: true, + Secure: conf.IsServerHttps(), Path: conf.Server.WebRoot, SameSite: http.SameSiteStrictMode, } @@ -152,6 +153,7 @@ func (p CookieSessionPersistor) CreateSession(_ *http.Request, w http.ResponseWr Value: strconv.Itoa(len(sessionDataChunks)), Expires: expiresOn, HttpOnly: true, + Secure: conf.IsServerHttps(), Path: conf.Server.WebRoot, SameSite: http.SameSiteStrictMode, } @@ -297,6 +299,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() + var cookiesToDrop []string numChunksCookie, chunksCookieErr := r.Cookie(config.TokenCookieName + "-chunks") @@ -326,6 +329,7 @@ func (p CookieSessionPersistor) TerminateSession(r *http.Request, w http.Respons Value: "", Expires: time.Unix(0, 0), HttpOnly: true, + Secure: conf.IsServerHttps(), MaxAge: -1, Path: conf.Server.WebRoot, SameSite: http.SameSiteStrictMode, diff --git a/business/authentication/session_persistor_test.go b/business/authentication/session_persistor_test.go index dba47ad869..baf9abb3ca 100644 --- a/business/authentication/session_persistor_test.go +++ b/business/authentication/session_persistor_test.go @@ -19,6 +19,38 @@ type testSessionPayload struct { FirstField string `json:"firstField,omitempty"` } +// TestSecureFlag tests that the cookie Secure flag is set to true when using HTTPS. +func TestSecureFlag(t *testing.T) { + cfg := config.NewConfig() + cfg.Server.WebRoot = "/kiali-app" + cfg.LoginToken.SigningKey = "kiali67890123456" + cfg.Identity.CertFile = "foo.cert" // setting conf.Identity will make it look as if the endpoint ... + cfg.Identity.PrivateKeyFile = "foo.key" // ... is HTTPS - this causes the cookies' Secure flag to be true + config.Set(cfg) + + clockTime := time.Date(2021, 12, 1, 0, 0, 0, 0, time.UTC) + util.Clock = util.ClockMock{Time: clockTime} + + payload := testSessionPayload{ + FirstField: "Foo", + } + + rr := httptest.NewRecorder() + persistor := CookieSessionPersistor{} + expiresTime := time.Date(2021, 12, 1, 1, 0, 0, 0, time.UTC) + err := persistor.CreateSession(nil, rr, "test", expiresTime, payload) + + response := rr.Result() + + assert.Nil(t, err) + assert.Len(t, response.Cookies(), 1) + + cookie := response.Cookies()[0] + assert.True(t, cookie.HttpOnly) + assert.True(t, true, cfg.IsServerHttps()) + assert.True(t, cookie.Secure) +} + // TestCreateSessionNoChunks tests that the CookieSessionPersistor correctly // sets one cookie if the payload of a session fits in one browser cookie func TestCreateSessionNoChunks(t *testing.T) { @@ -46,6 +78,8 @@ func TestCreateSessionNoChunks(t *testing.T) { cookie := response.Cookies()[0] assert.True(t, cookie.HttpOnly) + assert.Equal(t, false, cfg.IsServerHttps()) + assert.Equal(t, cfg.IsServerHttps(), cookie.Secure) assert.Equal(t, AESSessionCookieName, cookie.Name) assert.Equal(t, "/kiali-app", cookie.Path) assert.Equal(t, http.SameSiteStrictMode, cookie.SameSite) @@ -97,6 +131,7 @@ func TestCreateSessionWithChunks(t *testing.T) { for _, cookie := range response.Cookies() { assert.True(t, cookie.HttpOnly) + assert.Equal(t, cfg.IsServerHttps(), cookie.Secure) assert.Equal(t, "/kiali-app", cookie.Path) assert.Equal(t, http.SameSiteStrictMode, cookie.SameSite) assert.Equal(t, expiresTime, cookie.Expires) diff --git a/config/config.go b/config/config.go index 1590d9f2e5..dde7261034 100644 --- a/config/config.go +++ b/config/config.go @@ -877,6 +877,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 { + return conf.Identity.CertFile != "" && conf.Identity.PrivateKeyFile != "" +} + // Get the global Config func Get() (conf *Config) { rwMutex.RLock() diff --git a/server/server.go b/server/server.go index c3095bb0ef..65ad0152b3 100644 --- a/server/server.go +++ b/server/server.go @@ -98,10 +98,10 @@ func (s *Server) Start() { log.Infof("Server endpoint will start at [%v%v]", s.httpServer.Addr, s.conf.Server.WebRoot) log.Infof("Server endpoint will serve static content from [%v]", s.conf.Server.StaticContentRootDirectory) - secure := s.conf.Identity.CertFile != "" && s.conf.Identity.PrivateKeyFile != "" + go func() { var err error - if secure { + if s.conf.IsServerHttps() { log.Infof("Server endpoint will require https") log.Infof("Server will support protocols: %v", s.httpServer.TLSConfig.NextProtos) s.router.Use(secureHttpsMiddleware) diff --git a/server/server_test.go b/server/server_test.go index 1149d7c640..11327ab72a 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -18,6 +18,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/kiali/kiali/business" "github.com/kiali/kiali/config" "github.com/kiali/kiali/config/security" @@ -122,6 +124,8 @@ func TestAnonymousMode(t *testing.T) { apiURLWithAuthentication := serverURL + "/api/authenticate" apiURL := serverURL + "/api" + assert.False(t, conf.IsServerHttps()) + config.Set(conf) cf := kubernetes.NewTestingClientFactory(t) @@ -211,6 +215,8 @@ func TestSecureComm(t *testing.T) { apiURL := serverURL + "/api" metricsURL := fmt.Sprintf("http://%v:%v/", testHostname, testMetricsPort) + assert.True(t, conf.IsServerHttps()) + config.Set(conf) cf := kubernetes.NewTestingClientFactory(t)