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

Kim 163890622 update cookie attributes #1852

Merged
merged 11 commits into from Mar 14, 2019
@@ -817,16 +817,20 @@ func main() {
if err != nil {
logger.Fatal("Registering login provider", zap.Error(err))
}

useSecureCookie := true
if isDevOrTest {
useSecureCookie = false
}

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Mar 13, 2019

Contributor

Could you just write useSecureCookie := !isDevOrTest?

// Session management and authentication middleware
noSessionTimeout := v.GetBool("no-session-timeout")
sessionCookieMiddleware := auth.SessionCookieMiddleware(zapLogger, clientAuthSecretKey, noSessionTimeout, myHostname, officeHostname, tspHostname)
maskedCSRFMiddleware := auth.MaskedCSRFMiddleware(zapLogger, noSessionTimeout)
sessionCookieMiddleware := auth.SessionCookieMiddleware(zapLogger, clientAuthSecretKey, noSessionTimeout, myHostname, officeHostname, tspHostname, useSecureCookie)
maskedCSRFMiddleware := auth.MaskedCSRFMiddleware(zapLogger, noSessionTimeout, useSecureCookie)
userAuthMiddleware := authentication.UserAuthMiddleware(zapLogger)
clientCertMiddleware := authentication.ClientCertMiddleware(zapLogger, dbConnection)

handlerContext := handlers.NewHandlerContext(dbConnection, zapLogger)
handlerContext.SetCookieSecret(clientAuthSecretKey)
handlerContext.SetUseSecureCookie(useSecureCookie)
if noSessionTimeout {
handlerContext.SetNoSessionTimeout()
}
@@ -1166,16 +1170,16 @@ func main() {
authMux := goji.SubMux()
root.Handle(pat.New("/auth/*"), authMux)
authMux.Handle(pat.Get("/login-gov"), authentication.RedirectHandler{Context: authContext})
authMux.Handle(pat.Get("/login-gov/callback"), authentication.NewCallbackHandler(authContext, dbConnection, clientAuthSecretKey, noSessionTimeout))
authMux.Handle(pat.Post("/logout"), authentication.NewLogoutHandler(authContext, clientAuthSecretKey, noSessionTimeout))
authMux.Handle(pat.Get("/login-gov/callback"), authentication.NewCallbackHandler(authContext, dbConnection, clientAuthSecretKey, noSessionTimeout, useSecureCookie))
authMux.Handle(pat.Post("/logout"), authentication.NewLogoutHandler(authContext, clientAuthSecretKey, noSessionTimeout, useSecureCookie))

if isDevOrTest {
logger.Info("Enabling devlocal auth")
localAuthMux := goji.SubMux()
root.Handle(pat.New("/devlocal-auth/*"), localAuthMux)
localAuthMux.Handle(pat.Get("/login"), authentication.NewUserListHandler(authContext, dbConnection))
localAuthMux.Handle(pat.Post("/login"), authentication.NewAssignUserHandler(authContext, dbConnection, clientAuthSecretKey, noSessionTimeout))
localAuthMux.Handle(pat.Post("/new"), authentication.NewCreateAndLoginUserHandler(authContext, dbConnection, clientAuthSecretKey, noSessionTimeout))
localAuthMux.Handle(pat.Post("/new"), authentication.NewCreateAndLoginUserHandler(authContext, dbConnection, clientAuthSecretKey, noSessionTimeout, useSecureCookie))
localAuthMux.Handle(pat.Post("/create"), authentication.NewCreateUserHandler(authContext, dbConnection, clientAuthSecretKey, noSessionTimeout))

devlocalCa, err := ioutil.ReadFile(v.GetString("devlocal-ca")) // #nosec
@@ -89,14 +89,16 @@ type LogoutHandler struct {
Context
clientAuthSecretKey string
noSessionTimeout bool
useSecureCookie bool
}

// NewLogoutHandler creates a new LogoutHandler
func NewLogoutHandler(ac Context, clientAuthSecretKey string, noSessionTimeout bool) LogoutHandler {
func NewLogoutHandler(ac Context, clientAuthSecretKey string, noSessionTimeout bool, useSecureCookie bool) LogoutHandler {
handler := LogoutHandler{
Context: ac,
clientAuthSecretKey: clientAuthSecretKey,
noSessionTimeout: noSessionTimeout,
useSecureCookie: useSecureCookie,
}
return handler
}
@@ -118,7 +120,7 @@ func (h LogoutHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// This operation will delete all cookies from the session
session.IDToken = ""
session.UserID = uuid.Nil
auth.WriteSessionCookie(w, session, h.clientAuthSecretKey, h.noSessionTimeout, h.logger)
auth.WriteSessionCookie(w, session, h.clientAuthSecretKey, h.noSessionTimeout, h.logger, h.useSecureCookie)
http.Redirect(w, r, logoutURL, http.StatusSeeOther)
} else {
// Can't log out of login.gov without a token, redirect and let them re-auth
@@ -156,15 +158,17 @@ type CallbackHandler struct {
db *pop.Connection
clientAuthSecretKey string
noSessionTimeout bool
useSecureCookie bool
}

// NewCallbackHandler creates a new CallbackHandler
func NewCallbackHandler(ac Context, db *pop.Connection, clientAuthSecretKey string, noSessionTimeout bool) CallbackHandler {
func NewCallbackHandler(ac Context, db *pop.Connection, clientAuthSecretKey string, noSessionTimeout bool, useSecureCookie bool) CallbackHandler {
handler := CallbackHandler{
Context: ac,
db: db,
clientAuthSecretKey: clientAuthSecretKey,
noSessionTimeout: noSessionTimeout,
useSecureCookie: useSecureCookie,
}
return handler
}
@@ -321,7 +325,7 @@ func authorizeKnownUser(userIdentity *models.UserIdentity, h CallbackHandler, se

h.logger.Info("logged in", zap.Any("session", session))

auth.WriteSessionCookie(w, session, h.clientAuthSecretKey, h.noSessionTimeout, h.logger)
auth.WriteSessionCookie(w, session, h.clientAuthSecretKey, h.noSessionTimeout, h.logger, h.useSecureCookie)
http.Redirect(w, r, lURL, http.StatusTemporaryRedirect)
}

@@ -379,7 +383,7 @@ func authorizeUnknownUser(openIDUser goth.User, h CallbackHandler, session *auth

h.logger.Info("logged in", zap.Any("session", session))

auth.WriteSessionCookie(w, session, h.clientAuthSecretKey, h.noSessionTimeout, h.logger)
auth.WriteSessionCookie(w, session, h.clientAuthSecretKey, h.noSessionTimeout, h.logger, h.useSecureCookie)
http.Redirect(w, r, h.landingURL(session), http.StatusTemporaryRedirect)
}

@@ -73,7 +73,7 @@ func (suite *AuthSuite) TestAuthorizationLogoutHandler() {
req = req.WithContext(ctx)

authContext := NewAuthContext(suite.logger, fakeLoginGovProvider(suite.logger), "http", callbackPort)
handler := LogoutHandler{authContext, "fake key", false}
handler := LogoutHandler{authContext, "fake key", false, false}

rr := httptest.NewRecorder()
handler.ServeHTTP(rr, req.WithContext(ctx))
@@ -168,6 +168,7 @@ func (suite *AuthSuite) TestAuthorizeDisableUser() {
suite.DB(),
"fake key",
false,
false,
}
rr := httptest.NewRecorder()
span := trace.Span{}
@@ -108,6 +108,7 @@ type devlocalAuthHandler struct {
db *pop.Connection
clientAuthSecretKey string
noSessionTimeout bool
useSecureCookie bool
}

// AssignUserHandler logs a user in directly
@@ -194,12 +195,13 @@ func createUser(h devlocalAuthHandler, w http.ResponseWriter, r *http.Request) m
type CreateAndLoginUserHandler devlocalAuthHandler

// NewCreateAndLoginUserHandler creates a new CreateAndLoginUserHandler
func NewCreateAndLoginUserHandler(ac Context, db *pop.Connection, clientAuthSecretKey string, noSessionTimeout bool) CreateAndLoginUserHandler {
func NewCreateAndLoginUserHandler(ac Context, db *pop.Connection, clientAuthSecretKey string, noSessionTimeout bool, useSecureCookie bool) CreateAndLoginUserHandler {
handler := CreateAndLoginUserHandler{
Context: ac,
db: db,
clientAuthSecretKey: clientAuthSecretKey,
noSessionTimeout: noSessionTimeout,
useSecureCookie: useSecureCookie,
}
return handler
}
@@ -265,7 +267,7 @@ func loginUser(handler devlocalAuthHandler, user *models.User, w http.ResponseWr
}

handler.logger.Info("logged in", zap.Any("session", session))
auth.WriteSessionCookie(w, session, handler.clientAuthSecretKey, handler.noSessionTimeout, handler.logger)
auth.WriteSessionCookie(w, session, handler.clientAuthSecretKey, handler.noSessionTimeout, handler.logger, handler.useSecureCookie)

lURL := handler.landingURL(session)
http.Redirect(w, r, lURL, http.StatusSeeOther)
@@ -15,7 +15,7 @@ func (suite *AuthSuite) TestCreateUserHandler() {
req := httptest.NewRequest("POST", "/devlocal-auth/create", nil)

authContext := NewAuthContext(suite.logger, fakeLoginGovProvider(suite.logger), "http", 1234)
handler := CreateUserHandler{authContext, suite.DB(), "fake key", false}
handler := CreateUserHandler{authContext, suite.DB(), "fake key", false, false}
handler.ServeHTTP(rr, req)

suite.Equal(http.StatusOK, rr.Code, "handler returned wrong status code")
@@ -111,7 +111,7 @@ func sessionClaimsFromRequest(logger *zap.Logger, secret string, appName Applica
}

// WriteMaskedCSRFCookie update the masked_gorilla_csrf cookie value
func WriteMaskedCSRFCookie(w http.ResponseWriter, csrfToken string, noSessionTimeout bool, logger *zap.Logger) {
func WriteMaskedCSRFCookie(w http.ResponseWriter, csrfToken string, noSessionTimeout bool, logger *zap.Logger, useSecureCookie bool) {

expiry := GetExpiryTimeFromMinutes(SessionExpiryInMinutes)
maxAge := sessionExpiryInSeconds
@@ -123,23 +123,26 @@ func WriteMaskedCSRFCookie(w http.ResponseWriter, csrfToken string, noSessionTim

// New cookie
cookie := http.Cookie{
Name: MaskedGorillaCSRFToken,
Value: csrfToken,
Path: "/",
Expires: expiry,
MaxAge: maxAge,
Name: MaskedGorillaCSRFToken,
Value: csrfToken,
Path: "/",
Expires: expiry,
MaxAge: maxAge,
HttpOnly: false, // must be false to be read by POST/PUT/PATCH/DELETE requests

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Mar 13, 2019

Contributor

The comment here I believe is must be false to be read by client for use in POST/PUT/PATCH/DELETE requests.

SameSite: http.SameSiteStrictMode,
Secure: useSecureCookie,
}

http.SetCookie(w, &cookie)
}

// MaskedCSRFMiddleware handles setting the CSRF Token cookie
func MaskedCSRFMiddleware(logger *zap.Logger, noSessionTimeout bool) func(next http.Handler) http.Handler {
func MaskedCSRFMiddleware(logger *zap.Logger, noSessionTimeout bool, useSecureCookie bool) func(next http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
mw := func(w http.ResponseWriter, r *http.Request) {
// Write a CSRF cookie if none exists
if _, err := GetCookie(MaskedGorillaCSRFToken, r); err != nil {
WriteMaskedCSRFCookie(w, csrf.Token(r), noSessionTimeout, logger)
WriteMaskedCSRFCookie(w, csrf.Token(r), noSessionTimeout, logger, useSecureCookie)
}
next.ServeHTTP(w, r)
}
@@ -148,16 +151,18 @@ func MaskedCSRFMiddleware(logger *zap.Logger, noSessionTimeout bool) func(next h
}

// WriteSessionCookie update the cookie for the session
func WriteSessionCookie(w http.ResponseWriter, session *Session, secret string, noSessionTimeout bool, logger *zap.Logger) {
func WriteSessionCookie(w http.ResponseWriter, session *Session, secret string, noSessionTimeout bool, logger *zap.Logger, useSecureCookie bool) {

// Delete the cookie
cookieName := fmt.Sprintf("%s_%s", string(session.ApplicationName), UserSessionCookieName)
cookie := http.Cookie{
Name: cookieName,
Value: "blank",
Path: "/",
Expires: time.Unix(0, 0),
MaxAge: -1,
Name: cookieName,
Value: "blank",
Path: "/",
Expires: time.Unix(0, 0),
MaxAge: -1,
SameSite: http.SameSiteStrictMode,
Secure: useSecureCookie,
}

// unless we have a valid session
@@ -206,7 +211,7 @@ func ApplicationName(hostname, milHostname, officeHostname, tspHostname string)
}

// SessionCookieMiddleware handle serializing and de-serializing the session between the user_session cookie and the request context
func SessionCookieMiddleware(logger *zap.Logger, secret string, noSessionTimeout bool, milHostname, officeHostname, tspHostname string) func(next http.Handler) http.Handler {
func SessionCookieMiddleware(logger *zap.Logger, secret string, noSessionTimeout bool, milHostname, officeHostname, tspHostname string, useSecureCookie bool) func(next http.Handler) http.Handler {
logger.Info("Creating session", zap.String("milHost", milHostname), zap.String("officeHost", officeHostname), zap.String("tspHost", tspHostname))
return func(next http.Handler) http.Handler {
mw := func(w http.ResponseWriter, r *http.Request) {
@@ -237,7 +242,7 @@ func SessionCookieMiddleware(logger *zap.Logger, secret string, noSessionTimeout
ctx = SetSessionInRequestContext(r.WithContext(ctx), &session)

// And update the cookie. May get over-ridden later
WriteSessionCookie(w, &session, secret, noSessionTimeout, logger)
WriteSessionCookie(w, &session, secret, noSessionTimeout, logger, useSecureCookie)

span.AddTraceField("auth.application_name", session.ApplicationName)
span.AddTraceField("auth.hostname", session.Hostname)
@@ -62,7 +62,7 @@ func (suite *authSuite) TestSessionCookieMiddlewareWithBadToken() {
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
resultingSession = SessionFromRequestContext(r)
})
middleware := SessionCookieMiddleware(suite.logger, pem, false, MilTestHost, OfficeTestHost, TspTestHost)(handler)
middleware := SessionCookieMiddleware(suite.logger, pem, false, MilTestHost, OfficeTestHost, TspTestHost, false)(handler)

expiry := GetExpiryTimeFromMinutes(SessionExpiryInMinutes)
rr, req := getHandlerParamsWithToken(fakeToken, expiry)
@@ -104,7 +104,7 @@ func (suite *authSuite) TestSessionCookieMiddlewareWithValidToken() {
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
resultingSession = SessionFromRequestContext(r)
})
middleware := SessionCookieMiddleware(suite.logger, pem, false, MilTestHost, OfficeTestHost, TspTestHost)(handler)
middleware := SessionCookieMiddleware(suite.logger, pem, false, MilTestHost, OfficeTestHost, TspTestHost, false)(handler)

middleware.ServeHTTP(rr, req)

@@ -146,7 +146,7 @@ func (suite *authSuite) TestSessionCookieMiddlewareWithExpiredToken() {
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
resultingSession = SessionFromRequestContext(r)
})
middleware := SessionCookieMiddleware(suite.logger, pem, false, MilTestHost, OfficeTestHost, TspTestHost)(handler)
middleware := SessionCookieMiddleware(suite.logger, pem, false, MilTestHost, OfficeTestHost, TspTestHost, false)(handler)

rr, req := getHandlerParamsWithToken(ss, expiry)

@@ -192,9 +192,9 @@ func (suite *authSuite) TestSessionCookiePR161162731() {
var resultingSession *Session
handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
resultingSession = SessionFromRequestContext(r)
WriteSessionCookie(w, resultingSession, "freddy", false, suite.logger)
WriteSessionCookie(w, resultingSession, "freddy", false, suite.logger, false)
})
middleware := SessionCookieMiddleware(suite.logger, pem, false, MilTestHost, OfficeTestHost, TspTestHost)(handler)
middleware := SessionCookieMiddleware(suite.logger, pem, false, MilTestHost, OfficeTestHost, TspTestHost, false)(handler)

middleware.ServeHTTP(rr, req)

@@ -215,7 +215,7 @@ func (suite *authSuite) TestMaskedCSRFMiddleware() {
req, _ := http.NewRequest("GET", "/", nil)

handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
middleware := MaskedCSRFMiddleware(suite.logger, false)(handler)
middleware := MaskedCSRFMiddleware(suite.logger, false, false)(handler)
middleware.ServeHTTP(rr, req)

// We should get a 200 OK
@@ -242,7 +242,7 @@ func (suite *authSuite) TestMaskedCSRFMiddlewareCreatesOneToken() {
req.AddCookie(&cookie)

handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
middleware := MaskedCSRFMiddleware(suite.logger, false)(handler)
middleware := MaskedCSRFMiddleware(suite.logger, false, false)(handler)
middleware.ServeHTTP(rr, req)

// We should get a 200 OK
@@ -254,7 +254,7 @@ func (suite *authSuite) TestMaskedCSRFMiddlewareCreatesOneToken() {
}

func (suite *authSuite) TestMiddlewareConstructor() {
adm := SessionCookieMiddleware(suite.logger, "secret", false, MilTestHost, OfficeTestHost, TspTestHost)
adm := SessionCookieMiddleware(suite.logger, "secret", false, MilTestHost, OfficeTestHost, TspTestHost, false)
suite.NotNil(adm)
}

@@ -268,7 +268,7 @@ func (suite *authSuite) TestMiddlewareMilApp() {
suite.False(session.IsTspApp(), "first should not be tsp app")
suite.Equal(MilTestHost, session.Hostname)
})
milMoveMiddleware := SessionCookieMiddleware(suite.logger, "secret", false, MilTestHost, OfficeTestHost, TspTestHost)(milMoveTestHandler)
milMoveMiddleware := SessionCookieMiddleware(suite.logger, "secret", false, MilTestHost, OfficeTestHost, TspTestHost, false)(milMoveTestHandler)

req := httptest.NewRequest("GET", fmt.Sprintf("http://%s/some_url", MilTestHost), nil)
milMoveMiddleware.ServeHTTP(rr, req)
@@ -290,7 +290,7 @@ func (suite *authSuite) TestMiddlwareOfficeApp() {
suite.False(session.IsTspApp(), "should not be tsp app")
suite.Equal(OfficeTestHost, session.Hostname)
})
officeMiddleware := SessionCookieMiddleware(suite.logger, "secret", false, MilTestHost, OfficeTestHost, TspTestHost)(officeTestHandler)
officeMiddleware := SessionCookieMiddleware(suite.logger, "secret", false, MilTestHost, OfficeTestHost, TspTestHost, false)(officeTestHandler)

req := httptest.NewRequest("GET", fmt.Sprintf("http://%s/some_url", OfficeTestHost), nil)
officeMiddleware.ServeHTTP(rr, req)
@@ -312,7 +312,7 @@ func (suite *authSuite) TestMiddlwareTspApp() {
suite.True(session.IsTspApp(), "should be tsp app")
suite.Equal(TspTestHost, session.Hostname)
})
tspMiddleware := SessionCookieMiddleware(suite.logger, "secret", false, MilTestHost, OfficeTestHost, TspTestHost)(tspTestHandler)
tspMiddleware := SessionCookieMiddleware(suite.logger, "secret", false, MilTestHost, OfficeTestHost, TspTestHost, false)(tspTestHandler)

req := httptest.NewRequest("GET", fmt.Sprintf("http://%s/some_url", TspTestHost), nil)
tspMiddleware.ServeHTTP(rr, req)
@@ -330,7 +330,7 @@ func (suite *authSuite) TestMiddlewareBadApp() {
noAppTestHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
suite.Fail("Should not be called")
})
noAppMiddleware := SessionCookieMiddleware(suite.logger, "secret", false, MilTestHost, OfficeTestHost, TspTestHost)(noAppTestHandler)
noAppMiddleware := SessionCookieMiddleware(suite.logger, "secret", false, MilTestHost, OfficeTestHost, TspTestHost, false)(noAppTestHandler)

req := httptest.NewRequest("GET", "http://totally.bogus.hostname/some_url", nil)
noAppMiddleware.ServeHTTP(rr, req)
@@ -37,6 +37,8 @@ type HandlerContext interface {
SetIWSPersonLookup(rbs iws.PersonLookup)
SendProductionInvoice() bool
SetSendProductionInvoice(sendProductionInvoice bool)
UseSecureCookie() bool
SetUseSecureCookie(useSecureCookie bool)

GexSender() services.GexSender
SetGexSender(gexSender services.GexSender)
@@ -62,6 +64,7 @@ type handlerContext struct {
dpsAuthParams dpsauth.Params
senderToGex services.GexSender
icnSequencer sequence.Sequencer
useSecureCookie bool
}

// NewHandlerContext returns a new handlerContext with its required private fields set.
@@ -190,3 +193,13 @@ func (hctx *handlerContext) DPSAuthParams() dpsauth.Params {
func (hctx *handlerContext) SetDPSAuthParams(params dpsauth.Params) {
hctx.dpsAuthParams = params
}

// UseSecureCookie determines if the field "Secure" is set to true or false upon cookie creation
func (hctx *handlerContext) UseSecureCookie() bool {
return hctx.useSecureCookie
}

// Sets flag for using Secure cookie
func (hctx *handlerContext) SetUseSecureCookie(useSecureCookie bool) {
hctx.useSecureCookie = useSecureCookie
}
@@ -148,7 +148,7 @@ func (h CreateServiceMemberHandler) Handle(params servicememberop.CreateServiceM
// And return
serviceMemberPayload := payloadForServiceMemberModel(h.FileStorer(), newServiceMember)
responder := servicememberop.NewCreateServiceMemberCreated().WithPayload(serviceMemberPayload)
return handlers.NewCookieUpdateResponder(params.HTTPRequest, h.CookieSecret(), h.NoSessionTimeout(), h.Logger(), responder)
return handlers.NewCookieUpdateResponder(params.HTTPRequest, h.CookieSecret(), h.NoSessionTimeout(), h.Logger(), responder, h.UseSecureCookie())
}

// ShowServiceMemberHandler returns a serviceMember for a user and service member ID
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.