Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CORS middleware #1349

Merged
merged 3 commits into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions cmd/jimmsrv/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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
Expand Down
23 changes: 18 additions & 5 deletions cmd/jimmsrv/service/service.go
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you can add some requests forged to trigger cors policies in service_test.go

Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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())
Expand All @@ -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))
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
22 changes: 19 additions & 3 deletions internal/auth/oauth2.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -163,6 +168,7 @@ func NewAuthenticationService(ctx context.Context, params AuthenticationServiceP
db: params.Store,
sessionStore: params.SessionStore,
sessionCookieMaxAge: params.SessionCookieMaxAge,
secureCookies: params.SecureCookies,
}, nil
}

Expand Down Expand Up @@ -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 = true // Don't allow Javascript to modify cookie
session.Options.SameSite = http.SameSiteNoneMode // Allow cross-origin requests via Javascript
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's possible to set a domain for cookies, like .canonical. I can imagine in a production environment having the dashboard and jimm under the same subdomain.
In this way we are more secure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The domain automatically gets set to the domain that set the cookie. Doc source
image

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")
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
5 changes: 3 additions & 2 deletions internal/auth/oauth2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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, "[email protected]")
err = authSvc.CreateBrowserSession(ctx, rec, req, "[email protected]")
c.Assert(err, qt.IsNil)

cookies := rec.Header().Get("Set-Cookie")
Expand Down Expand Up @@ -508,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",
)
}
8 changes: 0 additions & 8 deletions internal/jimmhttp/auth_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ type OAuthHandler struct {
Router *chi.Mux
authenticator BrowserOAuthAuthenticator
dashboardFinalRedirectURL string
secureCookies bool
}

// OAuthHandlerParams holds the parameters to configure the OAuthHandler.
Expand All @@ -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
Expand All @@ -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
Expand All @@ -83,7 +77,6 @@ func NewOAuthHandler(p OAuthHandlerParams) (*OAuthHandler, error) {
Router: chi.NewRouter(),
authenticator: p.Authenticator,
dashboardFinalRedirectURL: p.DashboardFinalRedirectURL,
secureCookies: p.SecureCookies,
}, nil
}

Expand Down Expand Up @@ -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")
Expand Down
2 changes: 1 addition & 1 deletion internal/jimmtest/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions internal/jujuapi/admin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading