diff --git a/pkg/api/authn.go b/pkg/api/authn.go index cb1b3a84..bf47b011 100644 --- a/pkg/api/authn.go +++ b/pkg/api/authn.go @@ -135,7 +135,8 @@ func (amw *AuthnMiddleware) basicAuthn(ctlr *Controller, userAc *reqCtx.UserAcce // saved logged session only if the request comes from web (has UI session header value) if hasSessionHeader(request) { - if err := saveUserLoggedSession(cookieStore, response, request, identity, ctlr.Log); err != nil { + secure := ctlr.Config.UseSecureSession() + if err := saveUserLoggedSession(cookieStore, response, request, identity, secure, ctlr.Log); err != nil { return false, err } } @@ -173,7 +174,8 @@ func (amw *AuthnMiddleware) basicAuthn(ctlr *Controller, userAc *reqCtx.UserAcce // saved logged session only if the request comes from web (has UI session header value) if hasSessionHeader(request) { - if err := saveUserLoggedSession(cookieStore, response, request, identity, ctlr.Log); err != nil { + secure := ctlr.Config.UseSecureSession() + if err := saveUserLoggedSession(cookieStore, response, request, identity, secure, ctlr.Log); err != nil { return false, err } } @@ -787,11 +789,11 @@ func GetGithubUserInfo(ctx context.Context, client *github.Client, log log.Logge } func saveUserLoggedSession(cookieStore sessions.Store, response http.ResponseWriter, - request *http.Request, identity string, log log.Logger, + request *http.Request, identity string, secure bool, log log.Logger, ) error { session, _ := cookieStore.Get(request, "session") - session.Options.Secure = true + session.Options.Secure = secure session.Options.HttpOnly = true session.Options.SameSite = http.SameSiteDefaultMode session.Values["authStatus"] = true @@ -806,7 +808,7 @@ func saveUserLoggedSession(cookieStore sessions.Store, response http.ResponseWri } userInfoCookie := sessions.NewCookie("user", identity, &sessions.Options{ - Secure: true, + Secure: secure, HttpOnly: false, MaxAge: cookiesMaxAge, SameSite: http.SameSiteDefaultMode, @@ -846,7 +848,8 @@ func OAuth2Callback(ctlr *Controller, w http.ResponseWriter, r *http.Request, st // if this line has been reached, then a new session should be created // if the `session` key is already on the cookie, it's not a valid one - if err := saveUserLoggedSession(ctlr.CookieStore, w, r, email, ctlr.Log); err != nil { + secure := ctlr.Config.UseSecureSession() + if err := saveUserLoggedSession(ctlr.CookieStore, w, r, email, secure, ctlr.Log); err != nil { return "", err } diff --git a/pkg/api/authn_test.go b/pkg/api/authn_test.go index 43702a60..5819d0b0 100644 --- a/pkg/api/authn_test.go +++ b/pkg/api/authn_test.go @@ -5,6 +5,8 @@ package api_test import ( "context" + "crypto/tls" + "crypto/x509" "encoding/json" "errors" "io/fs" @@ -42,6 +44,11 @@ import ( var ErrUnexpectedError = errors.New("error: unexpected error") +const ( + sessionCookieName = "session" + userCookieName = "user" +) + type ( apiKeyResponse struct { mTypes.APIKeyDetails @@ -237,18 +244,21 @@ func TestAPIKeys(t *testing.T) { client := resty.New() client.SetRedirectPolicy(test.CustomRedirectPolicy(20)) - // first login user + // first login user - in OAuth2 flow, redirect policy automatically follows and gets cookies resp, err := client.R(). SetHeader(constants.SessionClientHeaderName, constants.SessionClientHeaderValue). SetQueryParam("provider", "oidc"). Get(baseURL + constants.LoginPath) So(err, ShouldBeNil) So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusCreated) cookies := resp.Cookies() - // call endpoint without session - resp, err = client.R(). + // call endpoint without session - use a new client without cookies + clientWithoutSession := resty.New() + resp, err = clientWithoutSession.R(). + SetBody(reqBody). SetHeader(constants.SessionClientHeaderName, constants.SessionClientHeaderValue). Post(baseURL + constants.APIKeyPath) So(err, ShouldBeNil) @@ -388,7 +398,10 @@ func TestAPIKeys(t *testing.T) { So(resp, ShouldNotBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusCreated) - client.SetCookies(resp.Cookies()) + cookies := resp.Cookies() + verifySessionCookiesSecureFlag(cookies, false) // No TLS configured + + client.SetCookies(cookies) // call endpoint with session ( added to client after previous request) resp, err = client.R(). @@ -518,7 +531,10 @@ func TestAPIKeys(t *testing.T) { So(resp, ShouldNotBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusCreated) - client.SetCookies(resp.Cookies()) + cookies = resp.Cookies() + verifySessionCookiesSecureFlag(cookies, false) // No TLS configured + + client.SetCookies(cookies) resp, err = client.R(). SetBody(reqBody). @@ -782,7 +798,10 @@ func TestAPIKeys(t *testing.T) { So(resp, ShouldNotBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusCreated) - client.SetCookies(resp.Cookies()) + cookies := resp.Cookies() + verifySessionCookiesSecureFlag(cookies, false) // No TLS configured + + client.SetCookies(cookies) // call endpoint with session ( added to client after previous request) resp, err = client.R(). @@ -817,7 +836,10 @@ func TestAPIKeys(t *testing.T) { So(resp, ShouldNotBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusCreated) - client.SetCookies(resp.Cookies()) + cookies := resp.Cookies() + verifySessionCookiesSecureFlag(cookies, false) // No TLS configured + + client.SetCookies(cookies) // call endpoint with session ( added to client after previous request) resp, err = client.R(). @@ -1112,6 +1134,159 @@ func TestCookiestoreCleanup(t *testing.T) { }) } +func TestCookieSecureFlag(t *testing.T) { + Convey("Test cookie Secure flag based on configuration", t, func() { + const ( + serverCertPath = "../../test/data/server.cert" + serverKeyPath = "../../test/data/server.key" + caCertPath = "../../test/data/ca.crt" + ) + + mockOIDCServer, err := authutils.MockOIDCRun() + So(err, ShouldBeNil) + + defer func() { + err := mockOIDCServer.Shutdown() + So(err, ShouldBeNil) + }() + + username, _ := test.GenerateRandomString() + password, _ := test.GenerateRandomString() + htpasswdPath := test.MakeHtpasswdFileFromString(test.GetCredString(username, password)) + + defer os.Remove(htpasswdPath) + + mockOIDCConfig := mockOIDCServer.Config() + defaultVal := true + + Convey("Test with TLS configured - cookies should be Secure=true", func() { + conf := config.New() + port := test.GetFreePort() + baseURL := test.GetSecureBaseURL(port) + + conf.HTTP.Port = port + conf.HTTP.TLS = &config.TLSConfig{ + Cert: serverCertPath, + Key: serverKeyPath, + } + conf.HTTP.Auth = &config.AuthConfig{ + HTPasswd: config.AuthHTPasswd{ + Path: htpasswdPath, + }, + OpenID: &config.OpenIDConfig{ + Providers: map[string]config.OpenIDProviderConfig{ + "oidc": { + ClientID: mockOIDCConfig.ClientID, + ClientSecret: mockOIDCConfig.ClientSecret, + KeyPath: "", + Issuer: mockOIDCConfig.Issuer, + Scopes: []string{"openid", "email", "groups"}, + }, + }, + }, + APIKey: defaultVal, + } + + ctlr := api.NewController(conf) + ctlr.Config.Storage.RootDirectory = t.TempDir() + + cm := test.NewControllerManager(ctlr) + + cm.StartServer() + + defer cm.StopServer() + + // Load CA certificate for proper TLS verification + caCert, err := os.ReadFile(caCertPath) + So(err, ShouldBeNil) + + caCertPool := x509.NewCertPool() + caCertPool.AppendCertsFromPEM(caCert) + + client := resty.New() + client.SetRedirectPolicy(test.CustomRedirectPolicy(20)) + client.SetTLSClientConfig(&tls.Config{RootCAs: caCertPool, MinVersion: tls.VersionTLS12}) + + for { + if _, err := client.R().Get(baseURL); err == nil { + break + } + + // wait for server to be ready + time.Sleep(test.SleepTime) + } + + resp, err := client.R(). + SetHeader(constants.SessionClientHeaderName, constants.SessionClientHeaderValue). + SetQueryParam("provider", "oidc"). + Get(baseURL + constants.LoginPath) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + + cookies := resp.Cookies() + So(len(cookies), ShouldBeGreaterThan, 0) + + // Verify cookies have Secure=true when TLS is configured + verifySessionCookiesSecureFlag(cookies, true) + }) + + Convey("Test with SecureSession=true configured - cookies should be Secure=true", func() { + secureTrue := true + conf := config.New() + port := test.GetFreePort() + baseURL := test.GetBaseURL(port) + + conf.HTTP.Port = port + conf.HTTP.TLS = nil // No TLS + conf.HTTP.Auth = &config.AuthConfig{ + HTPasswd: config.AuthHTPasswd{ + Path: htpasswdPath, + }, + OpenID: &config.OpenIDConfig{ + Providers: map[string]config.OpenIDProviderConfig{ + "oidc": { + ClientID: mockOIDCConfig.ClientID, + ClientSecret: mockOIDCConfig.ClientSecret, + KeyPath: "", + Issuer: mockOIDCConfig.Issuer, + Scopes: []string{"openid", "email", "groups"}, + }, + }, + }, + APIKey: defaultVal, + SecureSession: &secureTrue, + } + + ctlr := api.NewController(conf) + ctlr.Config.Storage.RootDirectory = t.TempDir() + + cm := test.NewControllerManager(ctlr) + + cm.StartServer() + + defer cm.StopServer() + test.WaitTillServerReady(baseURL) + + client := resty.New() + client.SetRedirectPolicy(test.CustomRedirectPolicy(20)) + + // login user + resp, err := client.R(). + SetHeader(constants.SessionClientHeaderName, constants.SessionClientHeaderValue). + SetQueryParam("provider", "oidc"). + Get(baseURL + constants.LoginPath) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + + cookies := resp.Cookies() + So(len(cookies), ShouldBeGreaterThan, 0) + + // Verify cookies have Secure=true when SecureSession is set to true + verifySessionCookiesSecureFlag(cookies, true) + }) + }) +} + func TestRedisCookieStore(t *testing.T) { log := log.Logger{} @@ -1268,3 +1443,21 @@ func (di badDirInfo) Name() string { func (di badDirInfo) String() string { return fs.FormatDirEntry(di) } + +func verifySessionCookiesSecureFlag(cookies []*http.Cookie, expectedSecure bool) { + var sessionCookie, userCookie *http.Cookie + + for _, cookie := range cookies { + if cookie.Name == sessionCookieName { + sessionCookie = cookie + } else if cookie.Name == userCookieName { + userCookie = cookie + } + } + + // Verify both cookies exist and have correct Secure flag + So(sessionCookie, ShouldNotBeNil) + So(userCookie, ShouldNotBeNil) + So(sessionCookie.Secure, ShouldEqual, expectedSecure) + So(userCookie.Secure, ShouldEqual, expectedSecure) +} diff --git a/pkg/api/config/config.go b/pkg/api/config/config.go index 9bb9a469..a718a029 100644 --- a/pkg/api/config/config.go +++ b/pkg/api/config/config.go @@ -80,6 +80,7 @@ type AuthConfig struct { SessionHashKey []byte `json:"-"` SessionEncryptKey []byte `json:"-"` SessionDriver map[string]any `mapstructure:",omitempty"` + SecureSession *bool `json:"secureSession,omitempty" mapstructure:"secureSession,omitempty"` } // IsLdapAuthEnabled checks if LDAP authentication is enabled in this auth config. @@ -670,6 +671,7 @@ func (c *Config) UpdateReloadableConfig(newConfig *Config) { c.HTTP.Auth.LDAP = newConfig.HTTP.Auth.LDAP c.HTTP.Auth.APIKey = newConfig.HTTP.Auth.APIKey c.HTTP.Auth.OpenID = newConfig.HTTP.Auth.OpenID + c.HTTP.Auth.SecureSession = newConfig.HTTP.Auth.SecureSession } // Initialize and update AccessControlConfig @@ -1016,6 +1018,31 @@ func (c *Config) IsCompatEnabled() bool { return len(c.HTTP.Compat) > 0 } +// UseSecureSession returns whether cookies should have the Secure flag set. +// If TLS is configured, always returns true. Otherwise, returns the value +// of SecureSession if set, or false by default. +func (c *Config) UseSecureSession() bool { + if c == nil { + return false + } + + c.mu.RLock() + defer c.mu.RUnlock() + + // If TLS is configured, cookies should be secure + if c.HTTP.TLS != nil { + return true + } + + // If TLS is not configured, check if SecureSession is explicitly set in auth config + if c.HTTP.Auth != nil && c.HTTP.Auth.SecureSession != nil { + return *c.HTTP.Auth.SecureSession + } + + // Default to false if TLS is not configured and no explicit setting + return false +} + // IsOpenIDSupported checks if the provider supports OpenID. func IsOpenIDSupported(provider string) bool { for _, supportedProvider := range openIDSupportedProviders { diff --git a/pkg/api/config/config_test.go b/pkg/api/config/config_test.go index b5127300..41d375da 100644 --- a/pkg/api/config/config_test.go +++ b/pkg/api/config/config_test.go @@ -1989,6 +1989,83 @@ func TestConfig(t *testing.T) { So(cfg.IsMTLSAuthEnabled(), ShouldBeTrue) // No basic auth, so mTLS enabled }) + Convey("Test UseSecureSession()", func() { + // Test with nil Config + var cfg *config.Config = nil + + So(cfg.UseSecureSession(), ShouldBeFalse) + + // Test with Config but no TLS and no Auth + cfg = &config.Config{ + HTTP: config.HTTPConfig{ + TLS: nil, + Auth: nil, + }, + } + So(cfg.UseSecureSession(), ShouldBeFalse) + + // Test with TLS configured (should return true) + cfg = &config.Config{ + HTTP: config.HTTPConfig{ + TLS: &config.TLSConfig{ + Cert: "/path/to/cert.pem", + Key: "/path/to/key.pem", + }, + }, + } + So(cfg.UseSecureSession(), ShouldBeTrue) + + // Test with no TLS but SecureSession explicitly set to true + secureTrue := true + cfg = &config.Config{ + HTTP: config.HTTPConfig{ + TLS: nil, + Auth: &config.AuthConfig{ + SecureSession: &secureTrue, + }, + }, + } + So(cfg.UseSecureSession(), ShouldBeTrue) + + // Test with no TLS but SecureSession explicitly set to false + secureFalse := false + cfg = &config.Config{ + HTTP: config.HTTPConfig{ + TLS: nil, + Auth: &config.AuthConfig{ + SecureSession: &secureFalse, + }, + }, + } + So(cfg.UseSecureSession(), ShouldBeFalse) + + // Test with no TLS and Auth but SecureSession not set + cfg = &config.Config{ + HTTP: config.HTTPConfig{ + TLS: nil, + Auth: &config.AuthConfig{ + APIKey: true, + }, + }, + } + So(cfg.UseSecureSession(), ShouldBeFalse) + + // Test with TLS configured and SecureSession set (TLS should take precedence) + secureTrue = true + cfg = &config.Config{ + HTTP: config.HTTPConfig{ + TLS: &config.TLSConfig{ + Cert: "/path/to/cert.pem", + Key: "/path/to/key.pem", + }, + Auth: &config.AuthConfig{ + SecureSession: &secureTrue, + }, + }, + } + So(cfg.UseSecureSession(), ShouldBeTrue) // TLS takes precedence + }) + Convey("Test IsCompatEnabled()", func() { // Test with nil Config var cfg *config.Config = nil diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 03c40dd6..91e5b7bc 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -5779,10 +5779,12 @@ func TestAuthnSessionErrors(t *testing.T) { }) Convey("web request without cookies", func() { - client.SetCookie(&http.Cookie{}) + // Create a new client without cookies to test unauthorized access + clientWithoutCookies := resty.New() + clientWithoutCookies.SetHeader(constants.SessionClientHeaderName, constants.SessionClientHeaderValue) - // call endpoint with session (added to client after previous request) - resp, err = client.R(). + // call endpoint without session cookies + resp, err = clientWithoutCookies.R(). SetHeader(constants.SessionClientHeaderName, constants.SessionClientHeaderValue). Get(baseURL + "/v2/_catalog") So(err, ShouldBeNil)