diff --git a/business/authentication/openid_auth_controller.go b/business/authentication/openid_auth_controller.go index 510d5f0da0..0fb5f2bda2 100644 --- a/business/authentication/openid_auth_controller.go +++ b/business/authentication/openid_auth_controller.go @@ -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(). @@ -367,11 +367,14 @@ func (c OpenIdAuthController) redirectToAuthServerHandler(w http.ResponseWriter, return } + guessedKialiURL := httputil.GuessKialiURL(r) + secureFlag := conf.IsServerHTTPS() || strings.HasPrefix(guessedKialiURL, "https:") nowTime := util.Clock.Now() expirationTime := nowTime.Add(time.Duration(conf.Auth.OpenId.AuthenticationTimeout) * time.Second) nonceCookie := http.Cookie{ Expires: expirationTime, HttpOnly: true, + Secure: secureFlag, Name: OpenIdNonceCookieName, Path: conf.Server.WebRoot, SameSite: http.SameSiteLaxMode, @@ -404,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"))), @@ -481,18 +484,22 @@ 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, - Path: config.Get().Server.WebRoot, + Secure: secureFlag, + 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..381d377a6a 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.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.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")) @@ -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..3581accc03 100644 --- a/business/authentication/session_persistor.go +++ b/business/authentication/session_persistor.go @@ -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. @@ -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. @@ -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 { @@ -137,6 +139,7 @@ func (p CookieSessionPersistor) CreateSession(_ *http.Request, w http.ResponseWr Value: chunk, Expires: expiresOn, HttpOnly: true, + Secure: secureFlag, Path: conf.Server.WebRoot, SameSite: http.SameSiteStrictMode, } @@ -152,6 +155,7 @@ func (p CookieSessionPersistor) CreateSession(_ *http.Request, w http.ResponseWr Value: strconv.Itoa(len(sessionDataChunks)), Expires: expiresOn, HttpOnly: true, + Secure: secureFlag, Path: conf.Server.WebRoot, SameSite: http.SameSiteStrictMode, } @@ -297,6 +301,8 @@ 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 numChunksCookie, chunksCookieErr := r.Cookie(config.TokenCookieName + "-chunks") @@ -326,6 +332,7 @@ func (p CookieSessionPersistor) TerminateSession(r *http.Request, w http.Respons Value: "", Expires: time.Unix(0, 0), HttpOnly: true, + Secure: secureFlag, 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..966bf748dc 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, 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.False(t, cfg.IsServerHTTPS()) + assert.False(t, 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.False(t, 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..18089fe133 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..496a099bac 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..6e5526123a 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)