set the Secure flag on auth cookies when appropriate

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:    "",
 	}

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"))

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,

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)

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()

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)

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)