From 14f063eb9d73d7ed00f97087ddc601235690586c Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Wed, 4 Sep 2024 13:32:56 +0200 Subject: [PATCH 1/3] add cors middleware - fix bug with cookies differing when calling login and whoami endpoints --- cmd/jimmsrv/main.go | 19 +++++++++++-------- cmd/jimmsrv/service/service.go | 23 ++++++++++++++++++----- go.mod | 1 + go.sum | 4 ++-- internal/auth/oauth2.go | 22 +++++++++++++++++++--- internal/auth/oauth2_test.go | 3 ++- internal/jimmhttp/auth_handler.go | 8 -------- internal/jimmtest/auth.go | 2 +- internal/jujuapi/admin_test.go | 1 + 9 files changed, 55 insertions(+), 28 deletions(-) diff --git a/cmd/jimmsrv/main.go b/cmd/jimmsrv/main.go index 4f5698bd3..efbb80309 100644 --- a/cmd/jimmsrv/main.go +++ b/cmd/jimmsrv/main.go @@ -142,6 +142,8 @@ func start(ctx context.Context, s *service.Service) error { return errors.E("jimm session store secret must be at least 64 characters") } + corsAllowedOrigins := strings.Split(os.Getenv("CORS_ALLOWED_ORIGINS"), " ") + jimmsvc, err := jimmsvc.NewService(ctx, jimmsvc.Params{ ControllerUUID: os.Getenv("JIMM_UUID"), DSN: os.Getenv("JIMM_DSN"), @@ -167,17 +169,18 @@ func start(ctx context.Context, s *service.Service) error { JWTExpiryDuration: jwtExpiryDuration, InsecureSecretStorage: insecureSecretStorage, OAuthAuthenticatorParams: jimmsvc.OAuthAuthenticatorParams{ - IssuerURL: issuerURL, - ClientID: clientID, - ClientSecret: clientSecret, - Scopes: scopesParsed, - SessionTokenExpiry: sessionTokenExpiryDuration, - SessionCookieMaxAge: sessionCookieMaxAgeInt, - JWTSessionKey: sessionSecretKey, + IssuerURL: issuerURL, + ClientID: clientID, + ClientSecret: clientSecret, + Scopes: scopesParsed, + SessionTokenExpiry: sessionTokenExpiryDuration, + SessionCookieMaxAge: sessionCookieMaxAgeInt, + JWTSessionKey: sessionSecretKey, + SecureSessionCookies: secureSessionCookies, }, DashboardFinalRedirectURL: os.Getenv("JIMM_DASHBOARD_FINAL_REDIRECT_URL"), - SecureSessionCookies: secureSessionCookies, CookieSessionKey: []byte(sessionSecretKey), + CorsAllowedOrigins: corsAllowedOrigins, }) if err != nil { return err diff --git a/cmd/jimmsrv/service/service.go b/cmd/jimmsrv/service/service.go index 6ba1dffca..c7e1ead5e 100644 --- a/cmd/jimmsrv/service/service.go +++ b/cmd/jimmsrv/service/service.go @@ -20,6 +20,7 @@ import ( "github.com/juju/names/v5" "github.com/juju/zaputil/zapctx" "github.com/prometheus/client_golang/prometheus/promhttp" + "github.com/rs/cors" "go.uber.org/zap" "gorm.io/driver/postgres" "gorm.io/gorm" @@ -82,6 +83,10 @@ type OAuthAuthenticatorParams struct { // SessionCookieMaxAge holds the max age for session cookies in seconds. SessionCookieMaxAge int + // SecureSessionCookies determines if HTTPS must be enabled in order for JIMM + // to set cookies when creating browser based sessions. + SecureSessionCookies bool + // JWTSessionKey holds the secret key used for signing/verifying JWT tokens. // See internal/auth/oauth2.go AuthenticationService.SessionSecretkey for more details. JWTSessionKey string @@ -175,14 +180,14 @@ type Params struct { // the /callback in an authorisation code OAuth2.0 flow to finish the flow. DashboardFinalRedirectURL string - // SecureSessionCookies determines if HTTPS must be enabled in order for JIMM - // to set cookies when creating browser based sessions. - SecureSessionCookies bool - // CookieSessionKey is a randomly generated secret passed via config used for signing // cookie data. The recommended length is 32/64 characters from the Gorilla securecookie lib. // https://github.com/gorilla/securecookie/blob/main/securecookie.go#L124 CookieSessionKey []byte + + // CorsAllowedOrigins represents all addresses that are valid for cross-origin + // requests. A wildcard '*' is accepted to allow all cross-origin requests. + CorsAllowedOrigins []string } // A Service is the implementation of a JIMM server. @@ -347,6 +352,7 @@ func NewService(ctx context.Context, p Params) (*Service, error) { SessionTokenExpiry: p.OAuthAuthenticatorParams.SessionTokenExpiry, SessionCookieMaxAge: p.OAuthAuthenticatorParams.SessionCookieMaxAge, JWTSessionKey: p.OAuthAuthenticatorParams.JWTSessionKey, + SecureCookies: p.OAuthAuthenticatorParams.SecureSessionCookies, Store: &s.jimm.Database, SessionStore: sessionStore, RedirectURL: redirectUrl, @@ -380,6 +386,14 @@ func NewService(ctx context.Context, p Params) (*Service, error) { return nil, errors.E(op, err, "failed to parse final redirect url for the dashboard") } + // Setup CORS middleware + corsOpts := cors.New(cors.Options{ + AllowedOrigins: p.CorsAllowedOrigins, + AllowedMethods: []string{"GET"}, + AllowCredentials: true, + }) + s.mux.Use(corsOpts.Handler) + // Setup all HTTP handlers. mountHandler := func(path string, h jimmhttp.JIMMHttpHandler) { s.mux.Mount(path, h.Routes()) @@ -406,7 +420,6 @@ func NewService(ctx context.Context, p Params) (*Service, error) { oauthHandler, err := jimmhttp.NewOAuthHandler(jimmhttp.OAuthHandlerParams{ Authenticator: authSvc, DashboardFinalRedirectURL: p.DashboardFinalRedirectURL, - SecureCookies: p.SecureSessionCookies, }) if err != nil { zapctx.Error(ctx, "failed to setup authentication handler", zap.Error(err)) diff --git a/go.mod b/go.mod index 30ba26e37..58a2c71b0 100644 --- a/go.mod +++ b/go.mod @@ -58,6 +58,7 @@ require ( github.com/lestrrat-go/iter v1.0.2 github.com/lestrrat-go/jwx/v2 v2.0.21 github.com/oklog/ulid/v2 v2.1.0 + github.com/rs/cors v1.11.1 github.com/stretchr/testify v1.9.0 golang.org/x/oauth2 v0.16.0 gopkg.in/errgo.v1 v1.0.1 diff --git a/go.sum b/go.sum index e742a5d03..5093613c6 100644 --- a/go.sum +++ b/go.sum @@ -956,8 +956,8 @@ github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFR github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4= -github.com/rs/cors v1.10.1 h1:L0uuZVXIKlI1SShY2nhFfo44TYvDPQ1w4oFkUJNfhyo= -github.com/rs/cors v1.10.1/go.mod h1:XyqrcTp5zjWr1wsJ8PIRZssZ8b/WMcMf71DJnit4EMU= +github.com/rs/cors v1.11.1 h1:eU3gRzXLRK57F5rKMGMZURNdIG4EoAmX8k94r9wXWHA= +github.com/rs/cors v1.11.1/go.mod h1:XyqrcTp5zjWr1wsJ8PIRZssZ8b/WMcMf71DJnit4EMU= github.com/rs/xid v1.2.1/go.mod h1:+uKXf+4Djp6Md1KODXJxgGQPKngRmWyn10oCKFzNHOQ= github.com/rs/xid v1.5.0 h1:mKX4bl4iPYJtEIxp6CYiUuLQ/8DYMoz0PUdtGgMFRVc= github.com/rs/xid v1.5.0/go.mod h1:trrq9SKmegXys3aeAKXMUTdJsYXVwGY3RLcfgqegfbg= diff --git a/internal/auth/oauth2.go b/internal/auth/oauth2.go index feff81490..b10e536a0 100644 --- a/internal/auth/oauth2.go +++ b/internal/auth/oauth2.go @@ -76,6 +76,8 @@ type AuthenticationService struct { sessionTokenExpiry time.Duration // sessionCookieMaxAge holds the max age for session cookies in seconds. sessionCookieMaxAge int + // secureCookies decides whether to set the secure flag on cookies. + secureCookies bool // jwtSessionKey holds the secret key used for signing/verifying JWT tokens. // According to https://datatracker.ietf.org/doc/html/rfc7518 minimum key lengths are // HSXXX e.g. HS256 - 256 bits, RSA - at least 2048 bits. @@ -119,6 +121,9 @@ type AuthenticationServiceParams struct { // SessionCookieMaxAge holds the max age for session cookies in seconds. SessionCookieMaxAge int + // SecureCookies decides whether to set the secure flag on cookies. + SecureCookies bool + // JWTSessionKey holds the secret key used for signing/verifying JWT tokens. // See AuthenticationService.JWTSessionKey for more details. JWTSessionKey string @@ -163,6 +168,7 @@ func NewAuthenticationService(ctx context.Context, params AuthenticationServiceP db: params.Store, sessionStore: params.SessionStore, sessionCookieMaxAge: params.SessionCookieMaxAge, + secureCookies: params.SecureCookies, }, nil } @@ -420,13 +426,23 @@ func (as *AuthenticationService) VerifyClientCredentials(ctx context.Context, cl return nil } +// sessionCrossOriginSafe sets parameters on the session that allow its use in cross-origin requests. +// Options are not saved to the database so this must be called whenever a session cookie will be returned to a client. +// +// Note browsers require cookies with the same-site policy as 'none' to additionally have the secure flag set. +func sessionCrossOriginSafe(session *sessions.Session, secure bool) *sessions.Session { + session.Options.Secure = secure // Ensures only sent with HTTPS + session.Options.HttpOnly = false // Allow Javascript to read it + session.Options.SameSite = http.SameSiteNoneMode // Allow cross-origin requests via Javascript + return session +} + // CreateBrowserSession creates a session and updates the cookie for a browser // login callback. func (as *AuthenticationService) CreateBrowserSession( ctx context.Context, w http.ResponseWriter, r *http.Request, - secureCookies bool, email string, ) error { const op = errors.Op("auth.AuthenticationService.CreateBrowserSession") @@ -438,8 +454,7 @@ func (as *AuthenticationService) CreateBrowserSession( session.IsNew = true // Sets cookie to a fresh new cookie session.Options.MaxAge = as.sessionCookieMaxAge // Expiry in seconds - session.Options.Secure = secureCookies // Ensures only sent with HTTPS - session.Options.HttpOnly = false // Allow Javascript to read it + session = sessionCrossOriginSafe(session, as.secureCookies) session.Values[SessionIdentityKey] = email if err = session.Save(r, w); err != nil { @@ -465,6 +480,7 @@ func (as *AuthenticationService) AuthenticateBrowserSession(ctx context.Context, if err != nil { return ctx, errors.E(op, err, "failed to retrieve session") } + session = sessionCrossOriginSafe(session, as.secureCookies) identityId, ok := session.Values[SessionIdentityKey] if !ok { diff --git a/internal/auth/oauth2_test.go b/internal/auth/oauth2_test.go index fa6ac6999..c73526179 100644 --- a/internal/auth/oauth2_test.go +++ b/internal/auth/oauth2_test.go @@ -50,6 +50,7 @@ func setupTestAuthSvc(ctx context.Context, c *qt.C, expiry time.Duration) (*auth SessionStore: sessionStore, SessionCookieMaxAge: 60, JWTSessionKey: "secret-key", + SecureCookies: false, }) c.Assert(err, qt.IsNil) cleanup := func() { @@ -295,7 +296,7 @@ func TestCreateBrowserSession(t *testing.T) { req, err := http.NewRequest("GET", "", nil) c.Assert(err, qt.IsNil) - err = authSvc.CreateBrowserSession(ctx, rec, req, false, "jimm-test@canonical.com") + err = authSvc.CreateBrowserSession(ctx, rec, req, "jimm-test@canonical.com") c.Assert(err, qt.IsNil) cookies := rec.Header().Get("Set-Cookie") diff --git a/internal/jimmhttp/auth_handler.go b/internal/jimmhttp/auth_handler.go index d8590122a..285093a84 100644 --- a/internal/jimmhttp/auth_handler.go +++ b/internal/jimmhttp/auth_handler.go @@ -34,7 +34,6 @@ type OAuthHandler struct { Router *chi.Mux authenticator BrowserOAuthAuthenticator dashboardFinalRedirectURL string - secureCookies bool } // OAuthHandlerParams holds the parameters to configure the OAuthHandler. @@ -45,10 +44,6 @@ type OAuthHandlerParams struct { // DashboardFinalRedirectURL is the final redirection URL to send users to // upon completing the authorisation code flow. DashboardFinalRedirectURL string - - // SessionCookies determines if HTTPS must be enabled in order for JIMM - // to set cookies when creating browser based sessions. - SecureCookies bool } // BrowserOAuthAuthenticator handles authorisation code authentication within JIMM @@ -63,7 +58,6 @@ type BrowserOAuthAuthenticator interface { ctx context.Context, w http.ResponseWriter, r *http.Request, - secureCookies bool, email string, ) error Logout(ctx context.Context, w http.ResponseWriter, req *http.Request) error @@ -83,7 +77,6 @@ func NewOAuthHandler(p OAuthHandlerParams) (*OAuthHandler, error) { Router: chi.NewRouter(), authenticator: p.Authenticator, dashboardFinalRedirectURL: p.DashboardFinalRedirectURL, - secureCookies: p.SecureCookies, }, nil } @@ -173,7 +166,6 @@ func (oah *OAuthHandler) Callback(w http.ResponseWriter, r *http.Request) { ctx, w, r, - oah.secureCookies, email, ); err != nil { writeError(ctx, w, http.StatusInternalServerError, err, "failed to setup session") diff --git a/internal/jimmtest/auth.go b/internal/jimmtest/auth.go index 2eb24d476..6a890dfcc 100644 --- a/internal/jimmtest/auth.go +++ b/internal/jimmtest/auth.go @@ -239,6 +239,7 @@ func SetupTestDashboardCallbackHandler(browserURL string, db *db.Database, sessi SessionStore: sessionStore, SessionCookieMaxAge: 60, JWTSessionKey: "test-secret", + SecureCookies: false, }) if err != nil { return nil, err @@ -247,7 +248,6 @@ func SetupTestDashboardCallbackHandler(browserURL string, db *db.Database, sessi h, err := jimmhttp.NewOAuthHandler(jimmhttp.OAuthHandlerParams{ Authenticator: authSvc, DashboardFinalRedirectURL: browserURL, - SecureCookies: false, }) if err != nil { return nil, err diff --git a/internal/jujuapi/admin_test.go b/internal/jujuapi/admin_test.go index 94e2db196..f306a58c9 100644 --- a/internal/jujuapi/admin_test.go +++ b/internal/jujuapi/admin_test.go @@ -62,6 +62,7 @@ func (s *adminSuite) SetUpTest(c *gc.C) { SessionStore: sessionStore, SessionCookieMaxAge: 60, JWTSessionKey: "test-secret", + SecureCookies: false, }) c.Assert(err, gc.Equals, nil) s.JIMM.OAuthAuthenticator = authSvc From bc70c8c05c84df8317f03932b4498362baee990b Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Wed, 4 Sep 2024 14:49:33 +0200 Subject: [PATCH 2/3] set httpOnly to true on cookies --- internal/auth/oauth2.go | 2 +- internal/auth/oauth2_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/auth/oauth2.go b/internal/auth/oauth2.go index b10e536a0..f68017734 100644 --- a/internal/auth/oauth2.go +++ b/internal/auth/oauth2.go @@ -432,7 +432,7 @@ func (as *AuthenticationService) VerifyClientCredentials(ctx context.Context, cl // Note browsers require cookies with the same-site policy as 'none' to additionally have the secure flag set. func sessionCrossOriginSafe(session *sessions.Session, secure bool) *sessions.Session { session.Options.Secure = secure // Ensures only sent with HTTPS - session.Options.HttpOnly = false // Allow Javascript to read it + session.Options.HttpOnly = true // Don't allow Javascript to modify cookie session.Options.SameSite = http.SameSiteNoneMode // Allow cross-origin requests via Javascript return session } diff --git a/internal/auth/oauth2_test.go b/internal/auth/oauth2_test.go index c73526179..9de52948f 100644 --- a/internal/auth/oauth2_test.go +++ b/internal/auth/oauth2_test.go @@ -509,6 +509,6 @@ func TestAuthenticateBrowserSessionHandlesMissingOrExpiredRefreshTokens(t *testi c.Assert( setCookieCookies, qt.Equals, - "jimm-browser-session=; Path=/; Expires=Thu, 01 Jan 1970 00:00:01 GMT; Max-Age=0", + "jimm-browser-session=; Path=/; Expires=Thu, 01 Jan 1970 00:00:01 GMT; Max-Age=0; HttpOnly; SameSite=None", ) } From cdcc91a60ff3d6ac0b28a255e111b4cd383af44f Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Wed, 4 Sep 2024 15:51:44 +0200 Subject: [PATCH 3/3] add test that CORS is enabled correctly --- cmd/jimmsrv/service/service_test.go | 44 +++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/cmd/jimmsrv/service/service_test.go b/cmd/jimmsrv/service/service_test.go index 9bc815d69..3d35384bc 100644 --- a/cmd/jimmsrv/service/service_test.go +++ b/cmd/jimmsrv/service/service_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "os" "testing" @@ -483,3 +484,46 @@ func TestCleanupDoesNotPanic_SessionStoreRelatedCleanups(t *testing.T) { svc.Cleanup() } + +func TestCORS(t *testing.T) { + c := qt.New(t) + + _, _, cofgaParams, err := jimmtest.SetupTestOFGAClient(c.Name()) + c.Assert(err, qt.IsNil) + p := jimmtest.NewTestJimmParams(c) + p.OpenFGAParams = cofgaParamsToJIMMOpenFGAParams(*cofgaParams) + allowedOrigin := "http://my-referrer.com" + p.CorsAllowedOrigins = []string{allowedOrigin} + p.InsecureSecretStorage = true + + svc, err := jimmsvc.NewService(context.Background(), p) + c.Assert(err, qt.IsNil) + defer svc.Cleanup() + + srv := httptest.NewServer(svc) + c.Cleanup(srv.Close) + + url, err := url.Parse(srv.URL + "/debug/info") + c.Assert(err, qt.IsNil) + // Invalid origin won't receive CORS headers. + req := http.Request{ + Method: "GET", + URL: url, + Header: http.Header{"Origin": []string{"123"}}, + } + response, err := srv.Client().Do(&req) + c.Assert(err, qt.IsNil) + defer response.Body.Close() + c.Assert(response.StatusCode, qt.Equals, http.StatusOK) + c.Assert(response.Header.Get("Access-Control-Allow-Credentials"), qt.Equals, "") + c.Assert(response.Header.Get("Access-Control-Allow-Origin"), qt.Equals, "") + + // Valid origin should receive CORS headers. + req.Header = http.Header{"Origin": []string{allowedOrigin}} + response, err = srv.Client().Do(&req) + c.Assert(err, qt.IsNil) + defer response.Body.Close() + c.Assert(response.StatusCode, qt.Equals, http.StatusOK) + c.Assert(response.Header.Get("Access-Control-Allow-Credentials"), qt.Equals, "true") + c.Assert(response.Header.Get("Access-Control-Allow-Origin"), qt.Equals, allowedOrigin) +}