diff --git a/changelog.d/0-release-notes/WPB-28193-nginx-conf-syntax-changed b/changelog.d/0-release-notes/WPB-28193-nginx-conf-syntax-changed new file mode 100644 index 00000000000..c23ec41570f --- /dev/null +++ b/changelog.d/0-release-notes/WPB-28193-nginx-conf-syntax-changed @@ -0,0 +1,18 @@ +**[Only relevant if you have overwritten `oauth_scope` directives in +your nginx config]** The nginz route configuration syntax changed: +`oauth_scope` (a bare scope base, e.g. `conversations_code`) is +deprecated in favor of `oauth_scopes` (whole scopes, +e.g. `["read:conversations_code", "write-only:conversations_code"]`). + +If you override nginz routes in your own values, migrate each +`oauth_scope` entry to the equivalent `oauth_scopes` list; +`oauth_scopes` wins where a route has both. If you use the bundled +chart, there is nothing to do, it is already updated. + +The old directive and old oauth tokens remain valid with new +wire-server, but we will remove the deprecated functionality in some +future release. + +See docs/src/developer/reference/config-options.md, +https://docs.wire.com/latest/developer/reference/oauth.html for more +context. diff --git a/changelog.d/1-api-changes/WPB-28193-disentangle-oauth-scopes-_write-access-should-not-imply-read-access_ b/changelog.d/1-api-changes/WPB-28193-disentangle-oauth-scopes-_write-access-should-not-imply-read-access_ new file mode 100644 index 00000000000..fe8f312730d --- /dev/null +++ b/changelog.d/1-api-changes/WPB-28193-disentangle-oauth-scopes-_write-access-should-not-imply-read-access_ @@ -0,0 +1 @@ +Disentangle oauth scopes (write access should not imply read access). diff --git a/charts/nginz/templates/conf/_nginx.conf.tpl b/charts/nginz/templates/conf/_nginx.conf.tpl index 671acd38869..db4d5a1204d 100644 --- a/charts/nginz/templates/conf/_nginx.conf.tpl +++ b/charts/nginz/templates/conf/_nginx.conf.tpl @@ -297,7 +297,14 @@ http { {{- end }} {{- end }} - {{- if ($location.oauth_scope) }} + {{- if hasKey $location "oauth_scopes" }} + {{- if $location.oauth_scopes }} + oauth_scopes {{ join " " $location.oauth_scopes }}; + {{- else }} + # 'oauth_scopes: []': no OAuth token gets in here. (Without any + # 'oauth_scope[s]' directive libzauth rejects them all.) + {{- end }} + {{- else if ($location.oauth_scope) }} oauth_scope {{ $location.oauth_scope }}; {{- end }} diff --git a/charts/nginz/values.yaml b/charts/nginz/values.yaml index d114b3c7d35..960c6c43d2b 100644 --- a/charts/nginz/values.yaml +++ b/charts/nginz/values.yaml @@ -228,7 +228,8 @@ nginx_conf: envs: - staging - path: /self$ # Matches exactly /self - oauth_scope: self + oauth_scope: self # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["read:self"] envs: - all - path: /self/name @@ -643,7 +644,8 @@ nginx_conf: - path: /conversations/([^/]*)/([^/]*)/name envs: - all - oauth_scope: conversations_name + oauth_scope: conversations_name # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["write-only:conversations_name"] - path: /broadcast envs: - all @@ -660,11 +662,13 @@ nginx_conf: - path: /conversations$ envs: - all - oauth_scope: conversations + oauth_scope: conversations # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["write-only:conversations"] - path: /conversations/([^/]*)/code envs: - all - oauth_scope: conversations_code + oauth_scope: conversations_code # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["read:conversations_code", "write-only:conversations_code"] - path: /conversations/join envs: - all @@ -758,7 +762,8 @@ nginx_conf: - path: /feature-configs(.*) envs: - all - oauth_scope: feature_configs + oauth_scope: feature_configs # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["read:feature_configs"] - path: /mls/welcome envs: - all @@ -785,12 +790,12 @@ nginx_conf: - path: /meetings$ envs: - all - oauth_scope: meetings - ## this rule can't be expressed yet: https://wearezeta.atlassian.net/browse/WPB-28193 - #- path: /meetings/([^/]*)/([^/]*)$ - # envs: - # - all - # oauth_scopes: [write:meetings, admin:meetings] + oauth_scope: meetings # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["write-only:meetings"] + - path: /meetings/([^/]*)/([^/]*)$ + envs: + - all + oauth_scopes: [] # https://wearezeta.atlassian.net/browse/WPB-28194: this will be `["write-only:meetings", "delete-only:meetings"]` soon. - path: /meetings/(.*) envs: - all diff --git a/deploy/dockerephemeral/federation-v0/nginz/conf/nginx.conf b/deploy/dockerephemeral/federation-v0/nginz/conf/nginx.conf index cd4ec97a1a7..2f479cff068 100644 --- a/deploy/dockerephemeral/federation-v0/nginz/conf/nginx.conf +++ b/deploy/dockerephemeral/federation-v0/nginz/conf/nginx.conf @@ -232,7 +232,7 @@ http { location ~* ^(/v[0-9]+)?/self$ { include common_response_with_zauth.conf; - oauth_scope self; + oauth_scopes read:self; proxy_pass http://brig; } @@ -357,13 +357,13 @@ http { location ~* ^(/v[0-9]+)?/conversations$ { include common_response_with_zauth.conf; - oauth_scope conversations; + oauth_scopes write-only:conversations; proxy_pass http://galley; } location ~* ^(/v[0-9]+)?/conversations/([^/]*)/code { include common_response_with_zauth.conf; - oauth_scope conversations_code; + oauth_scopes read:conversations_code write-only:conversations_code; proxy_pass http://galley; } @@ -429,7 +429,7 @@ http { location ~* ^(/v[0-9]+)?/feature-configs$ { include common_response_with_zauth.conf; - oauth_scope feature_configs; + oauth_scopes read:feature_configs; proxy_pass http://galley; } diff --git a/deploy/dockerephemeral/federation-v1/nginz/conf/nginx.conf b/deploy/dockerephemeral/federation-v1/nginz/conf/nginx.conf index 43f8c68b306..8346bf1f36e 100644 --- a/deploy/dockerephemeral/federation-v1/nginz/conf/nginx.conf +++ b/deploy/dockerephemeral/federation-v1/nginz/conf/nginx.conf @@ -232,7 +232,7 @@ http { location ~* ^(/v[0-9]+)?/self$ { include common_response_with_zauth.conf; - oauth_scope self; + oauth_scopes read:self; proxy_pass http://brig; } @@ -357,13 +357,13 @@ http { location ~* ^(/v[0-9]+)?/conversations$ { include common_response_with_zauth.conf; - oauth_scope conversations; + oauth_scopes write-only:conversations; proxy_pass http://galley; } location ~* ^(/v[0-9]+)?/conversations/([^/]*)/code { include common_response_with_zauth.conf; - oauth_scope conversations_code; + oauth_scopes read:conversations_code write-only:conversations_code; proxy_pass http://galley; } @@ -429,7 +429,7 @@ http { location ~* ^(/v[0-9]+)?/feature-configs$ { include common_response_with_zauth.conf; - oauth_scope feature_configs; + oauth_scopes read:feature_configs; proxy_pass http://galley; } diff --git a/deploy/dockerephemeral/federation-v2/nginz/conf/nginx.conf b/deploy/dockerephemeral/federation-v2/nginz/conf/nginx.conf index bef49f1d046..7d1319ec7ec 100644 --- a/deploy/dockerephemeral/federation-v2/nginz/conf/nginx.conf +++ b/deploy/dockerephemeral/federation-v2/nginz/conf/nginx.conf @@ -212,7 +212,7 @@ http { location ~* ^(/v[0-9]+)?/self$ { include common_response_with_zauth.conf; - oauth_scope self; + oauth_scopes read:self; proxy_pass http://brig; } @@ -337,13 +337,13 @@ http { location ~* ^(/v[0-9]+)?/conversations$ { include common_response_with_zauth.conf; - oauth_scope conversations; + oauth_scopes write-only:conversations; proxy_pass http://galley; } location ~* ^(/v[0-9]+)?/conversations/([^/]*)/code { include common_response_with_zauth.conf; - oauth_scope conversations_code; + oauth_scopes read:conversations_code write-only:conversations_code; proxy_pass http://galley; } @@ -409,7 +409,7 @@ http { location ~* ^(/v[0-9]+)?/feature-configs$ { include common_response_with_zauth.conf; - oauth_scope feature_configs; + oauth_scopes read:feature_configs; proxy_pass http://galley; } diff --git a/docs/src/developer/reference/config-options.md b/docs/src/developer/reference/config-options.md index 9154c97ea73..2a08c9711c7 100644 --- a/docs/src/developer/reference/config-options.md +++ b/docs/src/developer/reference/config-options.md @@ -1102,6 +1102,63 @@ optSettings: setOAuthMaxActiveRefreshTokens: 10 ``` +#### Scopes + +Which scope an OAuth token needs is configured per route in `nginz`'s Helm +chart, and enforced by *nginz*: + +```yaml +# [nginz/values.yaml] +nginx_conf: + upstreams: + galley: + - path: /conversations/([^/]*)/code + envs: + - all + oauth_scopes: ["read:conversations_code", "write-only:conversations_code"] +``` + +A scope has a tier and a base, e.g. the tier `read` and the base +`conversations_code`. The method of the request decides which tier it needs: + +| method | tier | +| --------------- | ------------- | +| `GET` | `read` | +| `POST`, `PUT` | `write-only` | +| `DELETE` | `delete-only` | + +Only the listed scopes of that tier let a token in, and the tiers are +independent of each other: `write-only:` does not include `read:`. So in the +example above, a `GET` needs `read:conversations_code`, and a `DELETE` gets +nowhere, because the list has no `delete-only:conversations_code`. + +A route with no `oauth_scopes` (or with an empty list) (and with no +`oauth_scope`, see next section) accepts no OAuth token at all. It +may still be reachable with a cookie or a zauth token; scopes are +about OAuth only. + +Every scope should also be named in the swagger docs, which is a separate +annotation in the routing tables. A unit test in `wire-api` compares the two +and fails if they disagree. + +##### Deprecated: `oauth_scope` + +Older configurations name only the base and leave the tier to *nginz*: + +```yaml +# [nginz/values.yaml] + oauth_scope: conversations_code +``` + +Here the tiers build on one another: `write:` includes `read:`, and `admin:` +includes `write:`. A `GET` therefore passes with `read:`, `write:`, or +`admin:conversations_code`, and a `DELETE` needs `admin:conversations_code`. + +`oauth_scopes` replaces this, and wins over it where a route has both. Tokens +work across the change in either direction: a token with old scopes gets into a +route that has moved to `oauth_scopes`, and a token with new scopes gets into a +route that has not. + #### Password hashing options Since release 5.6.0, wire-server can hash passwords with diff --git a/integration/test/API/Nginz.hs b/integration/test/API/Nginz.hs index 3649bf1d8c0..0232cd00ed4 100644 --- a/integration/test/API/Nginz.hs +++ b/integration/test/API/Nginz.hs @@ -31,14 +31,14 @@ getSystemSettingsUnAuthorized domain = do login :: (HasCallStack, MakesValue domain, MakesValue email, MakesValue password) => domain -> email -> password -> App Response login domain email pw = do - req <- rawBaseRequest domain Nginz Unversioned "/login" + req <- rawBaseNginzRequest domain Unversioned "/login" emailStr <- make email >>= asString pwStr <- make pw >>= asString submit "POST" (req & addJSONObject ["email" .= emailStr, "password" .= pwStr, "label" .= "auth"]) loginWith2ndFactor :: (HasCallStack, MakesValue domain, MakesValue email, MakesValue password, MakesValue sndFactor) => domain -> email -> password -> sndFactor -> App Response loginWith2ndFactor domain email pw sf = do - req <- rawBaseRequest domain Nginz Unversioned "/login" + req <- rawBaseNginzRequest domain Unversioned "/login" emailStr <- make email >>= asString pwStr <- make pw >>= asString sfStr <- make sf >>= asString @@ -46,13 +46,13 @@ loginWith2ndFactor domain email pw sf = do access :: (HasCallStack, MakesValue domain, MakesValue cookie) => domain -> cookie -> App Response access domain cookie = do - req <- rawBaseRequest domain Nginz Unversioned "/access" + req <- rawBaseNginzRequest domain Unversioned "/access" cookieStr <- make cookie >>= asString submit "POST" (req & setCookie cookieStr) logout :: (HasCallStack, MakesValue domain, MakesValue cookie, MakesValue token) => domain -> cookie -> token -> App Response logout d c t = do - req <- rawBaseRequest d Nginz Unversioned "/access/logout" + req <- rawBaseNginzRequest d Unversioned "/access/logout" cookie <- make c & asString token <- make t & asString submit "POST" (req & setCookie cookie & addHeader "Authorization" ("Bearer " <> token)) @@ -61,12 +61,49 @@ getConversation :: (HasCallStack, MakesValue user, MakesValue qcnv, MakesValue t getConversation user qcnv t = do (domain, cnv) <- objQid qcnv token <- make t & asString - req <- rawBaseRequest user Nginz Versioned (joinHttpPath ["conversations", domain, cnv]) + req <- rawBaseNginzRequest user Versioned (joinHttpPath ["conversations", domain, cnv]) submit "GET" (req & addHeader "Authorization" ("Bearer " <> token)) +-- | The endpoints below take an OAuth access token, which is what nginz checks +-- scopes against. A request with a zauth cookie or token is not affected by +-- any of that; see 'Test.OAuth'. +getSelf :: (HasCallStack, MakesValue user, MakesValue token) => user -> token -> App Response +getSelf user t = do + token <- make t & asString + req <- rawBaseNginzRequest user Versioned "/self" + submit "GET" (req & addHeader "Authorization" ("Bearer " <> token)) + +postConversation :: (HasCallStack, MakesValue user, MakesValue conv, MakesValue token) => user -> conv -> token -> App Response +postConversation user conv t = do + token <- make t & asString + body <- make conv + req <- rawBaseNginzRequest user Versioned "/conversations" + submit "POST" (req & addJSON body & addHeader "Authorization" ("Bearer " <> token)) + +getConversationCode :: (HasCallStack, MakesValue user, MakesValue conv, MakesValue token) => user -> conv -> token -> App Response +getConversationCode user conv t = do + convId <- objQidObject conv & objId + token <- make t & asString + req <- rawBaseNginzRequest user Versioned (joinHttpPath ["conversations", convId, "code"]) + submit "GET" (req & addHeader "Authorization" ("Bearer " <> token)) + +postConversationCode :: (HasCallStack, MakesValue user, MakesValue conv, MakesValue token) => user -> conv -> token -> App Response +postConversationCode user conv t = do + convId <- objQidObject conv & objId + token <- make t & asString + req <- rawBaseNginzRequest user Versioned (joinHttpPath ["conversations", convId, "code"]) + submit "POST" (req & addJSONObject [] & addHeader "Authorization" ("Bearer " <> token)) + +deleteConversationCode :: (HasCallStack, MakesValue user, MakesValue conv, MakesValue token) => user -> conv -> token -> App Response +deleteConversationCode user conv t = do + convId <- objQidObject conv & objId + token <- make t & asString + req <- rawBaseNginzRequest user Versioned (joinHttpPath ["conversations", convId, "code"]) + submit "DELETE" (req & addHeader "Authorization" ("Bearer " <> token)) + uploadProviderAsset :: (HasCallStack, MakesValue domain) => domain -> String -> String -> App Response uploadProviderAsset domain cookie payload = do - req <- rawBaseRequest domain Nginz Versioned $ joinHttpPath ["provider", "assets"] + req <- rawBaseNginzRequest domain Versioned $ joinHttpPath ["provider", "assets"] bdy <- txtAsset payload submit "POST" $ req diff --git a/integration/test/Test/OAuth.hs b/integration/test/Test/OAuth.hs index 5cd899885e0..e5cae68aeb8 100644 --- a/integration/test/Test/OAuth.hs +++ b/integration/test/Test/OAuth.hs @@ -20,7 +20,15 @@ module Test.OAuth where import API.Brig import API.BrigInternal import API.Common (defPassword) +import API.Galley +import qualified API.Nginz as Nginz +import qualified Data.Aeson as Aeson +import qualified Data.ByteString.Base64.URL as B64U import Data.String.Conversions +import qualified Data.Text as T +import Data.UUID (UUID) +import qualified Data.UUID as UUID +import Database.CQL.IO import Network.HTTP.Types import Network.URI import SetupHelpers @@ -31,7 +39,7 @@ testOAuthRevokeSession = do user <- randomUser OwnDomain def let uri = "https://example.com" cid <- createOAuthClient user "foobar" uri >>= getJSON 200 >>= flip (%.) "client_id" - let scopes = ["write:conversations"] + let scopes = ["write-only:conversations"] -- create a session that will be revoked later (tokenToBeRevoked, sessionToBeRevoked) <- do @@ -87,7 +95,7 @@ testRevokeApplicationAccountAccessV6 = do apps <- resp.json & asList length apps `shouldMatchInt` 0 let uri = "https://example.com" - let scopes = ["write:conversations"] + let scopes = ["write-only:conversations"] replicateM_ 3 $ do cid <- createOAuthClient user "foobar" uri >>= getJSON 200 >>= flip (%.) "client_id" generateAccessToken user cid scopes uri @@ -120,7 +128,7 @@ testRevokeApplicationAccountAccess = do apps <- resp.json & asList length apps `shouldMatchInt` 0 let uri = "https://example.com" - let scopes = ["write:conversations"] + let scopes = ["write-only:conversations"] replicateM_ 3 $ do cid <- createOAuthClient user "foobar" uri >>= getJSON 200 >>= flip (%.) "client_id" generateAccessToken user cid scopes uri @@ -146,6 +154,135 @@ testRevokeApplicationAccountAccess = do apps <- resp.json & asList length apps `shouldMatchInt` 0 +-- | The tiers of a scope are separate: a token that may write may not read, +-- and the other way round. This is about @/conversations/:cnv/code@, the one +-- location in the integration nginx.conf that uses 'oauth_scopes'. +testOAuthScopeTiersAreSeparate :: (HasCallStack) => App () +testOAuthScopeTiersAreSeparate = do + (user, _, _) <- createTeam OwnDomain 1 + conv <- postConversation user (allowGuests defProteus) >>= getJSON 201 + -- with a zauth token, so that there is a code to read later on + postConversationCode user conv Nothing Nothing >>= assertSuccess + + cid <- oauthClient user + readToken <- accessTokenFor user cid ["read:conversations_code"] + writeToken <- accessTokenFor user cid ["write-only:conversations_code"] + + Nginz.getConversationCode user conv readToken >>= assertStatus 200 + Nginz.postConversationCode user conv readToken >>= assertStatus 403 + + Nginz.postConversationCode user conv writeToken >>= assertSuccess + Nginz.getConversationCode user conv writeToken >>= assertStatus 403 + +testOAuthRejectUnusefulTokenRequests :: (HasCallStack) => App () +testOAuthRejectUnusefulTokenRequests = do + (user, _, _) <- createTeam OwnDomain 1 + cid <- oauthClient user + + generateOAuthAuthorizationCode user cid [] redirectUri >>= assertStatus 400 + -- the 400 has to say which scopes would have worked + bindResponse (generateOAuthAuthorizationCode user cid ["pizza"] redirectUri) $ \resp -> do + resp.status `shouldMatchInt` 400 + msg <- resp.json %. "message" & asString + msg `shouldContainString` "read:self" + msg `shouldContainString` "write-only:conversations" + generateOAuthAuthorizationCode user cid ["delete-only:conversations_code"] redirectUri >>= assertStatus 400 + +testOAuthNewScopesOnDeprecatedAttribute :: (HasCallStack) => App () +testOAuthNewScopesOnDeprecatedAttribute = do + user <- randomUser OwnDomain def + cid <- oauthClient user + + selfToken <- accessTokenFor user cid ["read:self"] + Nginz.getSelf user selfToken >>= assertStatus 200 + + convToken <- accessTokenFor user cid ["write-only:conversations"] + Nginz.postConversation user defProteus convToken >>= assertStatus 201 + + -- ... and the deprecated attribute still tells the scopes apart + Nginz.getSelf user convToken >>= assertStatus 403 + +testOAuthDeprecatedScopesInCassandra :: (HasCallStack) => TaggedBool "old scope syntax" -> App () +testOAuthDeprecatedScopesInCassandra (TaggedBool oldScopeSyntax) = do + (user, _, _) <- createTeam OwnDomain 1 + conv <- postConversation user (allowGuests defProteus) >>= getJSON 201 + postConversationCode user conv Nothing Nothing >>= assertSuccess + + cid <- oauthClient user + session <- generateAccessToken user cid ["write-only:conversations_code"] redirectUri + + when oldScopeSyntax (hackCassandra user) + + refreshToken <- session %. "refresh_token" & asString + refreshed <- createOAuthAccessTokenWithRefreshToken user cid refreshToken >>= getJSON 200 + + if oldScopeSyntax + then do + token <- refreshed %. "access_token" & asString + hasScopes token ["read:conversations_code", "write-only:conversations_code"] + Nginz.getConversationCode user conv token >>= assertSuccess + Nginz.postConversationCode user conv token >>= assertSuccess + else do + token <- refreshed %. "access_token" & asString + hasScopes token ["write-only:conversations_code"] + Nginz.getConversationCode user conv token >>= assertStatus 403 + Nginz.postConversationCode user conv token >>= assertSuccess + where + -- pretend the session was created before the split + hackCassandra :: Value -> App () + hackCassandra user = do + keyspace <- readServiceConfig Brig & (%. "cassandra.keyspace") & asString + let setScope :: PrepQuery W (Identity UUID) () = + fromString + $ "UPDATE " + <> keyspace + <> ".oauth_refresh_token SET scope = {'write:conversations_code', 'read:pizza'} WHERE id = ?" + rid <- refreshTokenId user + write setScope (defQueryParams LocalQuorum (Identity rid)) + + hasScopes :: String -> [String] -> App () + hasScopes token expectedScopes = do + claims <- accessTokenClaims token + scopes <- claims %. "scope" & asString + words scopes `shouldMatchSet` expectedScopes + +-------------------------------------------------------------------------------- +-- helpers + +redirectUri :: String +redirectUri = "https://example.com" + +oauthClient :: (HasCallStack, MakesValue user) => user -> App Value +oauthClient user = + createOAuthClient user "foobar" redirectUri >>= getJSON 200 >>= (%. "client_id") + +-- | The access token, which is the part nginz gets to see. +accessTokenFor :: (HasCallStack, MakesValue user, MakesValue cid) => user -> cid -> [String] -> App String +accessTokenFor user cid scopes = + generateAccessToken user cid scopes redirectUri >>= (%. "access_token") >>= asString + +-- | The id of the one session the user has. +refreshTokenId :: (HasCallStack, MakesValue user) => user -> App UUID +refreshTokenId user = do + [app] <- getOAuthApplications user >>= getJSON 200 >>= asList + [session] <- app %. "sessions" >>= asList + rid <- session %. "refresh_token_id" & asString + maybe (assertFailure ("not a uuid: " <> rid)) pure (UUID.fromString rid) + +-- | The claims of an access token, read without verifying anything: we only +-- want to see what brig put in. +accessTokenClaims :: (HasCallStack) => String -> App Value +accessTokenClaims token = do + payload <- case T.splitOn (cs ".") (cs token) of + (_ : p : _) -> pure p + _ -> assertFailure ("not a JWT: " <> token) + claims <- case B64U.decodeUnpadded (cs payload) of + Left e -> assertFailure ("not base64url: " <> token <> ": " <> e) + Right bs -> pure bs + case Aeson.eitherDecode (cs claims) of + Left e -> assertFailure ("not json: " <> token <> ": " <> e) + Right v -> pure v + generateAccessToken :: (MakesValue cid, MakesValue user) => user -> cid -> [String] -> String -> App Value generateAccessToken user cid scopes uri = do authCodeResponse <- generateOAuthAuthorizationCode user cid scopes uri diff --git a/integration/test/Testlib/HTTP.hs b/integration/test/Testlib/HTTP.hs index ad48ed444d4..07459588869 100644 --- a/integration/test/Testlib/HTTP.hs +++ b/integration/test/Testlib/HTTP.hs @@ -189,6 +189,14 @@ rawBaseRequest domain service versioned path = do let HostPort h p = serviceHostPort serviceMap service in "http://" <> h <> ":" <> show p <> ("/" <> joinHttpPath (pathSegsPrefix <> splitHttpPath path)) +-- | This is a thin wrapper around rawBaseRequest that adds a non-IP +-- Z-Host header. ('ZHostOpt' parses the HTTP header as a 'Domain', +-- and that doesn't parse if nginz is contacted under its IP address.) +rawBaseNginzRequest :: (HasCallStack, MakesValue domain) => domain -> Versioned -> String -> App HTTP.Request +rawBaseNginzRequest user versioned path = do + domain <- objDomain user + addHeader "Host" domain <$> rawBaseRequest user Nginz versioned path + -- | The bare minimum to ge a `HTTP.Request` given a URL externalRequest :: String -> App HTTP.Request externalRequest = liftIO . HTTP.parseRequest diff --git a/libs/cassandra-util/src/Cassandra.hs b/libs/cassandra-util/src/Cassandra.hs index 24406334813..1713ca9767e 100644 --- a/libs/cassandra-util/src/Cassandra.hs +++ b/libs/cassandra-util/src/Cassandra.hs @@ -26,7 +26,7 @@ import Cassandra.CQL as C ( Ascii (Ascii), BatchType (BatchLogged, BatchUnLogged), Blob (Blob), - ColumnType (AsciiColumn, BigIntColumn, BlobColumn, BooleanColumn, DoubleColumn, IntColumn, ListColumn, MaybeColumn, TextColumn, TimestampColumn, UdtColumn, UuidColumn, VarCharColumn), + ColumnType (AsciiColumn, BigIntColumn, BlobColumn, BooleanColumn, DoubleColumn, IntColumn, ListColumn, MaybeColumn, SetColumn, TextColumn, TimestampColumn, UdtColumn, UuidColumn, VarCharColumn), Consistency (All, LocalQuorum, One), -- DO NOT EXPORT 'Quorum' here (until a DC migration is complete) Cql, Keyspace (Keyspace), @@ -39,7 +39,7 @@ import Cassandra.CQL as C Tagged (Tagged), TimeUuid (TimeUuid), Tuple (), - Value (CqlAscii, CqlBigInt, CqlBlob, CqlBoolean, CqlDouble, CqlInt, CqlList, CqlText, CqlUdt), + Value (CqlAscii, CqlBigInt, CqlBlob, CqlBoolean, CqlDouble, CqlInt, CqlList, CqlSet, CqlText, CqlUdt), Version (V4), W, ctype, diff --git a/libs/cassandra-util/src/Cassandra/CQL.hs b/libs/cassandra-util/src/Cassandra/CQL.hs index d1e1e7c8c73..e66333a4ff0 100644 --- a/libs/cassandra-util/src/Cassandra/CQL.hs +++ b/libs/cassandra-util/src/Cassandra/CQL.hs @@ -25,7 +25,7 @@ import Database.CQL.Protocol as C ( Ascii (Ascii), BatchType (BatchLogged, BatchUnLogged), Blob (Blob), - ColumnType (AsciiColumn, BigIntColumn, BlobColumn, BooleanColumn, DoubleColumn, IntColumn, ListColumn, MaybeColumn, TextColumn, TimestampColumn, UdtColumn, UuidColumn, VarCharColumn), + ColumnType (AsciiColumn, BigIntColumn, BlobColumn, BooleanColumn, DoubleColumn, IntColumn, ListColumn, MaybeColumn, SetColumn, TextColumn, TimestampColumn, UdtColumn, UuidColumn, VarCharColumn), Consistency (All, LocalQuorum, One), -- DO NOT EXPORT 'Quorum' here (until a DC migration is complete) Cql, Keyspace (Keyspace), @@ -38,7 +38,7 @@ import Database.CQL.Protocol as C Tagged (Tagged), TimeUuid (TimeUuid), Tuple (), - Value (CqlAscii, CqlBigInt, CqlBlob, CqlBoolean, CqlDouble, CqlInt, CqlList, CqlText, CqlTimestamp, CqlUdt), + Value (CqlAscii, CqlBigInt, CqlBlob, CqlBoolean, CqlDouble, CqlInt, CqlList, CqlSet, CqlText, CqlTimestamp, CqlUdt), Version (V4), W, ctype, diff --git a/libs/libzauth/libzauth-c/src/lib.rs b/libs/libzauth/libzauth-c/src/lib.rs index 91eb28062f2..23bb6367294 100644 --- a/libs/libzauth/libzauth-c/src/lib.rs +++ b/libs/libzauth/libzauth-c/src/lib.rs @@ -30,7 +30,8 @@ use std::slice; use std::str; use zauth::acl; use zauth::{ - verify_oauth_token, Acl, Error, Keystore, OauthError, Token, TokenType, TokenVerification, + verify_oauth_token, verify_oauth_token_scopes, Acl, Error, Keystore, OauthError, Token, + TokenType, TokenVerification, }; /// Variant of std::try! that returns the unwrapped error. @@ -440,6 +441,8 @@ pub extern "C" fn oauth_key_delete(a: *mut OAuthPubJwk) { ); } +/// Verify against the deprecated `oauth_scope` directive, which holds the base +/// of a scope and leaves the tier to the request method. #[no_mangle] pub extern "C" fn oauth_verify_token( jwk: &OAuthPubJwk, @@ -450,6 +453,60 @@ pub extern "C" fn oauth_verify_token( method: *const u8, method_len: size_t, ) -> OAuthResult { + oauth_verify( + jwk, + token, + token_len, + scope, + scope_len, + method, + method_len, + verify_oauth_token, + ) +} + +/// Verify against the `oauth_scopes` directive, which holds whole scopes, +/// separated by spaces. +#[no_mangle] +pub extern "C" fn oauth_verify_token_scopes( + jwk: &OAuthPubJwk, + token: *const u8, + token_len: size_t, + scopes: *const u8, + scopes_len: size_t, + method: *const u8, + method_len: size_t, +) -> OAuthResult { + oauth_verify( + jwk, + token, + token_len, + scopes, + scopes_len, + method, + method_len, + verify_oauth_token_scopes, + ) +} + +/// A NULL `scope` is how a location with nothing configured arrives here, and +/// there is nothing we could let the token do: no scope, no access. +// Monomorphize over `F` with `#[inline]` to avoid indirect call overhead through `fn` pointers. +#[allow(clippy::too_many_arguments)] +#[inline] +fn oauth_verify( + jwk: &OAuthPubJwk, + token: *const u8, + token_len: size_t, + scope: *const u8, + scope_len: size_t, + method: *const u8, + method_len: size_t, + verify: F, +) -> OAuthResult +where + F: Fn(&str, &str, &str, &str) -> Result + std::panic::RefUnwindSafe, +{ match panic::catch_unwind(|| { if token.is_null() { return OAuthResult { @@ -475,7 +532,7 @@ pub extern "C" fn oauth_verify_token( let scope = try_unwrap!(str::from_utf8(bytes)); let bytes = unsafe { slice::from_raw_parts(method, method_len) }; let method = str::from_utf8(bytes).unwrap(); - let subject = try_unwrap!(verify_oauth_token(&jwk.0, token, scope, method)); + let subject = try_unwrap!(verify(&jwk.0, token, scope, method)); let c_str = try_unwrap!(CString::new(subject)); OAuthResult { uid: c_str.into_raw(), diff --git a/libs/libzauth/libzauth-c/src/zauth.h b/libs/libzauth/libzauth-c/src/zauth.h index 33878c88204..3fc3d707759 100644 --- a/libs/libzauth/libzauth-c/src/zauth.h +++ b/libs/libzauth/libzauth-c/src/zauth.h @@ -83,7 +83,10 @@ uint8_t zauth_token_version(ZauthToken const *); Range zauth_token_lookup(ZauthToken const *, uint8_t); ZauthResult zauth_token_allowed(ZauthToken const *, ZauthAcl const *, uint8_t const * path, size_t len, uint8_t * result); void zauth_token_delete(ZauthToken *); +// 's' is the base of a scope, e.g. "conversations_code" (deprecated). OAuthResult oauth_verify_token(OAuthPubJwk const *, uint8_t const * t, size_t t_len, uint8_t const * s, size_t s_len, uint8_t const * m, size_t m_len); +// 's' is a space separated list of whole scopes, e.g. "read:conversations_code write-only:conversations_code". +OAuthResult oauth_verify_token_scopes(OAuthPubJwk const *, uint8_t const * t, size_t t_len, uint8_t const * s, size_t s_len, uint8_t const * m, size_t m_len); OAuthResultStatus oauth_result_uid_delete(char *); #ifdef __cplusplus diff --git a/libs/libzauth/libzauth/src/lib.rs b/libs/libzauth/libzauth/src/lib.rs index 195d06138bf..a89dd0274fd 100644 --- a/libs/libzauth/libzauth/src/lib.rs +++ b/libs/libzauth/libzauth/src/lib.rs @@ -36,4 +36,4 @@ mod matcher; pub use acl::Acl; pub use error::Error; pub use zauth::{Keystore, Token, TokenType, TokenVerification}; -pub use oauth::{verify_oauth_token, OauthError}; +pub use oauth::{verify_oauth_token, verify_oauth_token_scopes, OauthError}; diff --git a/libs/libzauth/libzauth/src/oauth.rs b/libs/libzauth/libzauth/src/oauth.rs index 058e0b7cdae..a01b38b3c4e 100644 --- a/libs/libzauth/libzauth/src/oauth.rs +++ b/libs/libzauth/libzauth/src/oauth.rs @@ -23,12 +23,36 @@ pub struct OAuthToken { pub scope: String, } +/// Verify a token against the deprecated `oauth_scope` directive, which names +/// only the base of a scope (`conversations_code`) and leaves the tier to the +/// request method. pub fn verify_oauth_token( jwk: &str, token: &str, required_scope: &str, method: &str, ) -> Result { + let (subject, scopes) = verify_token(jwk, token)?; + verify_scope(&scopes, required_scope, method)?; + Ok(subject) +} + +/// Verify a token against the `oauth_scopes` directive, which lists whole +/// scopes (`read:conversations_code write-only:conversations_code`), tier +/// included. +pub fn verify_oauth_token_scopes( + jwk: &str, + token: &str, + required_scopes: &str, + method: &str, +) -> Result { + let (subject, scopes) = verify_token(jwk, token)?; + verify_scopes(&scopes, required_scopes, method)?; + Ok(subject) +} + +/// Check the signature and return `(subject, scope claim)`. +fn verify_token(jwk: &str, token: &str) -> Result<(String, String), OauthError> { let jwk = serde_json::from_str::(jwk)?; let key = try_from_jwk(&jwk)?; let options = VerificationOptions { @@ -37,35 +61,66 @@ pub fn verify_oauth_token( }; let claims = key.verify_token::(token, Some(options))?; let subject = claims.subject.ok_or(OauthError::InvalidJwtNoSubject)?; - verify_scope(&claims.custom.scope, required_scope, method)?; - Ok(subject) + Ok((subject, claims.custom.scope)) +} + +/// Compatibility: `authorized_scopes` must be new syntax, +/// `required_scopes` can be old or new. +fn verify_scopes( + authorized_scopes: &str, + required_scopes: &str, + method: &str, +) -> Result<(), OauthError> { + let tier = required_tier(method)?; + let required = required_scopes + .split_whitespace() + .filter(|s| scope_tier(s) == Some(tier)) + .map(|s| s.to_string()) + .collect::>(); + verify_any(authorized_scopes, &required) +} + +fn scope_tier(scope: &str) -> Option<&str> { + scope.split_once(':').map(|(tier, _)| tier) } -// if method is GET, authorized scopes must contain either read:_, write:_, or admin:_ -// if method is POST, authorized scopes must contain either write:_ or admin:_ -// if method is PUT, authorized scopes must contain either write:_ or admin:_ -// if method is DELETE, authorized scopes must contain admin:_ -// FUTUREWORK: this works for now, but maybe we should consider using a more flexible scope system in the future -// e.g. by using a configuration file with a mapping of scopes to methods and paths +/// This is deprecated, use tiers "read", "write-only", "delete-only" +/// instead. See `verify_scopes` below. +/// +/// Deprecated behavior: +/// +/// if method is GET, authorized scopes must contain either read:_, write:_, or admin:_ +/// if method is POST, authorized scopes must contain either write:_ or admin:_ +/// if method is PUT, authorized scopes must contain either write:_ or admin:_ +/// if method is DELETE, authorized scopes must contain admin:_ +/// +/// Compatibility: `authorized_scopes` may be old or new syntax; +/// `required_scopes` must be a bare base name (old syntax), the tier +/// is added implicitly. fn verify_scope( authorized_scopes: &str, required_scope: &str, method: &str, ) -> Result<(), OauthError> { - let valid_scopes = match method.to_uppercase().as_str() { - "GET" => Ok(vec!["read", "write", "admin"]), - "POST" => Ok(vec!["write", "admin"]), - "PUT" => Ok(vec!["write", "admin"]), - "DELETE" => Ok(vec!["admin"]), + let required = vec![format!("{}:{}", required_tier(method)?, required_scope)]; + verify_any(authorized_scopes, &required) +} + +fn required_tier(method: &str) -> Result<&'static str, OauthError> { + match method.to_uppercase().as_str() { + "GET" => Ok("read"), + "POST" => Ok("write-only"), + "PUT" => Ok("write-only"), + "DELETE" => Ok("delete-only"), _ => Err(OauthError::InvalidScope), - }? - .iter() - .map(|s| format!("{}:{}", s, required_scope)) - .collect::>(); + } +} +fn verify_any(authorized_scopes: &str, required: &[String]) -> Result<(), OauthError> { let valid = authorized_scopes .split_whitespace() - .any(|s| valid_scopes.contains(&s.to_string())); + .flat_map(granted_scopes) + .any(|granted| required.contains(&granted)); if !valid { return Err(OauthError::InvalidScope); @@ -73,6 +128,23 @@ fn verify_scope( Ok(()) } +/// Which scopes does a scope carried by a token (old or new syntax) +/// grant, in the vocabulary [`required_tier`] speaks? +fn granted_scopes(scope: &str) -> Vec { + let Some((tier, base)) = scope.split_once(':') else { + return Vec::new(); + }; + let tiers: &[&str] = match tier { + "read" => &["read"], + "write" => &["read", "write-only"], + "admin" => &["read", "write-only", "delete-only"], + "write-only" => &["write-only"], + "delete-only" => &["delete-only"], + _ => &[], + }; + tiers.iter().map(|t| format!("{}:{}", t, base)).collect() +} + fn try_from_jwk(jwk: &Jwk) -> Result { Ok(match &jwk.algorithm { AlgorithmParameters::OctetKeyPair(p) => { @@ -141,6 +213,73 @@ mod tests { assert!(verify_scope("foo bar", "self", "DELETE").is_err()); } + #[test] + fn should_grant_scopes() { + assert_eq!(granted_scopes("read:self"), vec!["read:self"]); + assert_eq!( + granted_scopes("write:self"), + vec!["read:self", "write-only:self"] + ); + assert_eq!( + granted_scopes("admin:self"), + vec!["read:self", "write-only:self", "delete-only:self"] + ); + assert_eq!(granted_scopes("write-only:self"), vec!["write-only:self"]); + assert_eq!(granted_scopes("delete-only:self"), vec!["delete-only:self"]); + assert!(granted_scopes("self").is_empty()); + assert!(granted_scopes("nonsense:self").is_empty()); + } + + // The deprecated directive with scopes as they are handed out now. + #[test] + fn should_verify_scope_with_new_scopes() { + assert!(verify_scope("read:self foo bar", "self", "GET").is_ok()); + assert!(verify_scope("write-only:self", "self", "GET").is_err()); // this is the interesting case. + assert!(verify_scope("write-only:self", "self", "POST").is_ok()); + assert!(verify_scope("write-only:self", "self", "PUT").is_ok()); + assert!(verify_scope("write-only:self", "self", "DELETE").is_err()); + assert!(verify_scope("delete-only:self", "self", "DELETE").is_ok()); + assert!(verify_scope("delete-only:self", "self", "POST").is_err()); + assert!(verify_scope("read:other write-only:other", "self", "GET").is_err()); + } + + #[test] + fn should_verify_scopes() { + let cfg = "read:self write-only:self"; + assert!(verify_scopes("read:self", cfg, "GET").is_ok()); + assert!(verify_scopes("write-only:self", cfg, "POST").is_ok()); + assert!(verify_scopes("write-only:self", cfg, "PUT").is_ok()); + assert!(verify_scopes("read:self", cfg, "POST").is_err()); + assert!(verify_scopes("write-only:self", cfg, "GET").is_err()); + assert!(verify_scopes("read:self write-only:self", cfg, "DELETE").is_err()); + assert!(verify_scopes("read:other", cfg, "GET").is_err()); + assert!(verify_scopes("foo bar", cfg, "GET").is_err()); + } + + // A list without a scope of the tier the method needs lets nobody in, and + // neither does a location with no scope configured at all. + #[test] + fn should_verify_scopes_closed() { + assert!(verify_scopes("read:self write-only:self admin:self", "", "GET").is_err()); + assert!(verify_scopes("delete-only:self", "read:self write-only:self", "DELETE").is_err()); + assert!(verify_scopes("read:self", "read:self", "PATCH").is_err()); + } + + // Tokens handed out before the migration carry cumulative scopes and have + // to keep working against a migrated location. + #[test] + fn should_verify_scopes_with_old_scopes() { + let cfg = "read:conversations_code write-only:conversations_code"; + assert!(verify_scopes("write:conversations_code", cfg, "GET").is_ok()); + assert!(verify_scopes("write:conversations_code", cfg, "POST").is_ok()); + assert!(verify_scopes("read:conversations_code", cfg, "GET").is_ok()); + assert!(verify_scopes("read:conversations_code", cfg, "POST").is_err()); + assert!(verify_scopes("admin:conversations_code", cfg, "GET").is_ok()); + assert!(verify_scopes("admin:conversations_code", cfg, "POST").is_ok()); + // ... but only as far as the location goes: nothing here is deletable. + assert!(verify_scopes("admin:conversations_code", cfg, "DELETE").is_err()); + } + #[test] fn should_verify_oauth_token() { let uid = "842ddbc8-56ec-408d-9fa8-7a8c37ad22a7"; @@ -158,6 +297,28 @@ mod tests { verify_oauth_token(&serde_json::to_string(&jwk).unwrap(), &jwt, "test", "GET").unwrap(); assert_eq!(&subject, uid); } + + #[test] + fn should_verify_oauth_token_scopes() { + let uid = "842ddbc8-56ec-408d-9fa8-7a8c37ad22a7"; + let key = Ed25519KeyPair::generate(); + let jwk = mk_jwk(key.public_key()); + let token = Claims::with_custom_claims( + OAuthToken { + scope: "write-only:foo read:test".to_string(), + }, + Duration::from_secs(3600), + ) + .with_subject(uid); + let jwt = key.sign::(token).unwrap(); + let jwk = serde_json::to_string(&jwk).unwrap(); + let subject = + verify_oauth_token_scopes(&jwk, &jwt, "read:test write-only:test", "GET").unwrap(); + assert_eq!(&subject, uid); + assert!(verify_oauth_token_scopes(&jwk, &jwt, "read:test write-only:test", "POST").is_err()); + assert!(verify_oauth_token_scopes(&jwk, &jwt, "write-only:foo", "POST").is_ok()); + } + fn mk_jwk(key: Ed25519PublicKey) -> Jwk { let x = base64::prelude::BASE64_URL_SAFE_NO_PAD.encode(&key.to_bytes()); Jwk { diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index 9392b7f909e..e42effe91ff 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -45,7 +45,7 @@ import Imports hiding (exp, head) import Prelude.Singletons (Show_) import Servant hiding (Handler, JSON, Tagged, addHeader, respond) import Servant.OpenApi.Internal.Orphans () -import Test.QuickCheck (Arbitrary (..)) +import Test.QuickCheck (Arbitrary (..), listOf1) import URI.ByteString import URI.ByteString.QQ qualified as URI.QQ import Web.FormUrlEncoded (Form (..), FromForm (..), ToForm (..), parseUnique) @@ -195,50 +195,60 @@ instance ToSchema OAuthResponseType where -- with the supported scopes defined in the nginx configs. -- However, having this typed makes it easier to handle scopes in the backend, -- and e.g. provide more meaningful error messages when the scope is invalid. +-- +-- Furthermore, since tokens are only issued if the scopes listed in +-- the request can be parsed, so this type also guarantees that no +-- unusable tokens can be issued. +-- +-- A scope is a tier and a base, but not every combination of the two is a +-- scope: only the ones listed here exist, and they are the ones the routing +-- tables document and @charts/nginz/values.yaml@ enforces. Anything else is +-- rejected by 'FromByteString', so nobody can be granted e.g. a +-- @delete-only:conversations_code@ that no endpoint honours. data OAuthScope = ReadFeatureConfigs | ReadSelf - | WriteConversations - | WriteConversationsCode - | WriteConversationsName - | WriteMeetings - | AdminMeetings + | ReadConversationsCode + | WriteOnlyConversations + | WriteOnlyConversationsCode + | WriteOnlyConversationsName + | WriteOnlyMeetings deriving (Eq, Show, Generic, Ord, Bounded, Enum) deriving (Arbitrary) via (GenericUniform OAuthScope) class IsOAuthScope scope where toOAuthScope :: OAuthScope -instance IsOAuthScope 'WriteConversations where - toOAuthScope = WriteConversations - -instance IsOAuthScope 'WriteConversationsCode where - toOAuthScope = WriteConversationsCode +instance IsOAuthScope 'ReadFeatureConfigs where + toOAuthScope = ReadFeatureConfigs instance IsOAuthScope 'ReadSelf where toOAuthScope = ReadSelf -instance IsOAuthScope 'ReadFeatureConfigs where - toOAuthScope = ReadFeatureConfigs +instance IsOAuthScope 'ReadConversationsCode where + toOAuthScope = ReadConversationsCode -instance IsOAuthScope 'WriteConversationsName where - toOAuthScope = WriteConversationsName +instance IsOAuthScope 'WriteOnlyConversations where + toOAuthScope = WriteOnlyConversations -instance IsOAuthScope 'WriteMeetings where - toOAuthScope = WriteMeetings +instance IsOAuthScope 'WriteOnlyConversationsCode where + toOAuthScope = WriteOnlyConversationsCode -instance IsOAuthScope 'AdminMeetings where - toOAuthScope = AdminMeetings +instance IsOAuthScope 'WriteOnlyConversationsName where + toOAuthScope = WriteOnlyConversationsName + +instance IsOAuthScope 'WriteOnlyMeetings where + toOAuthScope = WriteOnlyMeetings instance ToByteString OAuthScope where builder = \case - WriteConversations -> "write:conversations" - WriteConversationsCode -> "write:conversations_code" - WriteConversationsName -> "write:conversations_name" - WriteMeetings -> "write:meetings" - AdminMeetings -> "admin:meetings" - ReadSelf -> "read:self" ReadFeatureConfigs -> "read:feature_configs" + ReadSelf -> "read:self" + ReadConversationsCode -> "read:conversations_code" + WriteOnlyConversations -> "write-only:conversations" + WriteOnlyConversationsCode -> "write-only:conversations_code" + WriteOnlyConversationsName -> "write-only:conversations_name" + WriteOnlyMeetings -> "write-only:meetings" instance FromByteString OAuthScope where parser = do @@ -249,9 +259,17 @@ instance FromByteString OAuthScope where Nothing -> fail $ "invalid scope: " <> show s newtype OAuthScopes = OAuthScopes {unOAuthScopes :: Set OAuthScope} - deriving (Eq, Show, Generic, Monoid, Semigroup, Arbitrary) + deriving (Eq, Show, Generic, Monoid, Semigroup) deriving (A.ToJSON, A.FromJSON, S.ToSchema) via (Schema OAuthScopes) +instance Arbitrary OAuthScopes where + arbitrary = OAuthScopes . Set.fromList <$> listOf1 arbitrary + shrink (OAuthScopes s) = + [ OAuthScopes (Set.fromList xs) + | xs <- shrink (Set.toList s), + not (null xs) + ] + instance ToSchema OAuthScopes where schema = OAuthScopes <$> (oauthScopesToText . unOAuthScopes) .= withParser schema oauthScopeParser where @@ -261,13 +279,52 @@ instance ToSchema OAuthScopes where . fmap (TE.decodeUtf8With lenientDecode . toByteString') . Set.toList + -- A scope we do not know is an error. Silently dropping it, or silently + -- returning no scopes at all, would hand out a token that does not do + -- what the client asked for. oauthScopeParser :: Text -> A.Parser (Set OAuthScope) - oauthScopeParser scope = - pure $ - (not . T.null) - `filter` T.splitOn " " scope - & maybe Set.empty Set.fromList - . mapM (fromByteString' . fromStrict . TE.encodeUtf8) + oauthScopeParser scope = do + let ws = T.words scope + when (null ws) $ fail ("empty scope; " <> validScopes) + Set.fromList <$> mapM parseScope ws + + parseScope :: Text -> A.Parser OAuthScope + parseScope s = + (fromByteString' . fromStrict . TE.encodeUtf8) s + & maybe (fail ("invalid scope: " <> show s <> "; " <> validScopes)) pure + + validScopes :: String + validScopes = + "valid scopes are: " + <> T.unpack (oauthScopesToText (Set.fromList [minBound ..])) + +-- | A scope as it can appear in the database, in terms of the scopes we have +-- now. +-- +-- We write what 'ToByteString' gives us, but rows outlive renamings: a refresh +-- token handed out before the tiers were split up carries a deprecated, +-- cumulative scope. Reading one is not one-to-one, because @write:x@ implies +-- @read:x@, hence the 'Set'. Scopes are stored in a set column +-- anyway, so the extra elements simply join it. +-- +-- Anything we cannot make sense of yields no scopes at all. That can only +-- shrink what a token may do, never grow it. +storedScope :: Text -> Set OAuthScope +storedScope t = + (fromByteString' . fromStrict . TE.encodeUtf8) t + & maybe (deprecatedScope t) Set.singleton + +-- | The cumulative scopes we used to hand out. Only scopes that exist now can +-- come out of this, so @admin:meetings@ shrinks to @write-only:meetings@: there +-- is no @delete-only:meetings@ to grant, and no endpoint that would honour it. +deprecatedScope :: Text -> Set OAuthScope +deprecatedScope = \case + "write:conversations" -> Set.fromList [WriteOnlyConversations] + "write:conversations_code" -> Set.fromList [ReadConversationsCode, WriteOnlyConversationsCode] + "write:conversations_name" -> Set.fromList [WriteOnlyConversationsName] + "write:meetings" -> Set.fromList [WriteOnlyMeetings] + "admin:meetings" -> Set.fromList [WriteOnlyMeetings] + _ -> Set.empty data CodeChallengeMethod = S256 deriving (Eq, Show, Generic) @@ -762,16 +819,22 @@ instance Cql OAuthAuthorizationCode where fromCql (CqlAscii t) = OAuthAuthorizationCode <$> validateBase16 t fromCql _ = Left "OAuthAuthorizationCode: Ascii expected" -instance Cql OAuthScope where - ctype = Tagged TextColumn - toCql = CqlText . TE.decodeUtf8With lenientDecode . toByteString' - fromCql (CqlText t) = - maybe (Left "invalid oauth scope") Right - $ fromByteString' - . fromStrict - . TE.encodeUtf8 - $ t - fromCql _ = Left "OAuthScope: Text expected" +-- | Scopes are read and written as a whole set, not one at a time: a single +-- stored scope can stand for several of ours, see 'storedScope'. +instance Cql OAuthScopes where + ctype = Tagged (SetColumn TextColumn) + + toCql = + CqlSet + . fmap (CqlText . TE.decodeUtf8With lenientDecode . toByteString') + . Set.toList + . unOAuthScopes + + fromCql (CqlSet scopes) = OAuthScopes . Set.unions <$> mapM el scopes + where + el (CqlText t) = Right (storedScope t) + el _ = Left "OAuthScopes: Text expected" + fromCql _ = Left "OAuthScopes: Set expected" instance Cql OAuthCodeChallenge where ctype = Tagged BlobColumn diff --git a/libs/wire-api/src/Wire/API/Routes/Public.hs b/libs/wire-api/src/Wire/API/Routes/Public.hs index cacd78420dd..969f43bee30 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public.hs @@ -363,7 +363,7 @@ addScopeDescription = -- that test only.) renderOAuthScope :: OAuth.OAuthScope -> Text renderOAuthScope scope = - "\nOAuth scope: `" + "
OAuth scope: `" <> (decodeUtf8With lenientDecode . toStrict . toByteString $ scope) <> "`" diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs index 68bb60a9d3f..77bc1549d14 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs @@ -407,7 +407,7 @@ type ConversationAPI = :<|> Named "create-group-conversation@v2" ( Summary "Create a new conversation" - :> DescriptionOAuthScope 'WriteConversations + :> DescriptionOAuthScope 'WriteOnlyConversations :> Until 'V3 :> CanThrow 'ConvAccessDenied :> CanThrow 'MLSNonEmptyMemberList @@ -430,7 +430,7 @@ type ConversationAPI = :<|> Named "create-group-conversation@v3" ( Summary "Create a new conversation" - :> DescriptionOAuthScope 'WriteConversations + :> DescriptionOAuthScope 'WriteOnlyConversations :> From 'V3 :> Until 'V4 :> CanThrow 'ConvAccessDenied @@ -534,7 +534,7 @@ type ConversationAPI = :<|> Named "create-group-conversation" ( Summary "Create a new conversation" - :> DescriptionOAuthScope 'WriteConversations + :> DescriptionOAuthScope 'WriteOnlyConversations :> From 'V16 :> CanThrow 'ConvAccessDenied :> CanThrow 'MLSNonEmptyMemberList @@ -1085,7 +1085,7 @@ type ConversationAPI = "create-conversation-code-unqualified@v3" ( Summary "Create or recreate a conversation code" :> Until 'V4 - :> DescriptionOAuthScope 'WriteConversationsCode + :> DescriptionOAuthScope 'WriteOnlyConversationsCode :> CanThrow 'ConvAccessDenied :> CanThrow 'ConvNotFound :> CanThrow 'GuestLinksDisabled @@ -1104,7 +1104,7 @@ type ConversationAPI = "create-conversation-code-unqualified" ( Summary "Create or recreate a conversation code" :> From 'V4 - :> DescriptionOAuthScope 'WriteConversationsCode + :> DescriptionOAuthScope 'WriteOnlyConversationsCode :> CanThrow 'ConvAccessDenied :> CanThrow 'ConvNotFound :> CanThrow 'GuestLinksDisabled @@ -1151,7 +1151,7 @@ type ConversationAPI = :<|> Named "get-code" ( Summary "Get existing conversation code" - :> DescriptionOAuthScope 'WriteConversationsCode + :> DescriptionOAuthScope 'ReadConversationsCode :> CanThrow 'CodeNotFound :> CanThrow 'ConvAccessDenied :> CanThrow 'ConvNotFound @@ -1340,7 +1340,7 @@ type ConversationAPI = :<|> Named "update-conversation-name" ( Summary "Update conversation name" - :> DescriptionOAuthScope 'WriteConversationsName + :> DescriptionOAuthScope 'WriteOnlyConversationsName :> CanThrow ('ActionDenied 'ModifyConversationName) :> CanThrow 'ConvNotFound :> CanThrow 'InvalidOperation diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs index db65df71470..5cf22e5c41c 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs @@ -50,7 +50,7 @@ type MeetingsAPI = :<|> Named "create-meeting" ( Summary "Create a new meeting" - :> DescriptionOAuthScope 'WriteMeetings + :> DescriptionOAuthScope 'WriteOnlyMeetings :> From 'V17 :> ZLocalUser :> ZConn diff --git a/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs b/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs index a7775f8af1b..21f70112f1b 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs @@ -20,6 +20,7 @@ module Test.Wire.API.OAuth where import Data.Aeson +import Data.Set qualified as Set import Imports import Test.Tasty import Test.Tasty.HUnit @@ -30,9 +31,49 @@ tests = testGroup "Oauth" $ [ testGroup "code challenge verification should succeed" $ [ testCase "should" testCodeChallengeVerification + ], + testGroup "scopes" $ + [ testCase "only known scopes parse" testScopesParseOnlyKnown, + testCase "stored scopes cover the deprecated ones" testStoredScopes ] ] +-- | Rows outlive renamings, so what we read from the database is not what a +-- client may ask for. A deprecated scope was cumulative and can be worth more +-- than one of ours. +testStoredScopes :: Assertion +testStoredScopes = do + -- what we write now + storedScope "read:self" @?= Set.singleton ReadSelf + storedScope "write-only:conversations" @?= Set.singleton WriteOnlyConversations + -- the deprecated write tier was a read tier as well + storedScope "write:conversations_code" + @?= Set.fromList [ReadConversationsCode, WriteOnlyConversationsCode] + -- the deprecated admin tier had a delete tier on top, but there is no + -- delete-only:meetings to grant + storedScope "admin:meetings" @?= Set.singleton WriteOnlyMeetings + -- nothing we could honour + storedScope "read:pizza" @?= Set.empty + storedScope "smell:meetings" @?= Set.empty + +-- | A scope nobody can be granted has to be an error. If it were dropped, or +-- turned the whole set into no scopes at all, the client would get a token that +-- does not do what it asked for. +testScopesParseOnlyKnown :: Assertion +testScopesParseOnlyKnown = do + (eitherDecode "\"read:self write-only:conversations\"" :: Either String OAuthScopes) + @?= Right (OAuthScopes (Set.fromList [ReadSelf, WriteOnlyConversations])) + for_ + [ "\"\"", -- empty scope + "\"read:pizza\"", -- no such scope + "\"write:conversations\"", -- deprecated tier + "\"read:self read:pizza\"" -- one bad scope spoils the request + ] + $ \bad -> case eitherDecode bad :: Either String OAuthScopes of + Left _ -> pure () + Right scopes -> + assertFailure $ "expected a parse error for " <> show bad <> ", got " <> show scopes + testCodeChallengeVerification :: Assertion testCodeChallengeVerification = do mkChallenge codeVerifier @?= codeChallenge diff --git a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs index 0400bc3a9a6..f54d872caac 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs @@ -17,26 +17,12 @@ -- You should have received a copy of the GNU Affero General Public License along -- with this program. If not, see . --- | Two independent places declare which OAuth scope an endpoint needs, and --- nothing keeps them in sync: --- --- 1. @charts/nginz/values.yaml@ -- @oauth_scope:@ on an upstream entry. This is --- what is actually /enforced/: nginz rejects OAuth tokens without the scope. --- 2. The servant routing tables -- 'Wire.API.Routes.Public.DescriptionOAuthScope'. --- This is only /documentation/: it appends a line to the endpoint description --- in the swagger docs and has no effect on request handling. --- --- Forgetting (2) while doing (1) -- or, more commonly, adding a new version of an --- endpoint that is already covered by (1) and not carrying the annotation over -- --- silently produces endpoints that reject OAuth tokens for a scope documented --- nowhere. This module compares the two for the development version, which is --- the only one still assembled from the routing tables. module Test.Wire.API.Routes.OAuthScopes (tests) where import Data.Aeson qualified as A import Data.Aeson.Key qualified as Key import Data.Aeson.KeyMap qualified as KeyMap -import Data.ByteString.Conversion (toByteString') +import Data.ByteString.Conversion import Data.FileEmbed (embedFile, makeRelativeToProject) import Data.Map qualified as Map import Data.Set qualified as Set @@ -49,7 +35,7 @@ import Servant.API (toUrlPiece) import Test.Tasty import Test.Tasty.HUnit import Text.Regex.TDFA ((=~)) -import Wire.API.OAuth (OAuthScope) +import Wire.API.OAuth import Wire.API.Routes.Public (renderOAuthScope) import Wire.API.Routes.Public.Swagger (devVersion, devVersionSwagger) import Wire.API.Routes.Version @@ -58,14 +44,80 @@ tests :: TestTree tests = testGroup "OAuth scopes (charts/nginz/values.yaml vs. swagger docs)" - [ testCase "nginz path patterns avoid PCRE-only constructs" testPatternVocabulary, - testCase "every nginz oauth_scope names a real scope" testScopeNamesAreReal, - testCase "enforced scopes and documented scopes agree" testScopesAgree + [ testCase "enforced scopes and documented scopes agree" testScopesAgree, + testCase "nginz path patterns avoid PCRE-only constructs" testPatternVocabulary ] +-- | Two independent places declare which OAuth scope an endpoint needs, and +-- nothing keeps them in sync: +-- +-- 1. @charts/nginz/values.yaml@: @oauth_scope:@ (deprecated) or @oauth_scopes:@ +-- on an upstream entry. This is what is actually /enforced/: nginz rejects +-- OAuth tokens without a matching scope, and rejects all of them +-- where no scope is configured at all. +-- 2. The servant routing tables: 'Wire.API.Routes.Public.DescriptionOAuthScope'. +-- This is only documentation: it appends a line to the endpoint description +-- in the swagger docs and has no effect on request handling. +-- +-- This test matches the openapi docs generated from servant routes +-- against what nginz enforces. The behavior of nginz is emulated by +-- this test. Actual behavior of libzauth is tested in the rust code; +-- those tests and these here need to be kept in sync manually +-- (compare `enforcedScopes` below with +-- `/libs/libzauth/libzauth/src/oauth.rs` (search for `mod tests`)). +testScopesAgree :: Assertion +testScopesAgree = do + unless (Set.null actual) . assertFailure . T.unpack . T.unlines $ + [ "OAuth scope declarations are out of sync.", + "", + "Columns: version, method, path, accepted by nginz, documented in swagger.", + "The nginz column lists every scope that gets an OAuth token through to that", + "method; '[]' means none does, i.e. OAuth is not usable there at all (the route", + "may still be reachable with a zauth cookie or token). A finding means", + "swagger.json does not match values.yaml:", + "", + " accepted but not documented charts/nginz/values.yaml lets a scope through", + " that the swagger docs do not mention -- most", + " likely a missing DescriptionOAuthScope in the", + " routing table, e.g. on a newly added version of", + " an endpoint that already had one.", + " documented but not accepted the swagger docs promise a scope that does not", + " get anybody in -- a stale annotation, or a scope", + " missing from charts/nginz/values.yaml.", + "" + ] + <> section "deviations:" actual + where + actual = Set.fromList (renderFinding <$> findings) + section title xs + | Set.null xs = [] + | otherwise = [" " <> title] <> ((" " <>) <$> Set.toAscList xs) <> [""] + +testPatternVocabulary :: Assertion +testPatternVocabulary = + for_ nginzLocations $ \loc -> + for_ pcreOnlyConstructs $ \bad -> + when (bad `T.isInfixOf` locPattern loc) $ + assertFailure . T.unpack $ + "charts/nginz/values.yaml: the path pattern " + <> locPattern loc + <> " uses '" + <> bad + <> "', which nginx reads as PCRE but this test matches with regex-tdfa, " + <> "i.e. POSIX ERE. The two may disagree, which would be bad." + -------------------------------------------------------------------------------- -- what nginz enforces +data OAuthTier = Read | WriteOnly | DeleteOnly + deriving (Eq, Show) + +instance ToByteString OAuthTier where + builder = \case + Read -> "read" + WriteOnly -> "write-only" + DeleteOnly -> "delete-only" + -- | The locations nginz emits, in the order it emits them. -- -- @charts/nginz/templates/_helpers.tpl@ merges @upstreams@ (minus @@ -78,29 +130,80 @@ newtype NginzLocations = NginzLocations [Location] data Location = Location { locPattern :: Text, - locScope :: Maybe Text + locOldScope :: Maybe Text, -- only the base, e.g. "conversations_code": no tier without knowing the method. + locNewScopes :: Maybe [OAuthScope] } --- | The scope an OAuth token needs to get past nginz to this endpoint. +-- | Which scopes let an OAuth token through to this method and path? The +-- answer is always given in new scopes, also where values.yaml still uses old +-- ones. -- --- libzauth accepts a whole tier range per method, so a token holding --- @admin:meetings@ may also @POST@. Documenting every accepted scope would be --- noise, and would force @POST@ to be annotated with both @write:@ and --- @admin:@; what the docs should name is the /least/ privilege that suffices, --- so we take the lowest tier that is actually grantable. +-- If the matching location has @oauth_scopes@, the answer is the +-- listed scopes filtered by the tier corresponding to the HTTP +-- method. -- --- Empty when nginz requires no scope, and also when no tier it would accept is --- in 'Wire.API.OAuth.OAuthScope' -- then the endpoint cannot be reached with an --- OAuth token at all and there is nothing to document. Mistyped scope names --- are caught by 'testScopeNamesAreReal'. +-- If it only has the deprecated @oauth_scope@, the answer is the one scope made +-- of that base and the tier this method needs: under @oauth_scope: +-- conversations_code@, a @GET@ wants @read:conversations_code@ and nothing +-- else. -- --- FUTUREWORK(fisx): https://wearezeta.atlassian.net/browse/WPB-28193 -enforcedScopes :: Text -> Text -> Set Text -enforcedScopes method path = - maybe Set.empty Set.singleton $ do - loc <- find (`locationMatches` path) nginzLocations - base <- locScope loc - find (`Set.member` grantableScopes) [tier <> ":" <> base | tier <- methodScopeTiers method] +-- NB: an empty answer means no OAuth token gets in at all. That happens if +-- the location has no @oauth_scope[s]@, if its @oauth_scopes@ list has nothing +-- of the tier the method needs, or if the method is one nginz has no rule for. +enforcedScopes :: Text -> Text -> Set OAuthScope +enforcedScopes method path = case find locationMatches nginzLocations of + Nothing -> Set.empty + Just loc -> case (loc.locOldScope, loc.locNewScopes) of + (_, Just newScopes) -> + -- Filter scopes listed in values.yaml by matching method/tier. + Set.fromList (filter hasTierFor newScopes) + (Just base, Nothing) -> + -- The deprecated attribute gives the base; the tier comes from the method. + maybe Set.empty Set.singleton (oldScopeBase base) + (Nothing, Nothing) -> Set.empty + where + -- Does this location capture that path? nginx anchors regex locations at the + -- start of the URI but not at the end, so a pattern without a trailing @$@ + -- matches every path with that prefix. + -- + -- The patterns are PCRE (that is what nginx uses) and we match them with + -- regex-tdfa, which is POSIX ERE. The two agree on the handful of constructs + -- values.yaml actually uses; 'testPatternVocabulary' keeps it that way. + locationMatches :: Location -> Bool + locationMatches loc = + T.unpack (probePath path) =~ T.unpack ("^" <> locPattern loc) + + -- @/conversations/{cnv}/code@ becomes @/conversations/PARAM/code@: the literal + -- segments still have to match, the captures must not. + probePath :: Text -> Text + probePath t = + let (before, rest) = T.breakOn "{" t + in if T.null rest + then before + else before <> "PARAM" <> probePath (T.drop 1 (T.dropWhile (/= '}') rest)) + + hasTierFor :: OAuthScope -> Bool + hasTierFor scope = case newTier method of + Nothing -> False + Just tier -> + T.decodeUtf8 (toByteString' tier <> ":") + `T.isPrefixOf` T.decodeUtf8 (toByteString' scope) + + newTier :: Text -> Maybe OAuthTier + newTier = \case + "GET" -> Just Read + "POST" -> Just WriteOnly + "PUT" -> Just WriteOnly + "DELETE" -> Just DeleteOnly + _ -> Nothing + + -- Mirrors @verify_scope@ in @libs/libzauth/libzauth/src/oauth.rs@, which is + -- what nginz calls for a location with the deprecated attribute. 'Nothing' + -- for a base that is no scope of ours, e.g. a typo in values.yaml. + oldScopeBase :: Text -> Maybe OAuthScope + oldScopeBase base = do + tier <- newTier method + fromByteString (toByteString' tier <> ":" <> T.encodeUtf8 base) nginzLocations :: [Location] nginzLocations = @@ -141,58 +244,30 @@ instance A.FromJSON NginzLocations where <> Map.restrictKeys extra (Set.fromList (enabled :: [Text])) instance A.FromJSON Location where - parseJSON = A.withObject "nginz upstream entry" $ \o -> - Location <$> o A..: "path" <*> o A..:? "oauth_scope" + parseJSON = A.withObject "nginz upstream entry" $ \o -> do + path <- o A..: "path" + oldScope :: Maybe Text <- do + o A..:? "oauth_scope" + newScopes :: Maybe [OAuthScope] <- do + mbs :: Maybe [Text] <- o A..:? "oauth_scopes" + mapM (mapM validateNewScope) mbs + pure (Location path oldScope newScopes) --- | Does this location capture that path? nginx anchors regex locations at the --- start of the URI but not at the end, so a pattern without a trailing @$@ --- matches every path with that prefix. --- --- The patterns are PCRE (that is what nginx uses) and we match them with --- regex-tdfa, which is POSIX ERE. The two agree on the handful of constructs --- values.yaml actually uses; 'testPatternVocabulary' keeps it that way. -locationMatches :: Location -> Text -> Bool -locationMatches loc path = - T.unpack (probePath path) =~ T.unpack ("^" <> locPattern loc) - --- | @/conversations/{cnv}/code@ becomes @/conversations/PARAM/code@: the literal --- segments still have to match, the captures must not. -probePath :: Text -> Text -probePath t = - let (before, rest) = T.breakOn "{" t - in if T.null rest - then before - else before <> "PARAM" <> probePath (T.drop 1 (T.dropWhile (/= '}') rest)) +validateNewScope :: (MonadFail m) => Text -> m OAuthScope +validateNewScope s = + fromByteString @OAuthScope (T.encodeUtf8 s) + & maybe + (fail ("unknown new scope: " <> show s)) + pure pcreOnlyConstructs :: [Text] pcreOnlyConstructs = ["(?", "\\", "{", "*?", "+?"] --- | @oauth_scope: foo@ in values.yaml names a scope without a tier; libzauth --- decides which tiers satisfy it from the request method. See @verify_scope@ in --- @libs/libzauth/libzauth/src/oauth.rs@. Listed in increasing order of --- privilege: 'enforcedScopes' takes the first grantable one, so this order --- decides which scope an endpoint gets documented with. -methodScopeTiers :: Text -> [Text] -methodScopeTiers = \case - "GET" -> ["read", "write", "admin"] - "POST" -> ["write", "admin"] - "PUT" -> ["write", "admin"] - "DELETE" -> ["admin"] - _ -> [] - -------------------------------------------------------------------------------- -- what the swagger docs claim -documentedScopes :: Text -> Set Text -documentedScopes descr = - Set.fromList - [ T.decodeUtf8 (toByteString' scope) - | scope <- [minBound .. maxBound] :: [OAuthScope], - -- Recognise a documented scope by the very string - -- 'renderOAuthScope' produces, so that the two cannot drift - -- apart. - renderOAuthScope scope `T.isInfixOf` descr - ] +documentedScope :: Text -> Maybe OAuthScope +documentedScope descr = find ((`T.isInfixOf` descr) . renderOAuthScope) [minBound ..] httpMethods :: [Text] httpMethods = ["GET", "PUT", "POST", "DELETE", "OPTIONS", "HEAD", "PATCH", "TRACE"] @@ -214,30 +289,23 @@ operations doc = do -------------------------------------------------------------------------------- -- the comparison --- | 'Finding's are interesting iff @fEnforced /= fDocumented@. +-- | 'Finding's are interesting iff 'fDocumented' is not one of 'fEnforced'. data Finding = Finding { fVersion :: Version, fMethod :: Text, fPath :: Text, - fEnforced :: Set Text, - fDocumented :: Set Text + fEnforced :: Set OAuthScope, + fDocumented :: Maybe OAuthScope } --- | The scopes brig can actually issue. Anything else is not a scope at all: --- 'Wire.API.OAuth.OAuthScopes' fails to parse it, and yields the empty scope set --- for the whole request. -grantableScopes :: Set Text -grantableScopes = - Set.fromList [T.decodeUtf8 (toByteString' s) | s <- [minBound .. maxBound] :: [OAuthScope]] - renderFinding :: Finding -> Text renderFinding f = T.intercalate - "\t" + " " [ toUrlPiece (fVersion f), fMethod f, fPath f, - T.pack . show . toList $ fEnforced f, + T.pack . show . Set.toList $ fEnforced f, T.pack . show . toList $ fDocumented f ] @@ -246,61 +314,16 @@ findings = [ Finding devVersion method path enforced documented | (path, method, descr) <- operations (A.toJSON devVersionSwagger), let enforced = enforcedScopes method path, - let documented = documentedScopes descr, - enforced /= documented + let documented = documentedScope descr, + not (scopesMatch enforced documented) ] --------------------------------------------------------------------------------- --- the actual tests - -testPatternVocabulary :: Assertion -testPatternVocabulary = - for_ nginzLocations $ \loc -> - for_ pcreOnlyConstructs $ \bad -> - when (bad `T.isInfixOf` locPattern loc) $ - assertFailure . T.unpack $ - "charts/nginz/values.yaml: the path pattern " - <> locPattern loc - <> " uses '" - <> bad - <> "', which nginx reads as PCRE but this test matches with regex-tdfa, " - <> "i.e. POSIX ERE. The two may disagree, which would be bad." - --- | 'enforcedScopes' ignores scopes brig cannot issue, so a typo in an --- @oauth_scope:@ would otherwise make every endpoint under it drop silently out --- of the comparison. Require that each name is usable at some tier. -testScopeNamesAreReal :: Assertion -testScopeNamesAreReal = - for_ (nub (mapMaybe locScope nginzLocations)) $ \base -> - unless (any (\tier -> (tier <> ":" <> base) `Set.member` grantableScopes) ["read", "write", "admin"]) $ - assertFailure . T.unpack $ - "charts/nginz/values.yaml: 'oauth_scope: " - <> base - <> "' matches no scope in Wire.API.OAuth.OAuthScope at any tier, so no " - <> "OAuth token can ever satisfy it and every endpoint under that " - <> "location is closed to OAuth.\nEither fix the name, or add the scope." - -testScopesAgree :: Assertion -testScopesAgree = do - unless (Set.null actual) . assertFailure . T.unpack . T.unlines $ - [ "OAuth scope declarations are out of sync.", - "", - "Columns: version, method, path, accepted by nginz, documented in swagger.", - "'-' means no scope. A finding means those last two disagree:", - "", - " enforced but not documented charts/nginz/values.yaml requires a scope the", - " swagger docs do not mention -- most likely a", - " missing DescriptionOAuthScope in the routing", - " table, e.g. on a newly added version of an", - " endpoint that already had one.", - " documented but not enforced the swagger docs promise a scope nginz does not", - " require -- a stale annotation, or a missing", - " oauth_scope: in charts/nginz/values.yaml.", - "" - ] - <> section "deviations:" actual - where - actual = Set.fromList (renderFinding <$> findings) - section title xs - | Set.null xs = [] - | otherwise = [" " <> title] <> ((" " <>) <$> Set.toAscList xs) <> [""] +-- | The documented scope has to be one of the scopes that actually get a token +-- through. If no token gets through at all, there is nothing to document. +scopesMatch :: + -- | required + Set OAuthScope -> + -- | documented + Maybe OAuthScope -> + Bool +scopesMatch enforced = maybe (Set.null enforced) (`Set.member` enforced) diff --git a/services/brig/src/Brig/API/OAuth.hs b/services/brig/src/Brig/API/OAuth.hs index b84df3d0a62..0e1c0013292 100644 --- a/services/brig/src/Brig/API/OAuth.hs +++ b/services/brig/src/Brig/API/OAuth.hs @@ -28,7 +28,6 @@ import Brig.API.Handler (Handler) import Brig.App import Brig.Options qualified as Opt import Cassandra hiding (Set) -import Cassandra qualified as C import Control.Error import Control.Lens ((?~), (^?)) import Crypto.JWT hiding (params, uri) @@ -39,7 +38,6 @@ import Data.Json.Util (toUTCTimeMillis) import Data.Map qualified as Map import Data.Misc import Data.Qualified -import Data.Set qualified as Set import Data.Text.Ascii import Data.Text.Encoding qualified as T import Data.Time @@ -425,10 +423,9 @@ lookupOauthClient cid = do insertOAuthAuthorizationCode :: (MonadClient m) => Word64 -> OAuthAuthorizationCode -> OAuthClientId -> UserId -> OAuthScopes -> RedirectUrl -> OAuthCodeChallenge -> m () insertOAuthAuthorizationCode ttl code cid uid scope uri chal = do - let cqlScope = C.Set (Set.toList (unOAuthScopes scope)) - retry x5 . write q $ params LocalQuorum (code, cid, uid, cqlScope, uri, chal, fromIntegral ttl) + retry x5 . write q $ params LocalQuorum (code, cid, uid, scope, uri, chal, fromIntegral ttl) where - q :: PrepQuery W (OAuthAuthorizationCode, OAuthClientId, UserId, C.Set OAuthScope, RedirectUrl, OAuthCodeChallenge, Int32) () + q :: PrepQuery W (OAuthAuthorizationCode, OAuthClientId, UserId, OAuthScopes, RedirectUrl, OAuthCodeChallenge, Int32) () q = fromString $ "INSERT INTO oauth_auth_code (code, client, user, scope, redirect_uri, code_challenge) VALUES (?, ?, ?, ?, ?, ?) USING TTL ?" lookupAndDeleteByOAuthAuthorizationCode :: (MonadClient m) => OAuthAuthorizationCode -> m (Maybe (OAuthClientId, UserId, OAuthScopes, RedirectUrl, Maybe OAuthCodeChallenge)) @@ -436,10 +433,9 @@ lookupAndDeleteByOAuthAuthorizationCode code = lookupOAuthAuthorizationCode <* d where lookupOAuthAuthorizationCode :: (MonadClient m) => m (Maybe (OAuthClientId, UserId, OAuthScopes, RedirectUrl, Maybe OAuthCodeChallenge)) lookupOAuthAuthorizationCode = do - mTuple <- retry x5 . query1 q $ params LocalQuorum (Identity code) - pure $ mTuple <&> \(cid, uid, C.Set scope, uri, mChal) -> (cid, uid, OAuthScopes (Set.fromList scope), uri, mChal) + retry x5 . query1 q $ params LocalQuorum (Identity code) where - q :: PrepQuery R (Identity OAuthAuthorizationCode) (OAuthClientId, UserId, C.Set OAuthScope, RedirectUrl, Maybe OAuthCodeChallenge) + q :: PrepQuery R (Identity OAuthAuthorizationCode) (OAuthClientId, UserId, OAuthScopes, RedirectUrl, Maybe OAuthCodeChallenge) q = "SELECT client, user, scope, redirect_uri, code_challenge FROM oauth_auth_code WHERE code = ?" deleteOAuthAuthorizationCode :: (MonadClient m) => m () @@ -454,9 +450,9 @@ insertOAuthRefreshToken maxActiveTokens ttl info = do oldTokes <- determineOldestTokensToBeDeleted <$> lookupOAuthRefreshTokens info.userId for_ oldTokes (\t -> deleteOAuthRefreshToken t.userId t.refreshTokenId) retry x5 . write qInsertId $ params LocalQuorum (info.userId, rid, fromIntegral ttl) - retry x5 . write qInsertInfo $ params LocalQuorum (rid, info.clientId, info.userId, C.Set (Set.toList (unOAuthScopes info.scopes)), info.createdAt, fromIntegral ttl) + retry x5 . write qInsertInfo $ params LocalQuorum (rid, info.clientId, info.userId, info.scopes, info.createdAt, fromIntegral ttl) where - qInsertInfo :: PrepQuery W (OAuthRefreshTokenId, OAuthClientId, UserId, C.Set OAuthScope, UTCTime, Int32) () + qInsertInfo :: PrepQuery W (OAuthRefreshTokenId, OAuthClientId, UserId, OAuthScopes, UTCTime, Int32) () qInsertInfo = fromString $ "INSERT INTO oauth_refresh_token (id, client, user, scope, created_at) VALUES (?, ?, ?, ?, ?) USING TTL ?" qInsertId :: PrepQuery W (UserId, OAuthRefreshTokenId, Int32) () @@ -479,9 +475,9 @@ lookupOAuthRefreshTokens uid = do lookupOAuthRefreshTokenInfo :: (MonadClient m) => OAuthRefreshTokenId -> m (Maybe OAuthRefreshTokenInfo) lookupOAuthRefreshTokenInfo rid = do mTuple <- retry x5 . query1 q $ params LocalQuorum (Identity rid) - pure $ mTuple <&> \(cid, uid, C.Set scope, createdAt) -> OAuthRefreshTokenInfo rid cid uid (OAuthScopes (Set.fromList scope)) createdAt + pure $ mTuple <&> \(cid, uid, scopes, createdAt) -> OAuthRefreshTokenInfo rid cid uid scopes createdAt where - q :: PrepQuery R (Identity OAuthRefreshTokenId) (OAuthClientId, UserId, C.Set OAuthScope, UTCTime) + q :: PrepQuery R (Identity OAuthRefreshTokenId) (OAuthClientId, UserId, OAuthScopes, UTCTime) q = "SELECT client, user, scope, created_at FROM oauth_refresh_token WHERE id = ?" deleteOAuthRefreshToken :: (MonadClient m) => UserId -> OAuthRefreshTokenId -> m () diff --git a/services/brig/test/integration/API/OAuth.hs b/services/brig/test/integration/API/OAuth.hs index 7aa0f85d14f..97add951ca6 100644 --- a/services/brig/test/integration/API/OAuth.hs +++ b/services/brig/test/integration/API/OAuth.hs @@ -82,7 +82,12 @@ import Wire.API.User.Auth (CookieType (PersistentCookie)) import Wire.Sem.Jwk (readJwk) tests :: Manager -> C.ClientState -> Brig -> Nginz -> Opts -> TestTree -tests m db b n o = do +tests m db b nginz o = do + -- nginz forwards the request's Host as Z-Host and the services parse that as + -- a Domain. The test config contacts nginz by IP, which is not a Domain, so + -- send the local domain as Host. (Cf. 'rawBaseNginzRequest' in the new + -- integration suite.) + let n = nginz . header "Host" (cs (domainText o.settings.federationDomain)) testGroup "oauth" [ test m "register new oauth client" $ testRegisterNewOAuthClient b, @@ -122,9 +127,9 @@ tests m db b n o = do ], testGroup "accessing resources (only testing happy path to ensure scopes are valid)" - [ test m "write:conversations" $ testWriteConversationsSuccessNginz b n, + [ test m "write-only:conversations" $ testWriteConversationsSuccessNginz b n, test m "read:feature_configs" $ testReadFeatureConfigsSuccessNginz b n, - test m "write:conversations_code" $ testWriteConversationsCodeSuccessNginz b n + test m "write-only:conversations_code" $ testWriteConversationsCodeSuccessNginz b n ], testGroup "refresh tokens" @@ -159,7 +164,7 @@ testCreateOAuthCodeSuccess brig = do let newOAuthClient@(OAuthClientConfig _ redirectUrl) = newOAuthClientRequestBody "E Corp" "https://example.com" c <- registerNewOAuthClient brig newOAuthClient uid <- randomId - let scope = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scope = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] state <- UUID.toText <$> liftIO nextRandom createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest c.clientId scope OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do @@ -179,7 +184,7 @@ testCreateOAuthCodeRedirectUrlMismatch brig = do uid <- randomId state <- UUID.toText <$> liftIO nextRandom let differentUrl = mkUrl "https://wire.com" - createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest c.clientId mempty OAuthResponseTypeCode differentUrl state S256 challenge) !!! do + createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest c.clientId (OAuthScopes $ Set.singleton ReadSelf) OAuthResponseTypeCode differentUrl state S256 challenge) !!! do const 400 === statusCode const Nothing === (fmap getPath . getLocation) const (Just "redirect-url-miss-match") === fmap Error.label . responseJsonMaybe @@ -190,7 +195,7 @@ testCreateOAuthCodeClientNotFound brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" state <- UUID.toText <$> liftIO nextRandom - createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest cid mempty OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do + createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest cid (OAuthScopes $ Set.singleton ReadSelf) OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do const 404 === statusCode const (Just $ "access_denied") === (getLocation >=> getQueryParamValue "error") const (Just $ cs state) === (getLocation >=> getQueryParamValue "state") @@ -231,7 +236,7 @@ testCreateAccessTokenWrongClientId :: Brig -> Http () testCreateAccessTokenWrongClientId brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (_, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl cid <- randomId let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -243,7 +248,7 @@ testCreateAccessTokenWrongAuthorizationCode :: Brig -> Http () testCreateAccessTokenWrongAuthorizationCode brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, _) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let code = OAuthAuthorizationCode $ encodeBase16 "eb32eb9e2aa36c081c89067dddf81bce83c1c57e0b74cfb14c9f026f145f2b1f" let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -255,7 +260,7 @@ testCreateAccessTokenWrongUrl :: Brig -> Http () testCreateAccessTokenWrongUrl brig = do uid <- randomId let redirectUrl = mkUrl "https://wire.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let wrongUrl = mkUrl "https://example.com" let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code wrongUrl @@ -268,7 +273,7 @@ testCreateAccessTokenExpiredCode opts brig = withSettingsOverrides (opts & Opt.settingsLens . Opt.oAuthAuthorizationCodeExpirationTimeSecsInternalLens ?~ 1) $ do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl liftIO $ threadDelay (1 * 1200 * 1000) let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -280,7 +285,7 @@ testCreateAccessTokenWrongGrantType :: Brig -> Http () testCreateAccessTokenWrongGrantType brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeRefreshToken cid verifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! assertAccessDenied @@ -289,7 +294,7 @@ testCreateAccessTokenWrongCodeChallenge :: Brig -> Http () testCreateAccessTokenWrongCodeChallenge brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, code) <- generateOAuthClientAndAuthorizationCode' wrongCodeChallenge brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! do @@ -303,7 +308,7 @@ testCreateAccessTokenWrongCodeVerifier :: Brig -> Http () testCreateAccessTokenWrongCodeVerifier brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid wrongCodeVerifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! do @@ -327,7 +332,7 @@ testCreateCodeOAuthClientAccessDeniedWhenDisabled opts brig = uid <- randomId state <- UUID.toText <$> liftIO nextRandom let redirectUrl = mkUrl "https://example.com" - createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest cid mempty OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do + createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest cid (OAuthScopes $ Set.singleton ReadSelf) OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do const 403 === statusCode const (Just $ "access_denied") === (getLocation >=> getQueryParamValue "error") const (Just $ cs state) === (getLocation >=> getQueryParamValue "state") @@ -389,7 +394,7 @@ testAccessResourceInsufficientScope :: Brig -> Nginz -> Http () testAccessResourceInsufficientScope brig nginz = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -443,7 +448,7 @@ testRefreshTokenMaxActiveTokens opts db brig = uid <- randomId jwk <- liftIO $ readJwk (fromMaybe "path to jwk not set" opts.settings.oAuthJwkKeyPair) <&> fromMaybe (error "invalid key") let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] let delayOneSec = -- we have to wait ~1 sec before we create the next token, to make sure it is created with a different timestamp -- this is due to the interpreter of the `Now` effect which auto-updates every second @@ -645,7 +650,7 @@ testListApplicationsWithAccountAccess brig = do testWriteConversationsSuccessNginz :: Brig -> Nginz -> Http () testWriteConversationsSuccessNginz brig nginz = do (uid, tid) <- Team.createUserWithTeam brig - resp <- getAccessTokenForScope brig uid [WriteConversations] + resp <- getAccessTokenForScope brig uid [WriteOnlyConversations] createTeamConv nginz authHeader resp.accessToken tid "oauth test group" !!! do const 201 === statusCode @@ -659,7 +664,7 @@ testReadFeatureConfigsSuccessNginz brig nginz = do testWriteConversationsCodeSuccessNginz :: Brig -> Nginz -> Http () testWriteConversationsCodeSuccessNginz brig nginz = do (uid, tid) <- Team.createUserWithTeam brig - resp <- getAccessTokenForScope brig uid [WriteConversations, WriteConversationsCode] + resp <- getAccessTokenForScope brig uid [WriteOnlyConversations, WriteOnlyConversationsCode] conv <- responseJsonError @_ @(Conversation GroupConvType) =<< createTeamConv nginz authHeader resp.accessToken tid "oauth test group" UserId -> Http OAuthAccessTokenResponse createOAuthApplicationWithAccountAccess brig uid = do let redirectUrl = mkUrl "https://example.com" - (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid (OAuthScopes $ mempty) redirectUrl + (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid (OAuthScopes $ Set.singleton ReadSelf) redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl createOAuthAccessToken brig accessTokenRequest diff --git a/services/nginz/integration-test/conf/nginz/nginx.conf b/services/nginz/integration-test/conf/nginz/nginx.conf index a6e10ecf9ea..8ea7e27fa51 100644 --- a/services/nginz/integration-test/conf/nginz/nginx.conf +++ b/services/nginz/integration-test/conf/nginz/nginx.conf @@ -469,7 +469,13 @@ http { location ~* ^(/v[0-9]+)?/conversations/([^/]*)/code { include common_response_with_zauth.conf; - oauth_scope conversations_code; + # The only location where 'oauth_scopes' is used instead of the + # deprecated 'oauth_scope', so that one test run covers both + # paths through the nginz module. See + # 'testOAuthScopeTiersAreSeparate' and + # 'testOAuthNewScopesOnDeprecatedAttribute' in + # integration/test/Test/OAuth.hs. + oauth_scopes read:conversations_code write-only:conversations_code; proxy_pass http://galley; } diff --git a/services/nginz/third_party/nginx-zauth-module/zauth_module.c b/services/nginz/third_party/nginx-zauth-module/zauth_module.c index 27aeb02246f..98ac8ddb2c5 100644 --- a/services/nginz/third_party/nginx-zauth-module/zauth_module.c +++ b/services/nginz/third_party/nginx-zauth-module/zauth_module.c @@ -13,8 +13,9 @@ typedef struct { } ZauthServerConf; typedef struct { - ngx_flag_t zauth; // 1=on, 0=off - ngx_str_t oauth_scope; + ngx_flag_t zauth; // 1=on, 0=off + ngx_str_t oauth_scope; // scope base, tier implied by the method (deprecated) + ngx_str_t oauth_scopes; // whole scopes, separated by spaces; supersedes oauth_scope } ZauthLocationConf; enum { @@ -40,6 +41,7 @@ static char * merge_srv_conf (ngx_conf_t *, void *, void *); static char * load_keystore (ngx_conf_t *, ngx_command_t *, void *); static char * load_acl (ngx_conf_t *, ngx_command_t *, void *); static char * load_oauth_key (ngx_conf_t *, ngx_command_t *, void *); +static char * set_oauth_scopes(ngx_conf_t *, ngx_command_t *, void *); static void delete_srv_conf (void *); // Module setup @@ -69,7 +71,7 @@ static void zauth_empty_val (ngx_http_variable_value_t *); // Utility functions static ngx_int_t zauth_handle_request (ngx_http_request_t *, const ZauthServerConf *, ZauthToken const *); -static ngx_int_t oauth_handle_request(ngx_http_request_t *, OAuthPubJwk const *, ngx_str_t const); +static ngx_int_t oauth_handle_request(ngx_http_request_t *, OAuthPubJwk const *, ZauthLocationConf const *); static ngx_http_module_t zauth_module_ctx = { zauth_variables // pre-configuration @@ -99,6 +101,14 @@ static ngx_command_t zauth_commands [] = { , NULL } + , { ngx_string ("oauth_scopes") + , NGX_HTTP_LOC_CONF | NGX_CONF_1MORE + , set_oauth_scopes + , NGX_HTTP_LOC_CONF_OFFSET + , offsetof (ZauthLocationConf, oauth_scopes) + , NULL + } + , { ngx_string ("zauth_keystore") , NGX_HTTP_SRV_CONF | NGX_CONF_TAKE1 , load_keystore @@ -231,6 +241,46 @@ static char * merge_loc_conf (ngx_conf_t * _, void * pc, void * cc) { ZauthLocationConf * child = cc; ngx_conf_merge_off_value(child->zauth, parent->zauth, 1); ngx_conf_merge_str_value(child->oauth_scope, parent->oauth_scope, NULL); + if (child->oauth_scopes.data == NULL) { + child->oauth_scopes = parent->oauth_scopes; + } + return NGX_CONF_OK; +} + +// Join the arguments into one space separated string, which is how the scope +// claim of an OAuth token spells a list as well. +static char * set_oauth_scopes (ngx_conf_t * conf, ngx_command_t * cmd, void * data) { + ZauthLocationConf * lc = data; + + if (lc->oauth_scopes.data != NULL) { + return "is duplicate"; + } + + ngx_str_t * const args = conf->args->elts; + size_t len = conf->args->nelts - 2; // separators + + for (ngx_uint_t i = 1; i < conf->args->nelts; ++i) { + len += args[i].len; + } + + u_char * const buf = ngx_pnalloc(conf->pool, len); + + if (buf == NULL) { + return NGX_CONF_ERROR; + } + + u_char * p = buf; + + for (ngx_uint_t i = 1; i < conf->args->nelts; ++i) { + if (i > 1) { + *p++ = ' '; + } + p = ngx_cpymem(p, args[i].data, args[i].len); + } + + lc->oauth_scopes.data = buf; + lc->oauth_scopes.len = len; + return NGX_CONF_OK; } @@ -346,7 +396,7 @@ static ngx_int_t zauth_and_oauth_handle_request (ngx_http_request_t * r) { if (ctx != NULL && ctx->tag == CONTEXT_ZAUTH) { return zauth_handle_request(r, sc, ctx->token); } else if (ctx == NULL) { - return oauth_handle_request(r, sc->oauth_pub_key, lc->oauth_scope); + return oauth_handle_request(r, sc->oauth_pub_key, lc); } else { return NGX_HTTP_UNAUTHORIZED; } @@ -375,7 +425,7 @@ static ngx_int_t zauth_handle_request (ngx_http_request_t * r, const ZauthServer return NGX_OK; } -ngx_int_t oauth_handle_request(ngx_http_request_t *r, OAuthPubJwk const * key, ngx_str_t const scope) { +ngx_int_t oauth_handle_request(ngx_http_request_t *r, OAuthPubJwk const * key, ZauthLocationConf const * lc) { if (r->headers_in.authorization == NULL) { return NGX_HTTP_UNAUTHORIZED; } @@ -383,7 +433,14 @@ ngx_int_t oauth_handle_request(ngx_http_request_t *r, OAuthPubJwk const * key, n ngx_str_t hdr = r->headers_in.authorization->value; if (strncmp((char const *) hdr.data, "Bearer ", 7) == 0) { - OAuthResult res = oauth_verify_token(key, &hdr.data[7], hdr.len - 7, scope.data, scope.len, r->method_name.data, r->method_name.len); + // 'oauth_scopes' supersedes 'oauth_scope' where both are given. + // Where neither is, libzauth is handed a NULL scope and lets + // nobody in: no scope configured, no access through OAuth. + ngx_str_t const scopes = lc->oauth_scopes; + ngx_str_t const scope = lc->oauth_scope; + OAuthResult res = scopes.data != NULL + ? oauth_verify_token_scopes(key, &hdr.data[7], hdr.len - 7, scopes.data, scopes.len, r->method_name.data, r->method_name.len) + : oauth_verify_token(key, &hdr.data[7], hdr.len - 7, scope.data, scope.len, r->method_name.data, r->method_name.len); if (res.status == OAUTH_OK) { ZauthContext * ctx = alloc_oauth_context(r, res.uid); if (ctx == NULL) return NGX_HTTP_INTERNAL_SERVER_ERROR; // for OOM-safety