Skip to content

Commit

Permalink
MB-2321 Allow revoking individual user sessions
Browse files Browse the repository at this point in the history
**Description**:
In order to obtain an ATO (Authority to Operate) for MilMove, we need
to provide a way to revoke individual user sessions. Currently,
session management is provided by JWTs per ADR 15, but JWTs aren't
designed to be revoked on an individual basis. Instead, we need to
store session data on the server.

In this PR, we have chosen Redis because it automatically deletes
expired sessions. With Postgres, we would need to run a routine
periodically to clean up stale sessions.

After researching various session management solutions, I chose
`scs` because it was the easiest to integrate, and it supports various
stores out of the box. It is the second most popular repo after
`gorilla/sessions`. I didn't pick `gorilla/sessions` because it suffers
from memory leak issues, and uses its own `context` instead of the
`request.Context()` provided by Golang. The maintainers are aware of
the issues and have opened a GitHub issue to propose improvements for
v2. However, it doesn't look like any progress has been made over the
past 2 years, while `scs` has implemented most of those improvements.

The name of the Redis key that holds the session data is based on the
format `scs:session:token`, where `token` is the session cookie value.
In order to revoke an individual session, we need to know the token
corresponding to the user's session. To facilitate that lookup, I
added a new `session-id` to the RequestLogger.

**Setup**:
`docker pull redis`

**Reviewer Notes**:
Things to test:

**milmovelocal auth**
1. Go to milmovelocal:3000
- [ ] Verify that a session cookie named "mil_session_token" is present
(Developer Tools -> Application tab -> Cookies (under Storage in the
left sidebar))
- [ ] Verify that the value in the `Expires/Max-Age` column is `Session`
- [ ] Verify that the HttpOnly column is checked
- [ ] Verify that the Path is `/`
- [ ] Verify that `SameSite` is `Lax`
2. In your Terminal, run `redis-cli`, then type `KEYS *`
- [ ] Verify there is an entry labeled `scs:session:token`, where
`token` is the `Value` of the `mil_session_token` cookie
3. Sign in
- [ ] Verify that after successful sign in, the `Value` of the
`mil_session_token` cookie changes
4. Run `KEYS *` again in the Redis console
- [ ] Verify that the previous entry is gone and that a new one
corresponding to the new session cookie is present
- [ ] Verify that there is a `session-id` entry in the
`middleware/request_logger.go` output that is the same value as the
current browser cookie, not the previous one before the user signed in
5. Sign out
- [ ] Verify that the previous entry in Redis is gone and that a new one
corresponding to the new session cookie is present
- [ ] Verify that the session cookie changed in the browser
6. In `serve.go`, on line 504, change the IdleTimeout from 15 minutes
to 1 minute
7. Sign in, then wait a little over a minute
8. Try to make a new request without refreshing the browser, for
example, filling out the moves form and clicking the Next button
- [ ] Verify that you are not able to make a request and that you see
an Unauthorized Error. Ideally, the user would be redirected to the
sign in page. I'm working on implementing that.

**devlocal auth**
- [ ] Verify you can sign in and out via devlocal auth flow:
http://milmovelocal:3000/devlocal-auth/login
- [ ] Verify you can create a New milmove User
- [ ] Verify you can create a New dps User

**Role based auth**
1. In your `.envrc.local`, add
`export FEATURE_FLAG_ROLE_BASED_AUTH=true`
2. Stop the server, run `direnv allow`
3. run `echo $FEATURE_FLAG_ROLE_BASED_AUTH` to make sure it's `true`
4. run `make server_run`
5. Go to milmovelocal:3000 and make sure you can sign in and out

