From 60864d4aa7f28dfc02f06d7738a5dac04644310a Mon Sep 17 00:00:00 2001 From: Gavin Date: Thu, 16 May 2024 08:59:27 -0700 Subject: [PATCH] Get auth token from cookie ``` $ curl --cookie "kbase_session=$KBASE_CI_TOKEN" http://localhost:8080/node/06cc2b59-b33c-460a-8332-151a509b6d20/acl { "data": { "delete": [ "057abd01-95db-4500-9541-8ce2a925c3e3" ], "owner": "057abd01-95db-4500-9541-8ce2a925c3e3", "public": { "delete": false, "read": false, "write": false }, "read": [ "057abd01-95db-4500-9541-8ce2a925c3e3" ], "write": [ "057abd01-95db-4500-9541-8ce2a925c3e3" ] }, "error": null, "status": 200 } $ curl --cookie "kbase_sessionx=$KBASE_CI_TOKEN" http://localhost:8080/node/06cc2b59-b33c-460a-8332-151a509b6d20/acl { "data": null, "error": [ "User Unauthorized" ], "status": 401 } ``` --- README.md | 13 ++- RELEASE_NOTES.md | 1 + app/blobstore.go | 2 +- config/config.go | 8 ++ config/config_test.go | 5 + deploy.cfg.example | 3 + deployment/conf/deployment.cfg.templ | 3 + docker-compose.yml | 43 ++------- service/errortypes.go | 20 +++- service/integration_test.go | 134 +++++++++++++++++++++++---- service/server.go | 16 ++++ 11 files changed, 189 insertions(+), 59 deletions(-) diff --git a/README.md b/README.md index cebf585..bb13968 100644 --- a/README.md +++ b/README.md @@ -101,8 +101,17 @@ This data structure is identical to Shock's error data structure. # API -Requests are authenticated by including the header `Authorization: OAuth ` in the -request. +## Authentication + +Requests are authenticated by including the header `Authorization: OAuth ` or +including a cookie with the value of `` in the request. + +The names of cookies that the server will check are set in the deployment configuration file. + +The header takes precedence, then each cookie in the list in the configuration file in order. + +Note that for backwards compatibility, incorrect or invalid authentication headers respond with a +400 HTTP code. Invalid cookies respond with the appropriate 401 code. ## Root diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index fb7efea..15ee12a 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,6 +1,7 @@ # 0.1.4 * Added the `del` param when downloading the file from a node. +* The Blobstore will now look for auth tokens in cookies specified in the deployment configuration. # 0.1.3 diff --git a/app/blobstore.go b/app/blobstore.go index f20e075..7cb9870 100644 --- a/app/blobstore.go +++ b/app/blobstore.go @@ -19,7 +19,7 @@ import ( const ( name = "blobstore" - version = "0.1.3" + version = "0.1.4" shockname = "Shock" shockver = "0.9.6" // do not increment deprecation = "The id and version fields are deprecated." diff --git a/config/config.go b/config/config.go index 7a51ac4..05dd7df 100644 --- a/config/config.go +++ b/config/config.go @@ -46,6 +46,9 @@ const ( // KeyAuthAdminRoles is the configuration key where the value is comma-delimited auth server // roles that denote that a user is a blobstore admin KeyAuthAdminRoles = "kbase-auth-admin-roles" + // KeyAuthTokenCookies is the configuartion key where the value is comma-delimited cookie + // names where the serivce should look for authentication tokens. + KeyAuthTokenCookies = "kbase-auth-token-cookies" // KeyDontTrustXIPHeaders is the configuration key where the value determines whether to // distrust the X-Forwarded-For and X-Real-IP headers (true) or not (anything else). KeyDontTrustXIPHeaders = "dont-trust-x-ip-headers" @@ -82,6 +85,9 @@ type Config struct { // AuthAdminRoles are the auth server roles that denote that a user is a blobstore admin. // It is never nil but may be empty. AuthAdminRoles *[]string + // AuthTokenCookies are the cookie names to check for auth tokens. + // It is never nil but may be empty. + AuthTokenCookies *[]string // DontTrustXIPHeaders determines whether to distrust the X-Forwarded-For and X-Real-IP // headers. DontTrustXIPHeaders bool @@ -112,6 +118,7 @@ func New(configFilePath string) (*Config, error) { s3region, err := getString(err, configFilePath, sec, KeyS3Region, true) authurl, err := getURL(err, configFilePath, sec, KeyAuthURL) roles, err := getStringList(err, configFilePath, sec, KeyAuthAdminRoles) + cookies, err := getStringList(err, configFilePath, sec, KeyAuthTokenCookies) xip, err := getString(err, configFilePath, sec, KeyDontTrustXIPHeaders, false) if err != nil { return nil, err @@ -137,6 +144,7 @@ func New(configFilePath string) (*Config, error) { S3Region: s3region, AuthURL: authurl, AuthAdminRoles: roles, + AuthTokenCookies: cookies, DontTrustXIPHeaders: "true" == xip, }, nil diff --git a/config/config_test.go b/config/config_test.go index 032143d..0351037 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -93,6 +93,7 @@ func (t *TestSuite) TestMinimalConfig() { S3DisableSSLVerify: false, AuthURL: u, AuthAdminRoles: &[]string{}, + AuthTokenCookies: &[]string{}, DontTrustXIPHeaders: false, } t.Equal(&expected, cfg, "incorrect config") @@ -114,6 +115,7 @@ func (t *TestSuite) TestMinimalConfigWhitespaceFields() { "s3-region = us-west-1 \t ", "kbase-auth-url = https://kbase.us/authyauth", "kbase-auth-admin-roles = \t ", + "kbase-auth-token-cookies = \t ", "dont-trust-x-ip-headers = \t ", ) cfg, err := New(filePath) @@ -132,6 +134,7 @@ func (t *TestSuite) TestMinimalConfigWhitespaceFields() { S3DisableSSLVerify: false, AuthURL: u, AuthAdminRoles: &[]string{}, + AuthTokenCookies: &[]string{}, DontTrustXIPHeaders: false, } t.Equal(&expected, cfg, "incorrect config") @@ -153,6 +156,7 @@ func (t *TestSuite) TestMaximalConfig() { "s3-disable-ssl-verify= true ", "kbase-auth-url = https://kbase.us/authyauth", "kbase-auth-admin-roles = \t , foo , \tbar\t , , baz ,,", + "kbase-auth-token-cookies = \t , chokkiechip , \toreoinmilk\t , , earwaxNsnot ,,", "dont-trust-x-ip-headers = true \t ", ) cfg, err := New(filePath) @@ -173,6 +177,7 @@ func (t *TestSuite) TestMaximalConfig() { S3Region: "us-west-1", AuthURL: u, AuthAdminRoles: &[]string{"foo", "bar", "baz"}, + AuthTokenCookies: &[]string{"chokkiechip", "oreoinmilk", "earwaxNsnot"}, DontTrustXIPHeaders: true, } t.Equal(&expected, cfg, "incorrect config") diff --git a/deploy.cfg.example b/deploy.cfg.example index 04af651..256acf6 100644 --- a/deploy.cfg.example +++ b/deploy.cfg.example @@ -30,6 +30,9 @@ s3-region = us-west-1 kbase-auth-url = https://kbase.us/services/auth # KBase auth server custom roles that denote the user is a blobstore admin. Comma delimited. kbase-auth-admin-roles = KBASE_ADMIN, BLOBSTORE_ADMIN +# A list of comma separated cookie names to check for authentication tokens. +# The authentication header is checked first, then each cookie in the list in order. +# kbase-auth-token-cookies = # If "true", make the server ignore the X-Forwarded-For and X-Real-IP headers. Otherwise # (the default behavior), the logged IP address for a request, in order of precedence, is diff --git a/deployment/conf/deployment.cfg.templ b/deployment/conf/deployment.cfg.templ index bfccc16..5f0b3e2 100644 --- a/deployment/conf/deployment.cfg.templ +++ b/deployment/conf/deployment.cfg.templ @@ -28,6 +28,9 @@ s3-disable-ssl-verify = {{ default .Env.s3_disable_ssl_verify "false" }} kbase-auth-url = {{ default .Env.kbase_auth_url "https://ci.kbase.us/services/auth" }} # KBase auth server custom roles that denote the user is a blobstore admin. Comma delimited. kbase-auth-admin-roles = {{ default .Env.kbase_auth_admin_roles "KBASE_ADMIN, BLOBSTORE_ADMIN" }} +# A list of comma separated cookie names to check for authentication tokens. +# The authentication header is checked first, then each cookie in the list in order. +kbase-auth-token-cookies = {{ default .Env.kbase_auth_token_cookies "kbase_session, kbase_session_backup" }} # If "true", make the server ignore the X-Forwarded-For and X-Real-IP headers. Otherwise # (the default behavior), the logged IP address for a request, in order of precedence, is diff --git a/docker-compose.yml b/docker-compose.yml index 1487361..dd1605c 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -5,13 +5,13 @@ version: "3.1" # that is started up and polled services: kbase_blobstore: - image: kbase/blobstore:latest + build: . ports: - "8080:8080" environment: blobstore_host: 0.0.0.0:8080 - mongo_host: localhost:27017 - mongo_database: dc_blobstore_test + mongodb_host: mongo:27017 + mongodb_database: dc_blobstore_test kbase_auth_url: https://ci.kbase.us/services/auth kbase_auth_admin_roles: KBASE_ADMIN,BLOBSTORE_ADMIN s3_host: minio:9000 @@ -24,9 +24,7 @@ services: command: - "-multiline" - "-wait" - - "tcp://ci-mongo:27017" - - "-wait" - - "tcp://mongoinit:8080" + - "tcp://mongo:27017" - "-wait" - "tcp://minio:9000" - "-timeout" @@ -36,37 +34,10 @@ services: - "/kb/deployment/blobstore/blobstore" - "--conf" - "/kb/deployment/conf/deployment.cfg" - # If you needed to pass in context for template evaluation you would put something like - # these lines that tell dockerize to hit github for an INI style file for the context - # - "-env" - # - "https://raw.githubusercontent.com/kbase/mini_kb/master/deployment/conf/tauth2-minikb.yml" - # If the -env URL needs authentication you would use an -env-header directive that specified - # either the hard coded string for the header, or a path to a file that contains the header - # string ( used for working with docker secrets files) - # - "-env-header" - # - "AUTHORIZATION:authtokenvalue" - # or for a path to a secrets file: - # - "env-header" - # - "/run/secrets/authheader" - # If your server is using self-signed certs, or otherwise problematic for cert validation - # you can add the following flag: - # - "-validateCert=false" - depends_on: ["ci-mongo", "mongoinit", "minio"] - - mongoinit: - image: kbase/db_initialize:latest - entrypoint: - - "/kb/deployment/bin/dockerize.sh" - - "-wait" - - "tcp://ci-mongo:27017" - - "-timeout" - - "120s" - depends_on: [ "ci-mongo" ] + depends_on: ["mongo", "minio"] - ci-mongo: - image: mongo:2 - command: - - "--smallfiles" + mongo: + image: mongo:3.6 ports: - "27017:27017" diff --git a/service/errortypes.go b/service/errortypes.go index 8c7ebe4..2ef6951 100644 --- a/service/errortypes.go +++ b/service/errortypes.go @@ -18,7 +18,7 @@ const ( // special explanation in the error string. type UnauthorizedCustomError string -// UnauthorizedCustomError creates a new UnauthorizedCustomError. +// NewUnauthorizedCustomError creates a new UnauthorizedCustomError. func NewUnauthorizedCustomError(err string) *UnauthorizedCustomError { e := UnauthorizedCustomError(err) return &e @@ -28,6 +28,20 @@ func (e *UnauthorizedCustomError) Error() string { return string(*e) } +// InvalidTokenCustomError denotes that an invalid token was submitted and special explanation +// is needed in the error string. +type InvalidTokenCustomError string + +// NewInvalidTokenCustomError creates a new InvalidTokenCustomError. +func NewInvalidTokenCustomError(err string) *InvalidTokenCustomError { + e := InvalidTokenCustomError(err) + return &e +} + +func (e *InvalidTokenCustomError) Error() string { + return string(*e) +} + func translateError(err error) (code int, errstr string) { // not sure about this approach. Alternative is to add some state to every error that // can be mapped to a code, and I'm not super thrilled about that either. @@ -35,6 +49,8 @@ func translateError(err error) (code int, errstr string) { case *auth.InvalidTokenError: // Shock compatibility, should be 401 return http.StatusBadRequest, invalidAuthHeader + case *InvalidTokenCustomError: + return http.StatusUnauthorized, t.Error() case *core.NoBlobError: return http.StatusNotFound, "Node not found" case *core.UnauthorizedError: @@ -45,7 +61,7 @@ func translateError(err error) (code int, errstr string) { return http.StatusBadRequest, "Users that are not node owners can only delete " + "themselves from ACLs." case *UnauthorizedCustomError: - return http.StatusUnauthorized, t.Error() + return http.StatusForbidden, t.Error() case *auth.InvalidUserError: // no equivalent shock error, it accepts any string as a username return http.StatusBadRequest, t.Error() diff --git a/service/integration_test.go b/service/integration_test.go index 4877f94..3b9cdca 100644 --- a/service/integration_test.go +++ b/service/integration_test.go @@ -80,20 +80,20 @@ func (t *TestSuite) SetupSuite() { logrus.SetOutput(ioutil.Discard) t.loggerhook = logrust.NewGlobal() - roles := []string{adminRole, blobstoreRole} serv, err := New( &config.Config{ - Host: "foo", // not used - MongoHost: "localhost:" + strconv.Itoa(t.mongo.GetPort()), - MongoDatabase: testDB, - S3Host: "localhost:" + strconv.Itoa(t.minio.GetPort()), - S3Bucket: testBucket, - S3AccessKey: "ackey", - S3AccessSecret: "sooporsecret", - S3Region: "us-west-1", - S3DisableSSL: true, - AuthURL: &authurl, - AuthAdminRoles: &roles, + Host: "foo", // not used + MongoHost: "localhost:" + strconv.Itoa(t.mongo.GetPort()), + MongoDatabase: testDB, + S3Host: "localhost:" + strconv.Itoa(t.minio.GetPort()), + S3Bucket: testBucket, + S3AccessKey: "ackey", + S3AccessSecret: "sooporsecret", + S3Region: "us-west-1", + S3DisableSSL: true, + AuthURL: &authurl, + AuthAdminRoles: &[]string{adminRole, blobstoreRole}, + AuthTokenCookies: &[]string{"cookie1", "cookie2", "cookie3"}, }, ServerStaticConf{ ServerName: "servn", @@ -443,6 +443,104 @@ func (t *TestSuite) checkXIPLogs(xFF, xRIP, ip string) { t.loggerhook.Reset() } +// Since the middleware applies to every method, testing it exhaustively is unreasonable. +// Here we test it using a node get for simpliciaty. +// These tests are expected to exercise the middleware, not the specific data method. + +func (t *TestSuite) TestAuthenticationMiddleWare() { + fake := "fake" + mt := "" + ws := " " + t.testAuthenticationMiddleWare( + t.noRole.user, &t.noRole.token, &fake, &fake, &fake, + t.noRole2.user, nil, &t.noRole2.token, &fake, &fake, + ) + t.testAuthenticationMiddleWare( + t.noRole.user, nil, &t.noRole.token, &fake, &fake, + t.noRole2.user, &t.noRole2.token, &fake, &fake, &fake, + ) + t.testAuthenticationMiddleWare( + t.noRole.user, &mt, nil, &t.noRole.token, &fake, + t.noRole2.user, nil, nil, nil, &t.noRole2.token, + ) + t.testAuthenticationMiddleWare( + t.noRole.user, &mt, &ws, nil, &t.noRole.token, + t.noRole2.user, &mt, &ws, &t.noRole2.token, &fake, + ) + t.testAuthenticationMiddleWare( + t.noRole.user, &mt, &mt, &ws, &t.noRole.token, + t.noRole2.user, &mt, &mt, &t.noRole2.token, &fake, + ) +} + +func (t *TestSuite) testAuthenticationMiddleWare( + pstUser string, pstHeader *string, pstCookie1 *string, pstCookie2 *string, pstCookie3 *string, + getUser string, getHeader *string, getCookie1 *string, getCookie2 *string, getCookie3 *string, +) { + req1, err := http.NewRequest("POST", t.url+"/node", strings.NewReader("d")) + t.Nil(err, "unexpected error") + addAuthHeader(req1, pstHeader) + addCookie(req1, "cookie1", pstCookie1) + addCookie(req1, "cookie2", pstCookie2) + addCookie(req1, "cookie3", pstCookie3) + body := t.requestToJSON(req1, 374, 200) + + t.checkLogs(logEvent{logrus.InfoLevel, "POST", "/node", 200, &pstUser, + "request complete", mtmap(), false}, + ) + data := body["data"].(map[string]interface{}) + id := data["id"].(string) + t.Equal(body["status"], float64(200), "incorrect status") + + // add read for a different user + body = t.req("PUT", t.url+"/node/"+id+"/acl/read?users="+t.noRole2.user, nil, + "Oauth "+t.noRole.token, 441, 200) + t.loggerhook.Reset() + + // now get the node with a different user + req2, err := http.NewRequest("GET", t.url+"/node/" + id, nil) + t.Nil(err, "unexpected error") + addAuthHeader(req2, getHeader) + addCookie(req2, "cookie1", getCookie1) + addCookie(req2, "cookie2", getCookie2) + addCookie(req2, "cookie3", getCookie3) + body = t.requestToJSON(req2, 374, 200) + + t.checkLogs(logEvent{logrus.InfoLevel, "GET", "/node/" + id, 200, &getUser, + "request complete", mtmap(), false}, + ) + t.Equal(body["status"], float64(200), "incorrect status") +} + +func addCookie(req *http.Request, name string, val *string) { + if val != nil { + req.AddCookie(&http.Cookie{Name: name, Value: *val}) + } +} + +func addAuthHeader(req *http.Request, token *string) { + if token != nil { + if *token == "" { + req.Header.Set("authorization", "") + } else { + req.Header.Set("authorization", "Oauth "+*token) + } + } +} + +func (t *TestSuite) TestAuthenticationMiddleWareFailBadCookie() { + req1, err := http.NewRequest("POST", t.url+"/node", strings.NewReader("d")) + t.Nil(err, "unexpected error") + c := "fake" + addCookie(req1, "cookie2", &c) + addCookie(req1, "cookie3", &c) + body := t.requestToJSON(req1, 125, 401) + t.checkError(body, 401, "KBase auth server reported token was invalid from cookie cookie2") + t.checkLogs(logEvent{logrus.ErrorLevel, "POST", "/node", 401, nil, + "KBase auth server reported token was invalid from cookie cookie2", mtmap(), false}, + ) +} + func (t *TestSuite) TestStoreAndGetWithFilename() { // check whitespace body := t.req("POST", t.url+"/node?filename=%20%20myfile%20%20", @@ -1148,18 +1246,18 @@ func (t *TestSuite) TestGetFileFailDelete() { t.req("PUT", t.url+"/node/"+id+"/acl/read?users="+t.noRole2.user, nil, "Oauth "+t.noRole.token, 441, 200) t.loggerhook.Reset() - body5 := t.get(t.url+"/node/"+id+"?download&del", &t.noRole2, 94, 401) - t.checkError(body5, 401, "Only node owners can delete nodes") - t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, &t.noRole2.user, + body5 := t.get(t.url+"/node/"+id+"?download&del", &t.noRole2, 94, 403) + t.checkError(body5, 403, "Only node owners can delete nodes") + t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 403, &t.noRole2.user, "Only node owners can delete nodes", mtmap(), false}, ) // make node public t.req("PUT", t.url+"/node/"+id+"/acl/public_read", nil, "Oauth "+t.noRole.token, 440, 200) t.loggerhook.Reset() - body6 := t.get(t.url+"/node/"+id+"?download&del", nil, 94, 401) - t.checkError(body6, 401, "Only node owners can delete nodes") - t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, nil, + body6 := t.get(t.url+"/node/"+id+"?download&del", nil, 94, 403) + t.checkError(body6, 403, "Only node owners can delete nodes") + t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 403, nil, "Only node owners can delete nodes", mtmap(), false}, ) } diff --git a/service/server.go b/service/server.go index b8f33ef..6909337 100644 --- a/service/server.go +++ b/service/server.go @@ -68,6 +68,7 @@ type Server struct { auth *authcache.Cache store *core.BlobStore ignoreXIPheaders bool + authCookieNames *[]string } // New create a new server. @@ -88,6 +89,7 @@ func New(cfg *config.Config, sconf ServerStaticConf) (*Server, error) { auth: deps.AuthCache, store: deps.BlobStore, ignoreXIPheaders: cfg.DontTrustXIPHeaders, + authCookieNames: cfg.AuthTokenCookies, } router.NotFoundHandler = http.HandlerFunc(s.notFoundHandler) router.MethodNotAllowedHandler = http.HandlerFunc(s.notAllowedHandler) @@ -218,11 +220,25 @@ func (s *Server) authLogMiddleWare(next http.Handler) http.Handler { writeErrorWithCode(le, err.Error(), 400, w) return } + cookieName := "" + if token == "" { + for _, cn := range *s.authCookieNames { + cookie, err := r.Cookie(cn) + if err == nil && strings.TrimSpace(cookie.Value) != "" { + token = cookie.Value + cookieName = cn + break + } + } + } var user *auth.User if token != "" { var err error user, err = s.auth.GetUser(le, token) if err != nil { + if cookieName != "" { + err = NewInvalidTokenCustomError(err.Error()+" from cookie "+cookieName) + } writeError(le, err, w) return }