**References**:
[gorilla sessions issues](gorilla/sessions#105)
[scs repo](https://github.com/alexedwards/scs)
  • Loading branch information
monfresh committed Apr 14, 2020
1 parent b2771f9 commit 7218672
Show file tree
Hide file tree
Showing 16 changed files with 370 additions and 526 deletions.
61 changes: 51 additions & 10 deletions cmd/milmove/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"context"
"crypto/tls"
"encoding/gob"
"encoding/hex"
"encoding/json"
"fmt"
Expand All @@ -18,13 +19,17 @@ import (
"strings"
"sync"
"syscall"
"time"

"github.com/alexedwards/scs/redisstore"
"github.com/alexedwards/scs/v2"
"github.com/aws/aws-sdk-go/aws/credentials"
"github.com/aws/aws-sdk-go/aws/credentials/stscreds"
awssession "github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/sts"
"github.com/dgrijalva/jwt-go"
"github.com/gobuffalo/pop"
"github.com/gomodule/redigo/redis"
"github.com/gorilla/csrf"
"github.com/pkg/errors"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -477,10 +482,45 @@ func serveFunction(cmd *cobra.Command, args []string) error {
logger.Fatal("Registering login provider", zap.Error(err))
}

gob.Register(auth.Session{})
var sessionManager *scs.SessionManager
sessionManager = scs.New()
pool := &redis.Pool{
MaxIdle: 10,
Dial: func() (redis.Conn, error) {
return redis.Dial("tcp", "localhost:6379")
},
}
sessionManager.Store = redisstore.New(pool)

// IdleTimeout controls the maximum length of time a session can be inactive
// before it expires. For example, some applications may wish to set this so
// there is a timeout after 20 minutes of inactivity. By default IdleTimeout
// is not set and there is no inactivity timeout.
// This preserves the behavior we had when we managed our own session
// cookies, where we extended the session expiry by 15 minutes on every
// request while the session was still valid.
sessionManager.IdleTimeout = 15 * time.Minute

// Lifetime controls the maximum length of time that a session is valid for
// before it expires. The lifetime is an 'absolute expiry' which is set when
// the session is first created and does not change. The default value is 24
// hours.
// sessionManager.Lifetime = 24 * time.Hour

sessionManager.Cookie.Path = "/"

// A value of false means the session cookie will be deleted when the
// browser is closed.
sessionManager.Cookie.Persist = false

useSecureCookie := !isDevOrTest
if useSecureCookie {
sessionManager.Cookie.Secure = true
}
// Session management and authentication middleware
noSessionTimeout := v.GetBool(cli.NoSessionTimeoutFlag)
sessionCookieMiddleware := auth.SessionCookieMiddleware(logger, clientAuthSecretKey, noSessionTimeout, appnames, useSecureCookie)
sessionCookieMiddleware := auth.SessionCookieMiddleware(logger, appnames, sessionManager)
maskedCSRFMiddleware := auth.MaskedCSRFMiddleware(logger, useSecureCookie)
userAuthMiddleware := authentication.UserAuthMiddleware(logger)
if v.GetBool(cli.FeatureFlagRoleBasedAuth) {
Expand All @@ -495,6 +535,7 @@ func serveFunction(cmd *cobra.Command, args []string) error {
handlerContext.SetUseSecureCookie(useSecureCookie)
if noSessionTimeout {
handlerContext.SetNoSessionTimeout()
sessionManager.IdleTimeout = 24 * time.Hour
}

handlerContext.SetAppNames(appnames)
Expand Down Expand Up @@ -809,7 +850,7 @@ func serveFunction(cmd *cobra.Command, args []string) error {
root.Use(csrf.Protect(csrfAuthKey, csrf.Secure(!isDevOrTest), csrf.Path("/"), csrf.CookieName(auth.GorillaCSRFToken)))
root.Use(maskedCSRFMiddleware)

site.Handle(pat.New("/*"), root)
site.Handle(pat.New("/*"), sessionManager.LoadAndSave(root))

if v.GetBool(cli.ServeAPIInternalFlag) {
internalMux := goji.SubMux()
Expand All @@ -827,7 +868,7 @@ func serveFunction(cmd *cobra.Command, args []string) error {
internalMux.Handle(pat.New("/*"), internalAPIMux)
internalAPIMux.Use(userAuthMiddleware)
internalAPIMux.Use(middleware.NoCache(logger))
api := internalapi.NewInternalAPI(handlerContext)
api := internalapi.NewInternalAPI(handlerContext, sessionManager)
internalAPIMux.Handle(pat.New("/*"), api.Serve(nil))
if handlerContext.GetFeatureFlag(cli.FeatureFlagRoleBasedAuth) {
internalAPIMux.Use(roleAuthMiddleware(api.Context()))
Expand Down Expand Up @@ -886,18 +927,18 @@ func serveFunction(cmd *cobra.Command, args []string) error {
)
authMux := goji.SubMux()
root.Handle(pat.New("/auth/*"), authMux)
authMux.Handle(pat.Get("/login-gov"), authentication.RedirectHandler{Context: authContext, UseSecureCookie: useSecureCookie})
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))
authMux.Handle(pat.Get("/login-gov"), authentication.NewRedirectHandler(authContext, sessionManager))
authMux.Handle(pat.Get("/login-gov/callback"), authentication.NewCallbackHandler(authContext, dbConnection, sessionManager))
authMux.Handle(pat.Post("/logout"), authentication.NewLogoutHandler(authContext, sessionManager))

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

if stringSliceContains([]string{cli.EnvironmentTest, cli.EnvironmentDevelopment}, v.GetString(cli.EnvironmentFlag)) {
logger.Info("Adding devlocal CA to root CAs")
Expand Down
6 changes: 5 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ require (
github.com/0xAX/notificator v0.0.0-20191016112426-3962a5ea8da1 // indirect
github.com/99designs/aws-vault v4.5.1+incompatible
github.com/99designs/keyring v1.1.4
github.com/alexedwards/scs/redisstore v0.0.0-20200225172727-3308e1066830
github.com/alexedwards/scs/v2 v2.3.0
github.com/aws/aws-sdk-go v1.30.7
github.com/codegangsta/envy v0.0.0-20141216192214-4b78388c8ce4 // indirect
github.com/codegangsta/gin v0.0.0-20171026143024-cafe2ce98974
Expand Down Expand Up @@ -34,6 +36,7 @@ require (
github.com/gocarina/gocsv v0.0.0-20190927101021-3ecffd272576
github.com/gofrs/flock v0.7.1
github.com/gofrs/uuid v3.2.0+incompatible
github.com/gomodule/redigo v2.0.0+incompatible
github.com/gorilla/csrf v1.6.2
github.com/imdario/mergo v0.3.9
github.com/jessevdk/go-flags v1.4.0
Expand Down Expand Up @@ -62,9 +65,10 @@ require (
go.mozilla.org/pkcs7 v0.0.0-20181213175627-3cffc6fbfe83
go.uber.org/zap v1.14.1
goji.io v2.0.2+incompatible
golang.org/x/crypto v0.0.0-20200128174031-69ecbb4d6d5d
golang.org/x/crypto v0.0.0-20200317142112-1b76d66859c6
golang.org/x/net v0.0.0-20200226121028-0de0cce0169b
golang.org/x/text v0.3.2
google.golang.org/appengine v1.6.5 // indirect
gopkg.in/alexcesaro/quotedprintable.v3 v3.0.0-20150716171945-2caba252f4dc // indirect
gopkg.in/go-playground/assert.v1 v1.2.1 // indirect
gopkg.in/go-playground/validator.v9 v9.31.0
Expand Down
12 changes: 10 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578/go.mod h1:uGdko
github.com/agnivade/levenshtein v1.0.1/go.mod h1:CURSv5d9Uaml+FovSIICkLbAUZ9S4RqaHDIsdSBg7lM=
github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
github.com/alexedwards/scs/redisstore v0.0.0-20200225172727-3308e1066830 h1:84mrg6CV1OZ8RnRZ6zkJ4bvjg/6CHXPPVDKQKecVb+0=
github.com/alexedwards/scs/redisstore v0.0.0-20200225172727-3308e1066830/go.mod h1:u2uSOc9yz8e3S+beMudSPwYL36kcbBChOLBJDAQNy38=
github.com/alexedwards/scs/v2 v2.3.0 h1:V8rtn2P5QGh8C9S7T/ikBo/AdA27vDoQJPbiAaOCmFg=
github.com/alexedwards/scs/v2 v2.3.0/go.mod h1:ToaROZxyKukJKT/xLcVQAChi5k6+Pn1Gvmdl7h3RRj8=
github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883/go.mod h1:rCTlJbsFo29Kk6CurOXKm700vrz8f0KW0JNfpkRJY/8=
github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8=
github.com/asaskevich/govalidator v0.0.0-20180720115003-f9ffefc3facf/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY=
Expand Down Expand Up @@ -264,6 +268,8 @@ github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5y
github.com/golang/protobuf v1.3.1 h1:YF8+flBXS5eO826T4nzqPrxfhQThhXl0YzfuUPu4SBg=
github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/snappy v0.0.1/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q=
github.com/gomodule/redigo v2.0.0+incompatible h1:K/R+8tc58AaqLkqG2Ol3Qk+DR/TlNuhuh457pBFPtt0=
github.com/gomodule/redigo v2.0.0+incompatible/go.mod h1:B4C85qUVwatsJoIUNIfCRsp7qO0iAmpGFZ4EELWSbC4=
github.com/google/btree v1.0.0/go.mod h1:lNA+9X1NB3Zf8V7Ke586lFgjr2dZNuvo3lPJSGZ5JPQ=
github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5aqRK0M=
github.com/google/go-cmp v0.3.0 h1:crn/baboCvb5fXaQ0IJ1SGTsTVrWpDsCWC8EGETZijY=
Expand Down Expand Up @@ -596,8 +602,8 @@ golang.org/x/crypto v0.0.0-20190617133340-57b3e21c3d56/go.mod h1:yigFU9vqHzYiE8U
golang.org/x/crypto v0.0.0-20190621222207-cc06ce4a13d4/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
golang.org/x/crypto v0.0.0-20190701094942-4def268fd1a4/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
golang.org/x/crypto v0.0.0-20190820162420-60c769a6c586/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
golang.org/x/crypto v0.0.0-20200128174031-69ecbb4d6d5d h1:9FCpayM9Egr1baVnV1SX0H87m+XB0B8S0hAMi99X/3U=
golang.org/x/crypto v0.0.0-20200128174031-69ecbb4d6d5d/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
golang.org/x/crypto v0.0.0-20200317142112-1b76d66859c6 h1:TjszyFsQsyZNHwdVdZ5m7bjmreu0znc2kRYsEml9/Ww=
golang.org/x/crypto v0.0.0-20200317142112-1b76d66859c6/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
golang.org/x/image v0.0.0-20190823064033-3a9bac650e44/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
golang.org/x/image v0.0.0-20190910094157-69e4b8554b2a h1:gHevYm0pO4QUbwy8Dmdr01R5r1BuKtfYqRqF0h/Cbh0=
golang.org/x/image v0.0.0-20190910094157-69e4b8554b2a/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
Expand Down Expand Up @@ -696,6 +702,8 @@ google.golang.org/appengine v1.1.0/go.mod h1:EbEs0AVv82hx2wNQdGPgUI5lhzA/G0D9Ywl
google.golang.org/appengine v1.2.0/go.mod h1:xpcJRLb0r/rnEns0DIKYYv+WjYCduHsrkT7/EB5XEv4=
google.golang.org/appengine v1.6.1 h1:QzqyMA1tlu6CgqCDUtU9V+ZKhLFT2dkJuANu5QaxI3I=
google.golang.org/appengine v1.6.1/go.mod h1:i06prIuMbXzDqacNJfV5OdTW448YApPu5ww/cMBSeb0=
google.golang.org/appengine v1.6.5 h1:tycE03LOZYQNhDpS27tcQdAzLCVMaj7QT2SXxebnpCM=
google.golang.org/appengine v1.6.5/go.mod h1:8WjMMxjGQR8xUklV/ARdw2HLXBOI7O7uCIDZVag1xfc=
google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8/go.mod h1:JiN7NxoALGmiZfu7CAH4rXhgtRTLTxftemlI0sWmxmc=
google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c=
google.golang.org/grpc v1.21.0/go.mod h1:oYelfM1adQP15Ek0mdvEgi9Df8B9CZIaU1084ijfRaM=
Expand Down
Loading

0 comments on commit 7218672

Please sign in to comment.