diff --git a/CHANGELOG.md b/CHANGELOG.md index 45c6d21..48e0e02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,24 @@ ## [Unreleased] - 2024 +### Changed + +#### Resource-server hardening + +- `Bearer` and `RequireScope` protect every path by default; `except:` lists open paths and `prefixes:` is now + an allowlist with a documented warning. Paths are percent-decoded, `//`-collapsed and dot-resolved before + matching (`Authly::Handler::PathFilter`); `/api/` covers `/api`; `prefixes: []` is refused +- A token store that fails (Redis down) gives 503 + `Retry-After` instead of 401; `Authly::VerificationUnavailable` +- Access tokens carry `aud` (`config.access_token_audience`, `base_url` by default) and `token_type`; + `StrategyTokenVerifier` requires `token_type: "access_token"` (`legacy_tokens: true` for older tokens) and + enforces `audience:`; refresh tokens carry `token_type: "refresh_token"` +- `StrategyTokenVerifier` honors the configuration it is given and decodes the token once +- `context.authly_claims!` raises `Authly::MissingClaims` +- JWKS: the algorithm must fit the key type; the endpoint sets `Cache-Control` only on success (`no-store` on errors) +- `Bearer`: `pass_preflight:` for CORS preflights, repeated `access_token` query values refused, `Cache-Control: private` and + `Referrer-Policy: no-referrer` on query-token responses +- `OpaqueToken#valid?` no longer turns store errors into "invalid"; `JWTToken#valid?` logs at debug, without token content + ### Added #### Resource Server Handlers diff --git a/README.md b/README.md index 5caa23b..0c29947 100644 --- a/README.md +++ b/README.md @@ -1092,13 +1092,14 @@ configuration and hands the verified claims to the handlers after it: require "authly" server = HTTP::Server.new([ - Authly::Handler::Bearer.new(realm: "api", prefixes: ["/api"]), + Authly::Handler::Bearer.new(realm: "api", except: ["/health"]), MyApp.new, ]) ``` -Downstream, the claims are `context.authly_claims` (an `Authly::Claims`, `nil` where the handler did -not apply). They are an instance variable of that request's own `HTTP::Server::Context`, so concurrent +`Bearer` protects **every** request except the paths in `except` (see below). Downstream, the claims are +`context.authly_claims` (an `Authly::Claims`, `nil` where the handler did not apply) or +`context.authly_claims!`, which raises `Authly::MissingClaims` instead of returning nil. They are an instance variable of that request's own `HTTP::Server::Context`, so concurrent requests never see each other's claims and nothing global has to be cleaned up: ```crystal @@ -1118,15 +1119,27 @@ end What is verified: the signature with the configured `algorithm` (the token's own `alg` header is never trusted, so `alg: none` and key-confusion tokens fail), `exp` (required), `nbf`, the issuer, and -revocation. Only **access tokens** pass: a refresh token is signed with the same key but has no `scope`, -and is refused. Both `:jwt` and `:opaque` token strategies work. To also require an audience: +revocation. Only **access tokens** pass: they carry `token_type: "access_token"`, and a refresh token (same +key, other type) is refused. Both `:jwt` and `:opaque` token strategies work. The token is decoded once per +request. Tokens issued before `token_type` existed (up to `access_ttl` old after an upgrade) are accepted +only with `StrategyTokenVerifier.new(legacy_tokens: true)`, for the length of that rollout. + +**Set an audience.** Access tokens carry `aud` (`config.access_token_audience`, `base_url` by default). A +resource server that does not check it accepts a token minted for *any* API that shares the key (a confused +deputy). Give each API its own audience and check it: ```crystal +# on the authorization server +config.access_token_audience = "https://api.example.com" + +# on the resource server verifier = Authly::StrategyTokenVerifier.new(audience: "https://api.example.com") -Authly::Handler::Bearer.new(verifier, realm: "api") +Authly::Handler::Bearer.new(verifier, realm: "api", except: ["/health"]) ``` -Tokens issued by Authly carry no `aud` claim, so an audience check only suits tokens you add one to. +Without `audience:` any `aud` is accepted. Authly issues one audience per server, not per request +(no RFC 8707 resource indicators), so use one authorization server per audience if APIs must be isolated. + Anything else (remote introspection, another key source) is an `Authly::TokenVerifier`: implement `verify(token : String) : Authly::Claims?` and pass it as the first argument. @@ -1138,13 +1151,28 @@ Failures follow RFC 6750 and never echo the token or log it: | Malformed header, two headers, empty token | 400 | `Bearer realm="api", error="invalid_request"` | | Bad, expired, revoked, wrong-key token | 401 | `Bearer realm="api", error="invalid_token"` | | Valid token without the required scope | 403 | `... error="insufficient_scope", scope="a b"` | +| Token store unreachable (Redis down) | 503 | none; `Retry-After: 1` | -The scheme is case-insensitive (`bearer`, `BEARER`). The token is read from the header only. Set -`allow_query_token: true` to also accept `?access_token=`, which RFC 6750 discourages because URLs end up -in logs; it is off by default, and a request with both a header and a query token is refused. +A store outage is a 503, not a 401: a 401 would send every client off to re-authenticate. It still fails +closed (nobody gets in). -`prefixes:` limits a handler to those paths (whole segments: `/api` covers `/api/users`, not `/apiary`), -so `/health` and the like can sit in the same chain. +The scheme is case-insensitive (`bearer`, `BEARER`); a tab is not a separator. The token is read from the +header only. Set `allow_query_token: true` to also accept `?access_token=` (one value only; a header as well +is refused). RFC 6750 discourages it: URLs end up in access logs, referrers and browser history, so +make sure your logs do not record query strings. Such responses get `Cache-Control: private` and +`Referrer-Policy: no-referrer`. + +**Which paths.** `except: ["/health"]` lists what stays open (it covers `/health/live` too, not `/healthz`); +everything else is protected, so a route added later is protected by default. `prefixes: ["/api"]` is the +opposite, an **allowlist: every path you do not list is open**. A forgotten route, a new top-level path or +a router that matches differently is then unprotected, so prefer `except`. Paths are compared after +percent-decoding, collapsing `//` and resolving `.`/`..`, so `/%61pi/x`, `//api/x` and `/x/../api/x` are +`/api/x`. `prefixes` ignores case (`/API/x` is protected by `/api`), `except` is exact (`/HEALTH` is not +skipped by `/health`). A trailing slash does not matter, and an empty `prefixes: []` is refused. + +**CORS.** Put the CORS handler **before** `Bearer`, or a browser cannot read the 401. A preflight +(`OPTIONS` with `Access-Control-Request-Method`) carries no credentials: either let the CORS handler +answer it, or give `Bearer` `pass_preflight: true` to hand it to the next handler. Off by default. ### Requiring Scopes @@ -1152,14 +1180,15 @@ so `/health` and the like can sit in the same chain. ```crystal HTTP::Server.new([ - Authly::Handler::Bearer.new, + Authly::Handler::Bearer.new(except: ["/health"]), Authly::Handler::RequireScope.all("read:users", prefixes: ["/users"]), Authly::Handler::RequireScope.any("admin", "root", prefixes: ["/admin"]), MyApp.new, ]) ``` -`RequireScope.new(all: [...], any: [...])` needs both. A request that lacks a scope gets +`RequireScope.new(all: [...], any: [...])` needs both. It takes `prefixes:` and `except:` like `Bearer` and applies to +every path unless narrowed, so repeat the `except` paths of `Bearer` if it is used without `prefixes`. A request that lacks a scope gets `403` with `error="insufficient_scope"` and the scopes it needs; a request that no `Bearer` authenticated gets `401`, it is never let through. Scope names and realms are checked when the handler is built, so a value cannot inject into the challenge header. @@ -1185,8 +1214,11 @@ end The key is published with `use`, `alg` and a `kid` (its RFC 7638 thumbprint). Authly does not put `kid` in the tokens it issues and publishes one key, so rotation (several keys at once) is not supported yet. -A private key, or anything that is not a PEM public key, is refused (the endpoint answers 500) instead of -being published. EdDSA, PS* and ES256K are not published. +The algorithm has to fit the key (RS* an RSA key, ES256/384/512 an EC key on P-256/P-384/P-521). A private key, a key that does not fit, or anything that is not a PEM public key, is refused (the endpoint answers 500 with `Cache-Control: no-store`) instead of +being published; a published set is `public, max-age=300`. EdDSA, PS* and ES256K are not published. + +The metadata's `issuer` is `base_url`, while tokens carry `iss` = `config.issuer`. Set both to the same +URL if clients validate the metadata issuer against the token. A resource server that is a separate process can verify the same tokens by configuring the public key (`algorithm` and `public_key`) and using `Bearer`; revocation is only seen where the token store is shared. @@ -1207,7 +1239,7 @@ app.router.get "/users", UsersEndpoint.new server = app.server([ *app.default_handlers, Authly::Handler.new, - Authly::Handler::Bearer.new(prefixes: ["/users"]), + Authly::Handler::Bearer.new(except: ["/health", "/oauth"]), Authly::Handler::RequireScope.all("read:users", prefixes: ["/users"]), ] of HTTP::Handler) ``` diff --git a/spec/bearer_hardening_spec.cr b/spec/bearer_hardening_spec.cr new file mode 100644 index 0000000..36ad739 --- /dev/null +++ b/spec/bearer_hardening_spec.cr @@ -0,0 +1,332 @@ +require "./spec_helper" +require "./support/bearer_helpers" + +class FailingStore + include Authly::TokenStore + + property? failing = false + + def initialize(@store = Authly::InMemoryStore.new) + end + + def store(token_id : String, payload) + @store.store(token_id, payload) + end + + def fetch(token_id : String) + guard + @store.fetch(token_id) + end + + def revoke(token_id : String) + @store.revoke(token_id) + end + + def revoked?(token_id : String) : Bool + guard + @store.revoked?(token_id) + end + + def valid?(token_id : String) : Bool + !revoked?(token_id) + end + + private def guard + raise IO::Error.new("connection refused") if failing? + end +end + +module HardeningSpec + extend self + + def with_config(&) + config = Authly.config + previous = {config.token_strategy, config.token_store, config.access_token_audience} + begin + yield config + ensure + config.token_strategy, config.token_store, config.access_token_audience = previous + end + end + + def server(bearer : Authly::Handler::Bearer, &) + BearerSpec.with_server([bearer, ClaimsEcho.new] of HTTP::Handler) { |client| yield client } + end +end + +describe Authly::Handler::PathFilter do + guarded = ->(filter : Authly::Handler::PathFilter, path : String) { filter.protects?(path) } + + describe "protecting everything but a list" do + filter = Authly::Handler::PathFilter.new(except: ["/health", "/static/"]) + + it "lets the listed paths and their subpaths through, however they are spelled" do + ["/health", "/health/", "/health/live", "/static", "/static/app.css", "//health", "/./health", "/x/../health", "/%68ealth"].each do |path| + {path, guarded.call(filter, path)}.should eq({path, false}) + end + end + + it "protects everything else, including look-alikes and other cases" do + ["/", "/healthz", "/HEALTH", "/api", "/health/../api", "/health/%2e%2e/api", "/health%2f..%2fapi", "/staticfiles"].each do |path| + {path, guarded.call(filter, path)}.should eq({path, true}) + end + end + end + + describe "protecting an allowlist" do + filter = Authly::Handler::PathFilter.new(prefixes: ["/api/"]) + + it "protects the prefix however the path is spelled" do + ["/api", "/api/", "/api/x/", "//api", "//api/x", "/API", "/Api/x", "/%61pi/x", "/x/../api/x", "/x/%2e%2e/api/x", "/api%2fx", "/api\\x"].each do |path| + {path, guarded.call(filter, path)}.should eq({path, true}) + end + end + + it "leaves the rest open" do + ["/", "/apiary", "/other/api"].each do |path| + {path, guarded.call(filter, path)}.should eq({path, false}) + end + end + + it "applies except on top of the allowlist" do + both = Authly::Handler::PathFilter.new(prefixes: ["/api"], except: ["/api/health"]) + + both.protects?("/api/users").should be_true + both.protects?("/api/health").should be_false + end + end + + it "refuses an allowlist that is empty" do + expect_raises(ArgumentError) { Authly::Handler::PathFilter.new(prefixes: [] of String) } + end +end + +describe "Bearer path handling over the wire" do + it "protects an allowlisted prefix however the request spells it" do + handlers = [Authly::Handler::Bearer.new(prefixes: ["/api/"]), ClaimsEcho.new] of HTTP::Handler + BearerSpec.with_server(handlers) do |client| + ["/api", "/api/", "/API/x", "/%61pi/x", "/x/../api/x", "/x/%2e%2e/api/x"].each do |path| + {path, BearerSpec.get(client, path).status_code}.should eq({path, 401}) + end + BearerSpec.get(client, "/apiary").status_code.should eq 200 + end + end + + it "applies RequireScope to the same spellings" do + handlers = [Authly::Handler::Bearer.new, Authly::Handler::RequireScope.all("admin", prefixes: ["/admin"]), ClaimsEcho.new] of HTTP::Handler + BearerSpec.with_server(handlers) do |client| + token = BearerSpec.token("read:users") + ["/admin", "/ADMIN/x", "/%61dmin", "/x/../admin"].each do |path| + {path, BearerSpec.get(client, path, "Bearer #{token}").status_code}.should eq({path, 403}) + end + end + end +end + +describe "context.authly_claims!" do + it "raises a clear error when nothing authenticated the request" do + context = HTTP::Server::Context.new(HTTP::Request.new("GET", "/"), HTTP::Server::Response.new(IO::Memory.new)) + + expect_raises(Authly::MissingClaims, /Bearer has to run before/) { context.authly_claims! } + context.authly_claims = Authly::Claims.from({"sub" => "alice"}) + context.authly_claims!.subject.should eq "alice" + end +end + +describe "token store failures" do + it "answers 503 with Retry-After, not 401, while a JWT revocation store is down" do + HardeningSpec.with_config do |config| + store = FailingStore.new + config.token_strategy = :jwt + config.token_store = store + token = BearerSpec.token + + HardeningSpec.server(Authly::Handler::Bearer.new) do |client| + BearerSpec.get(client, "/", "Bearer #{token}").status_code.should eq 200 + + store.failing = true + response = BearerSpec.get(client, "/", "Bearer #{token}") + response.status_code.should eq 503 + response.headers["Retry-After"].should eq "1" + response.headers["Cache-Control"].should eq "no-store" + response.headers["WWW-Authenticate"]?.should be_nil + response.body.should_not contain(token) + + store.failing = false + BearerSpec.get(client, "/", "Bearer #{token}").status_code.should eq 200 + end + end + end + + it "answers 503 while an opaque token store is down, and still refuses a token it does not know" do + HardeningSpec.with_config do |config| + store = FailingStore.new + config.token_strategy = :opaque + config.token_store = store + token = BearerSpec.token + + HardeningSpec.server(Authly::Handler::Bearer.new) do |client| + store.failing = true + BearerSpec.get(client, "/", "Bearer #{token}").status_code.should eq 503 + + store.failing = false + BearerSpec.get(client, "/", "Bearer #{token}").status_code.should eq 200 + BearerSpec.get(client, "/", "Bearer #{"0" * 64}").status_code.should eq 401 + end + end + end + + it "lets the verifier raise instead of answering nil" do + HardeningSpec.with_config do |config| + store = FailingStore.new + config.token_store = store + token = BearerSpec.token + store.failing = true + + expect_raises(Authly::VerificationUnavailable) { Authly::StrategyTokenVerifier.new.verify(token) } + end + end + + it "does not take a malformed token for a store failure" do + HardeningSpec.with_config do |config| + store = FailingStore.new + config.token_store = store + store.failing = true + + Authly::StrategyTokenVerifier.new.verify("not.a.jwt").should be_nil + Authly::StrategyTokenVerifier.new.verify("a.b.%%%").should be_nil + end + end +end + +describe "access token audience and type" do + it "issues access tokens for the configured audience, base_url by default" do + HardeningSpec.with_config do |config| + claims = Authly::StrategyTokenVerifier.new.verify(BearerSpec.token).not_nil! + claims["aud"].as_s.should eq config.base_url + claims["token_type"].as_s.should eq "access_token" + + config.access_token_audience = "https://api.example.com" + Authly::StrategyTokenVerifier.new.verify(BearerSpec.token).not_nil!["aud"].as_s.should eq "https://api.example.com" + end + end + + it "makes a verifier with an audience refuse tokens meant for another API" do + HardeningSpec.with_config do |config| + config.access_token_audience = "https://billing.example.com" + for_billing = BearerSpec.token + + Authly::StrategyTokenVerifier.new(audience: "https://billing.example.com").verify(for_billing).should_not be_nil + Authly::StrategyTokenVerifier.new(audience: "https://reports.example.com").verify(for_billing).should be_nil + end + end + + it "refuses refresh tokens, and tokens of another type, even with the legacy flag" do + access = Authly::AccessToken.new("test_client", "read:users", subject: "alice") + other = BearerSpec.jwt(BearerSpec.payload({"token_type" => "id_token"})) + + [Authly::StrategyTokenVerifier.new, Authly::StrategyTokenVerifier.new(legacy_tokens: true)].each do |verifier| + verifier.verify(access.refresh_token).should be_nil + verifier.verify(other).should be_nil + end + end + + it "accepts a token issued before token_type existed only with the legacy flag" do + legacy = BearerSpec.jwt(BearerSpec.payload.reject("token_type")) + + Authly::StrategyTokenVerifier.new.verify(legacy).should be_nil + Authly::StrategyTokenVerifier.new(legacy_tokens: true).verify(legacy).should_not be_nil + end +end + +describe Authly::StrategyTokenVerifier do + it "uses the configuration it is given, not the global one" do + own = Authly::Configuration.new + own.secret_key = "another key entirely" + own.issuer = "https://other-issuer.example" + own.token_store = Authly::InMemoryStore.new + verifier = Authly::StrategyTokenVerifier.new(own) + + payload = BearerSpec.payload({"iss" => own.issuer, "token_type" => "access_token"}) + token = JWT.encode(payload, own.secret_key, own.algorithm) + verifier.verify(token).should_not be_nil + verifier.verify(BearerSpec.token).should be_nil + + own.token_store.revoke(payload["jti"].to_s) + verifier.verify(token).should be_nil + end + + it "follows a token store replaced at runtime" do + HardeningSpec.with_config do |config| + config.token_strategy = :opaque + config.token_store = Authly::InMemoryStore.new + verifier = Authly::StrategyTokenVerifier.new + + first = BearerSpec.token + verifier.verify(first).should_not be_nil + + config.token_store = Authly::InMemoryStore.new + verifier.verify(first).should be_nil + second = BearerSpec.token + verifier.verify(second).should_not be_nil + + config.token_store.revoke(second) + verifier.verify(second).should be_nil + end + end +end + +describe "Bearer request edge cases" do + it "does not take a tab for the separator after the scheme" do + BearerSpec.with_server(BearerSpec.chain) do |client| + BearerSpec.get(client, "/users", "Bearer\t#{BearerSpec.token}").status_code.should eq 401 + BearerSpec.get(client, "/users", "Bearer \t#{BearerSpec.token}").status_code.should eq 400 + end + end + + it "treats HEAD like GET" do + BearerSpec.with_server(BearerSpec.chain) do |client| + client.head("/users").status_code.should eq 401 + client.head("/users", HTTP::Headers{"Authorization" => "Bearer #{BearerSpec.token}"}).status_code.should eq 200 + end + end + + it "answers a CORS preflight with 401 unless told to let it through" do + preflight = HTTP::Headers{"Origin" => "https://app.example", "Access-Control-Request-Method" => "GET"} + BearerSpec.with_server(BearerSpec.chain) { |client| client.exec("OPTIONS", "/users", preflight).status_code.should eq 401 } + + HardeningSpec.server(Authly::Handler::Bearer.new(pass_preflight: true)) do |client| + client.exec("OPTIONS", "/", preflight).status_code.should eq 200 + client.exec("OPTIONS", "/").status_code.should eq 401 + client.get("/", preflight).status_code.should eq 401 + end + end + + it "needs any-of only when it is not empty" do + handlers = [Authly::Handler::Bearer.new, Authly::Handler::RequireScope.new(all: ["a"], any: [] of String), ClaimsEcho.new] of HTTP::Handler + BearerSpec.with_server(handlers) do |client| + BearerSpec.get(client, "/", "Bearer #{BearerSpec.token("a")}").status_code.should eq 200 + BearerSpec.get(client, "/", "Bearer #{BearerSpec.token("b")}").status_code.should eq 403 + end + end + + describe "query string tokens" do + it "refuses a repeated access_token" do + HardeningSpec.server(Authly::Handler::Bearer.new(allow_query_token: true)) do |client| + token = BearerSpec.token + client.get("/?access_token=#{token}&access_token=#{token}").status_code.should eq 400 + end + end + + it "keeps the response out of caches and referrers, only when a query token was sent" do + HardeningSpec.server(Authly::Handler::Bearer.new(allow_query_token: true)) do |client| + response = client.get("/?access_token=#{BearerSpec.token}") + response.headers["Cache-Control"].should eq "private" + response.headers["Referrer-Policy"].should eq "no-referrer" + + header_only = BearerSpec.get(client, "/", "Bearer #{BearerSpec.token}") + header_only.headers["Referrer-Policy"]?.should be_nil + end + end + end +end diff --git a/spec/bearer_spec.cr b/spec/bearer_spec.cr index f22c72a..d426495 100644 --- a/spec/bearer_spec.cr +++ b/spec/bearer_spec.cr @@ -11,22 +11,34 @@ describe Authly::Handler::Bearer do end end - it "leaves paths outside its prefixes alone, with no claims" do + it "protects every path except the ones listed" do BearerSpec.with_server(BearerSpec.chain) do |client| response = BearerSpec.get(client, "/public") - response.status_code.should eq 200 JSON.parse(response.body)["sub"].raw.should be_nil + + BearerSpec.get(client, "/public/deep/er").status_code.should eq 200 + BearerSpec.get(client, "/anything-else").status_code.should eq 401 end end - it "does not match a prefix inside a longer segment" do + it "does not let a listed path cover a longer segment" do BearerSpec.with_server(BearerSpec.chain) do |client| - BearerSpec.get(client, "/usersettings").status_code.should eq 200 + BearerSpec.get(client, "/publicity").status_code.should eq 401 BearerSpec.get(client, "/users/1").status_code.should eq 401 end end + it "keeps protecting after a request on a connection to a skipped path" do + BearerSpec.with_server(BearerSpec.chain) do |client| + BearerSpec.get(client, "/health").status_code.should eq 200 + BearerSpec.get(client, "/users").status_code.should eq 401 + BearerSpec.get(client, "/health").status_code.should eq 200 + BearerSpec.get(client, "/users", "Bearer #{BearerSpec.token}").status_code.should eq 200 + BearerSpec.get(client, "/users").status_code.should eq 401 + end + end + it "answers a request without credentials with a bare challenge" do BearerSpec.with_server(BearerSpec.chain) do |client| response = BearerSpec.get(client, "/users") diff --git a/spec/jwks_spec.cr b/spec/jwks_spec.cr index c4b3078..1d7fbed 100644 --- a/spec/jwks_spec.cr +++ b/spec/jwks_spec.cr @@ -98,6 +98,16 @@ describe Authly::JWKS do JWKSSpec.keys(JWT::Algorithm::RS256, pem).first["kid"].should eq "NzbLsXh8uDCcd-6MNwXF4W_7noWXFZAfHkxZsRGC9Xs" end + it "refuses to publish a key under an algorithm it does not fit" do + ec_p256 = JWKSSpec::EC["ES256"][1] + ec_p384 = JWKSSpec::EC["ES384"][1] + + expect_raises(ArgumentError) { JWKSSpec.keys(JWT::Algorithm::RS256, ec_p256) } + expect_raises(ArgumentError) { JWKSSpec.keys(JWT::Algorithm::ES256, JWKSSpec::RSA_PUBLIC) } + expect_raises(ArgumentError) { JWKSSpec.keys(JWT::Algorithm::ES256, ec_p384) } + expect_raises(ArgumentError) { JWKSSpec.keys(JWT::Algorithm::ES512, ec_p256) } + end + it "refuses a private key and anything that is not a PEM public key" do private_pem = OpenSSL::PKey::RSA.new(2048).to_pem @@ -143,6 +153,19 @@ describe "the well-known endpoints" do end end + it "mark a published key set cacheable and an error uncacheable" do + BearerSpec.with_server([Authly::Handler.new] of HTTP::Handler) do |client| + client.get("/.well-known/jwks.json").headers["Cache-Control"].should eq "public, max-age=300" + + private_pem = OpenSSL::PKey::RSA.new(2048).to_pem + JWKSSpec.with_key(JWT::Algorithm::RS256, private_pem, private_pem) do + failed = client.get("/.well-known/jwks.json") + failed.status_code.should eq 500 + failed.headers["Cache-Control"].should eq "no-store" + end + end + end + it "only answer GET" do BearerSpec.with_server([Authly::Handler.new] of HTTP::Handler) do |client| client.post("/.well-known/jwks.json").status_code.should eq 404 diff --git a/spec/support/bearer_helpers.cr b/spec/support/bearer_helpers.cr index 178fca2..499d333 100644 --- a/spec/support/bearer_helpers.cr +++ b/spec/support/bearer_helpers.cr @@ -8,6 +8,8 @@ class ClaimsEcho sleep 50.milliseconds if context.request.path == "/slow" claims = context.authly_claims context.response.content_type = "application/json" + return if context.request.method == "HEAD" + context.response.print({ sub: claims.try(&.subject), scopes: claims.try(&.scopes), @@ -28,13 +30,14 @@ module BearerSpec def payload(overrides = {} of String => JSON::Any::Type) base = { - "sub" => "alice", - "iss" => Authly.config.issuer, - "cid" => "test_client", - "iat" => Time.utc.to_unix, - "exp" => 1.hour.from_now.to_unix, - "scope" => "read:users", - "jti" => Random::Secure.hex(8), + "sub" => "alice", + "iss" => Authly.config.issuer, + "cid" => "test_client", + "iat" => Time.utc.to_unix, + "exp" => 1.hour.from_now.to_unix, + "scope" => "read:users", + "token_type" => "access_token", + "jti" => Random::Secure.hex(8), } of String => JSON::Any::Type base.merge(overrides) end @@ -57,7 +60,7 @@ module BearerSpec def chain : Array(HTTP::Handler) [ - Authly::Handler::Bearer.new(realm: "api", prefixes: ["/users", "/admin", "/slow"]), + Authly::Handler::Bearer.new(realm: "api", except: ["/public", "/health"]), Authly::Handler::RequireScope.all("read:users", realm: "api", prefixes: ["/users"]), Authly::Handler::RequireScope.any("admin", "root", realm: "api", prefixes: ["/admin"]), ClaimsEcho.new, diff --git a/src/authly/access_token.cr b/src/authly/access_token.cr index 445fc5b..312cea9 100644 --- a/src/authly/access_token.cr +++ b/src/authly/access_token.cr @@ -49,6 +49,7 @@ module Authly access_payload["iat"] = Time.utc.to_unix access_payload["exp"] = expires_at.to_unix access_payload["token_type"] = "access_token" + access_payload["aud"] = Authly.config.token_audience access_payload["jti"] = @jti # Add certificate thumbprint if provided diff --git a/src/authly/configuration.cr b/src/authly/configuration.cr index 4df6c57..8520df3 100644 --- a/src/authly/configuration.cr +++ b/src/authly/configuration.cr @@ -17,6 +17,14 @@ module Authly property state_store : StateStore = InMemoryStateStore.new property device_code_store : DeviceCodeStore = InMemoryDeviceCodeStore.new + # The `aud` of the access tokens issued, i.e. which API they are for. Defaults to `base_url`. + # A resource server that sets the same `audience` on its verifier refuses tokens meant for others. + property access_token_audience : String? = nil + + def token_audience : String + access_token_audience || base_url + end + def public_key : String @public_key || secret_key end diff --git a/src/authly/context_claims.cr b/src/authly/context_claims.cr index 4ff05ba..6bde264 100644 --- a/src/authly/context_claims.cr +++ b/src/authly/context_claims.cr @@ -1,9 +1,23 @@ require "http/server" +module Authly + # Raised by `context.authly_claims!` when no verified claims are there. + class MissingClaims < Exception + def initialize + super("No verified claims: Authly::Handler::Bearer has to run before this handler and cover this path") + end + end +end + # The verified claims of the request, set by `Authly::Handler::Bearer`. # # They live in an instance variable of the request's own context, so concurrent requests (one # fiber each) never share them and nothing global has to be cleaned up when the request ends. class HTTP::Server::Context property authly_claims : Authly::Claims? + + # The claims, or `Authly::MissingClaims` when `Bearer` did not authenticate this request. + def authly_claims! : Authly::Claims + authly_claims || raise Authly::MissingClaims.new + end end diff --git a/src/authly/handler/bearer.cr b/src/authly/handler/bearer.cr index 0b42c36..97a2451 100644 --- a/src/authly/handler/bearer.cr +++ b/src/authly/handler/bearer.cr @@ -1,28 +1,45 @@ require "http/server" require "./challenge" +require "./path_filter" module Authly class Handler # Protects the routes behind it with OAuth 2.0 bearer tokens (RFC 6750). It is a plain # `HTTP::Handler`: it needs no web framework, so it goes into any handler chain. # + # It protects **every** request unless told otherwise; list what is public with `except`: + # # ``` # verifier = Authly::StrategyTokenVerifier.new(audience: "https://api.example.com") - # bearer = Authly::Handler::Bearer.new(verifier, realm: "api") + # bearer = Authly::Handler::Bearer.new(verifier, realm: "api", except: ["/health"]) # HTTP::Server.new([bearer, Authly::Handler::RequireScope.all("read:users"), app]) # ``` # + # `prefixes:` turns that around into an allowlist (only those paths are protected, every other + # path is open), which fails open for any route you forget to list: prefer `except`. See `PathFilter` + # for how paths are compared. + # + # Set an `audience` on the verifier (and `config.access_token_audience` on the issuer): without + # it a token minted for one API is accepted by every other API that shares the key. + # # A verified token's claims are available downstream as `context.authly_claims`. Anything else # ends the request here: no credentials 401, a malformed header 400 `invalid_request`, a bad, - # expired, revoked or wrongly addressed token 401 `invalid_token`. The token is read from the - # `Authorization` header only, unless `allow_query_token` is set (RFC 6750 section 2.3 discourages - # `?access_token=`: URLs end up in logs), and it is never logged. + # expired, revoked or wrongly addressed token 401 `invalid_token`, and a token store that cannot + # be reached 503 with `Retry-After` (not 401, which would make clients re-authenticate for + # nothing). The token is read from the `Authorization` header only, unless `allow_query_token` is + # set (RFC 6750 section 2.3 discourages `?access_token=`: URLs end up in logs, referrers and + # browser history; such responses get `Cache-Control: private` and `Referrer-Policy: no-referrer`), + # and it is never logged. + # + # Put a CORS handler **before** this one, or browsers cannot read the 401. A CORS preflight + # (`OPTIONS` with `Access-Control-Request-Method`) carries no credentials; `pass_preflight: true` + # lets it through to the next handler instead of answering 401. class Bearer include HTTP::Handler - include PathPrefixes MAX_TOKEN_BYTES = 8192 TOKEN68 = /\A[A-Za-z0-9\-._~+\/]+=*\z/ + RETRY_AFTER = "1" getter realm : String @@ -30,14 +47,20 @@ module Authly @verifier : TokenVerifier = StrategyTokenVerifier.new, realm : String = "authly", @allow_query_token : Bool = false, - @prefixes : Array(String)? = nil, + prefixes : Array(String)? = nil, + except : Array(String) = [] of String, + @pass_preflight : Bool = false, ) @realm = Handler.header_token!(realm, "realm") + @paths = PathFilter.new(prefixes, except) end def call(context : HTTP::Server::Context) context.authly_claims = nil - return call_next(context) unless covers?(context.request.path) + return call_next(context) unless @paths.protects?(context.request.path) + return call_next(context) if @pass_preflight && preflight?(context.request) + + keep_query_token_private(context) if @allow_query_token case credential = credential_from(context.request) in Challenge @@ -47,6 +70,17 @@ module Authly end end + private def preflight?(request : HTTP::Request) : Bool + request.method == "OPTIONS" && request.headers.has_key?("Access-Control-Request-Method") + end + + private def keep_query_token_private(context : HTTP::Server::Context) : Nil + return unless context.request.query_params.has_key?("access_token") + + context.response.headers["Cache-Control"] = "private" + context.response.headers["Referrer-Policy"] = "no-referrer" + end + private def authenticate(context : HTTP::Server::Context, token : String) : Nil claims = token.bytesize <= MAX_TOKEN_BYTES ? @verifier.verify(token) : nil if claims @@ -55,18 +89,32 @@ module Authly else Challenge.invalid_token(@realm).write(context) end + rescue VerificationUnavailable + context.response.headers["Retry-After"] = RETRY_AFTER + context.response.headers["Cache-Control"] = "no-store" + ResponseHelper.write(context, 503, "application/json", {error: "temporarily_unavailable"}.to_json) end private def credential_from(request : HTTP::Request) : String | Challenge from_header = header_credential(request.headers.get?("Authorization")) return from_header if from_header.is_a?(Challenge) - from_query = @allow_query_token ? request.query_params["access_token"]? : nil + from_query = query_credential(request) + return from_query if from_query.is_a?(Challenge) return Challenge.invalid_request(@realm) if from_header && from_query from_header || from_query || Challenge.missing(@realm) end + private def query_credential(request : HTTP::Request) : (String | Challenge)? + return unless @allow_query_token + + values = request.query_params.fetch_all("access_token") + return if values.empty? + + values.size > 1 ? Challenge.invalid_request(@realm) : values.first + end + private def header_credential(values : Array(String)?) : (String | Challenge)? return if values.nil? return Challenge.invalid_request(@realm) if values.size > 1 diff --git a/src/authly/handler/challenge.cr b/src/authly/handler/challenge.cr index 2429696..4ce03f3 100644 --- a/src/authly/handler/challenge.cr +++ b/src/authly/handler/challenge.cr @@ -47,19 +47,6 @@ module Authly end end - # Which requests a handler applies to: all of them, or those under one of the path prefixes - # (matched on whole segments, so "/api" covers "/api" and "/api/users" but not "/apiary"). - module PathPrefixes - private def covers?(path : String) : Bool - prefixes = @prefixes - return true if prefixes.nil? - - prefixes.any? do |prefix| - path == prefix || path.starts_with?(prefix.ends_with?('/') ? prefix : "#{prefix}/") - end - end - end - # Header-safe: no quotes, backslashes, whitespace or control characters could split a challenge. # :nodoc: def self.header_token!(value : String, what : String) : String diff --git a/src/authly/handler/path_filter.cr b/src/authly/handler/path_filter.cr new file mode 100644 index 0000000..13a9676 --- /dev/null +++ b/src/authly/handler/path_filter.cr @@ -0,0 +1,55 @@ +module Authly + class Handler + # Which requests a handler applies to. By default all of them; `except` lists paths that + # are let through (a health check), `prefixes` narrows the handler to an allowlist. + # + # Paths are compared whole-segment, after percent-decoding, collapsing `//` and resolving `.` + # and `..`, so `/%61pi/x`, `//api/x` and `/x/../api/x` are all `/api/x`. A path that cannot + # be decoded is protected. `prefixes` match case-insensitively (`/API/x` is protected by + # `/api`: the safe direction for a router that ignores case), `except` matches exactly + # (`/HEALTH` is not skipped by `/health`). An entry covers its subpaths, and a trailing slash + # does not matter (`/api/` covers `/api`). + struct PathFilter + @prefixes : Array(String)? + @except : Array(String) + + def initialize(prefixes : Array(String)? = nil, except : Array(String) = [] of String) + raise ArgumentError.new("An empty prefixes list would protect nothing") if prefixes.try(&.empty?) + + @prefixes = prefixes.try(&.map { |prefix| PathFilter.entry(prefix).downcase }) + @except = except.map { |path| PathFilter.entry(path) } + end + + protected def self.entry(path : String) : String + normalize(path) || raise ArgumentError.new("Invalid path: #{path.inspect}") + end + + def protects?(path : String) : Bool + normalized = PathFilter.normalize(path) + return true if normalized.nil? + + listed?(normalized.downcase, @prefixes, true) && !listed?(normalized, @except, false) + end + + def self.normalize(path : String) : String? + segments = [] of String + URI.decode(path).tr("\\", "/").split('/') do |segment| + case segment + when "", "." then next + when ".." then segments.pop? + else segments << segment + end + end + "/#{segments.join('/')}" + rescue URI::Error + nil + end + + private def listed?(path : String, entries : Array(String)?, default : Bool) : Bool + return default if entries.nil? + + entries.any? { |entry| entry == "/" || path == entry || path.starts_with?("#{entry}/") } + end + end + end +end diff --git a/src/authly/handler/require_scope.cr b/src/authly/handler/require_scope.cr index 82b7e7e..de98a95 100644 --- a/src/authly/handler/require_scope.cr +++ b/src/authly/handler/require_scope.cr @@ -1,5 +1,6 @@ require "http/server" require "./challenge" +require "./path_filter" module Authly class Handler @@ -11,11 +12,13 @@ module Authly # ``` # users = Authly::Handler::RequireScope.all("read:users", prefixes: ["/users"]) # admin = Authly::Handler::RequireScope.any("admin", "root", prefixes: ["/admin"]) + # + # Applies to every path unless narrowed (`prefixes:` is an allowlist, `except:` lists open paths); + # see `PathFilter`. # HTTP::Server.new([bearer, users, admin, app]) # ``` class RequireScope include HTTP::Handler - include PathPrefixes getter realm : String @@ -23,24 +26,26 @@ module Authly @all : Array(String) = [] of String, @any : Array(String) = [] of String, realm : String = "authly", - @prefixes : Array(String)? = nil, + prefixes : Array(String)? = nil, + except : Array(String) = [] of String, ) raise ArgumentError.new("RequireScope needs at least one scope") if @all.empty? && @any.empty? (@all + @any).each { |scope| Handler.header_token!(scope, "scope") } @realm = Handler.header_token!(realm, "realm") + @paths = PathFilter.new(prefixes, except) end - def self.all(*scopes : String, realm : String = "authly", prefixes : Array(String)? = nil) : RequireScope - new(all: scopes.to_a, realm: realm, prefixes: prefixes) + def self.all(*scopes : String, realm : String = "authly", prefixes : Array(String)? = nil, except : Array(String) = [] of String) : RequireScope + new(all: scopes.to_a, realm: realm, prefixes: prefixes, except: except) end - def self.any(*scopes : String, realm : String = "authly", prefixes : Array(String)? = nil) : RequireScope - new(any: scopes.to_a, realm: realm, prefixes: prefixes) + def self.any(*scopes : String, realm : String = "authly", prefixes : Array(String)? = nil, except : Array(String) = [] of String) : RequireScope + new(any: scopes.to_a, realm: realm, prefixes: prefixes, except: except) end def call(context : HTTP::Server::Context) - return call_next(context) unless covers?(context.request.path) + return call_next(context) unless @paths.protects?(context.request.path) claims = context.authly_claims if claims.nil? diff --git a/src/authly/handlers/jwks_handler.cr b/src/authly/handlers/jwks_handler.cr index 098e719..98f0d5c 100644 --- a/src/authly/handlers/jwks_handler.cr +++ b/src/authly/handlers/jwks_handler.cr @@ -1,9 +1,11 @@ module Authly class JWKSHandler def self.handle(context) + body = JWKS.new.to_json context.response.headers["Cache-Control"] = "public, max-age=300" - ResponseHelper.write(context, 200, "application/jwk-set+json", JWKS.new.to_json) + ResponseHelper.write(context, 200, "application/jwk-set+json", body) rescue ArgumentError + context.response.headers["Cache-Control"] = "no-store" ResponseHelper.write(context, 500, "application/json", {error: "server_error", error_description: "The signing key cannot be published"}.to_json) end end diff --git a/src/authly/handlers/response_helper.cr b/src/authly/handlers/response_helper.cr index 8aa2280..8c21164 100644 --- a/src/authly/handlers/response_helper.cr +++ b/src/authly/handlers/response_helper.cr @@ -3,7 +3,9 @@ module Authly def self.write(context, status_code, content_type = "text/plain", body = "") context.response.status_code = status_code context.response.content_type = content_type - context.response.print body + # A HEAD response has no body: bytes written anyway are read as the start of the next response + # on a kept-alive connection. + context.response.print body unless context.request.method == "HEAD" end def self.redirect(context, location) diff --git a/src/authly/jwks.cr b/src/authly/jwks.cr index 6d19059..d6f2a16 100644 --- a/src/authly/jwks.cr +++ b/src/authly/jwks.cr @@ -87,11 +87,25 @@ module Authly def initialize(@members : Hash(String, String)) end + # The key must be of the kind the algorithm signs with: RS* an RSA key, ES256/384/512 an EC key + # on P-256/P-384/P-521. Publishing a key under an algorithm it cannot verify would mislead clients. # `kid` is the RFC 7638 thumbprint, so it follows the key. def to_jwk(algorithm : String) : Hash(String, String) + raise ArgumentError.new("The public key does not fit #{algorithm}") unless fits?(algorithm) + @members.merge({"use" => "sig", "alg" => algorithm, "kid" => thumbprint}) end + private def fits?(algorithm : String) : Bool + case algorithm + when .starts_with?("RS") then @members["kty"] == "RSA" + when "ES256" then @members["crv"]? == "P-256" + when "ES384" then @members["crv"]? == "P-384" + when "ES512" then @members["crv"]? == "P-521" + else false + end + end + private def thumbprint : String required = @members["kty"] == "RSA" ? {"e", "kty", "n"} : {"crv", "kty", "x", "y"} canonical = required.map { |name| "#{name.to_json}:#{@members[name].to_json}" }.join(',') diff --git a/src/authly/token_generator.cr b/src/authly/token_generator.cr index 846bd00..2e9ee77 100644 --- a/src/authly/token_generator.cr +++ b/src/authly/token_generator.cr @@ -21,13 +21,15 @@ module Authly def generate_access_token(client_id : String, scope : String, sub : String, jti : String, cert_thumbprint : String? = nil) : String payload = { - "sub" => sub, - "iss" => @config.issuer, - "cid" => client_id, - "iat" => Time.utc.to_unix, - "exp" => @config.access_ttl.from_now.to_unix, - "scope" => scope, - "jti" => jti, + "sub" => sub, + "iss" => @config.issuer, + "aud" => @config.token_audience, + "token_type" => "access_token", + "cid" => client_id, + "iat" => Time.utc.to_unix, + "exp" => @config.access_ttl.from_now.to_unix, + "scope" => scope, + "jti" => jti, } of String => (String | Int64 | Hash(String, String)) # Add certificate binding if thumbprint is provided @@ -44,12 +46,13 @@ module Authly def generate_refresh_token(client_id : String, jti : String, sub : String) : String Authly.jwt_encode({ - "jti" => jti, - "sub" => sub, - "name" => "refresh token", - "iat" => Time.utc.to_unix, - "iss" => @config.issuer, - "exp" => @config.refresh_ttl.from_now.to_unix, + "jti" => jti, + "sub" => sub, + "name" => "refresh token", + "token_type" => "refresh_token", + "iat" => Time.utc.to_unix, + "iss" => @config.issuer, + "exp" => @config.refresh_ttl.from_now.to_unix, }) end @@ -81,6 +84,7 @@ module Authly payload["iat"] = Time.utc.to_unix payload["exp"] = expires_at.to_unix payload["token_type"] = "access_token" + payload["aud"] = @config.token_audience # Add certificate thumbprint if provided if cert_thumbprint diff --git a/src/authly/token_manager.cr b/src/authly/token_manager.cr index d7b4e28..a182e82 100644 --- a/src/authly/token_manager.cr +++ b/src/authly/token_manager.cr @@ -45,7 +45,7 @@ module Authly exp = decoded_token["exp"].to_s.to_i64 Time.utc.to_unix < exp rescue e : JWT::DecodeError - Log.error { "Invalid token - #{e.message}" } + Log.debug { "Invalid token (#{e.class})" } false end @@ -106,7 +106,7 @@ module Authly exp_value = payload["exp"] exp = exp_value.is_a?(Int64) ? exp_value : exp_value.to_s.to_i64 Time.utc.to_unix < exp - rescue + rescue Error | KeyError | ArgumentError false end diff --git a/src/authly/token_verifier.cr b/src/authly/token_verifier.cr index d086f8b..5bb6663 100644 --- a/src/authly/token_verifier.cr +++ b/src/authly/token_verifier.cr @@ -1,26 +1,41 @@ module Authly + # Verification could not be carried out (the token store is down), as opposed to the token being + # wrong. Callers answer 503, not 401: a 401 sends every client off to re-authenticate. + class VerificationUnavailable < Exception + end + # Turns a presented bearer token into verified claims. Implement it to plug in another # verification scheme (a remote introspection call, a key set fetched from elsewhere). module TokenVerifier - # The claims of a valid access token, nil for anything else. Must not raise. + # The claims of a valid access token, nil for a token that is not one. Raises + # `VerificationUnavailable` when it cannot tell. abstract def verify(token : String) : Claims? end - # Verifies with Authly's own configuration: signature (the configured algorithm, never the - # token's `alg` header), `exp`, `nbf`, `iss` and the revocation store for JWTs, the token store for - # opaque tokens. Only access tokens pass: a refresh token is signed with the same key but carries - # no `scope`. + # Verifies with a `Configuration` (the global one by default), decoding the token once: + # signature (the configured algorithm, never the token's `alg` header), `exp`, `nbf`, `iss` and + # the revocation store for JWTs; the token store for opaque tokens. + # + # Only access tokens pass: `token_type` must be `access_token`. A refresh token is signed with the + # same key but is not one. Tokens issued before `token_type` existed are accepted only with + # `legacy_tokens: true` (they must still carry a `scope`), for the length of `access_ttl` after an + # upgrade. + # + # With an `audience`, the token's `aud` must contain it. Without one any audience passes, so a token + # minted for one API is accepted by every API sharing the key: set it. class StrategyTokenVerifier include TokenVerifier - def initialize(@config : Configuration = Authly.config, @audience : String? = nil) + ACCESS_TOKEN = "access_token" + + def initialize(@config : Configuration = Authly.config, @audience : String? = nil, @legacy_tokens : Bool = false) end def verify(token : String) : Claims? - return unless TokenManager.new(@config).valid?(token) - claims = load(token) claims if claims && access_token?(claims) && audience_accepted?(claims) + rescue e : VerificationUnavailable + raise e rescue e Log.debug { "Bearer token rejected (#{e.class})" } nil @@ -28,16 +43,49 @@ module Authly private def load(token : String) : Claims? case @config.token_strategy - when :jwt - payload, _header = Authly.jwt_decode(token) - Claims.from(payload.as_h) - when :opaque - @config.token_store.fetch(token).try { |payload| Claims.from(payload) } + when :jwt then load_jwt(token) + when :opaque then load_opaque(token) end end + private def load_jwt(token : String) : Claims? + payload, _header = JWT.decode(token, @config.public_key, @config.algorithm, iss: @config.issuer) + claims = Claims.from(payload.as_h) + claims if claims.id.try { |jti| !revoked?(jti) } + end + + private def load_opaque(token : String) : Claims? + return if revoked?(token) + + claims = Claims.from(stored(token)) + claims if (claims["exp"]?.try(&.as_i64?) || 0_i64) > Time.utc.to_unix + end + + # A store that fails is not a token that is wrong: say so, instead of answering "invalid". + private def revoked?(id : String) : Bool + @config.token_store.revoked?(id) + rescue e + raise unavailable(e) + end + + private def stored(token : String) + @config.token_store.fetch(token) + rescue e : Error + raise e + rescue e + raise unavailable(e) + end + + private def unavailable(cause : Exception) : VerificationUnavailable + Log.error { "Token store unavailable (#{cause.class})" } + VerificationUnavailable.new("Token store unavailable", cause) + end + private def access_token?(claims : Claims) : Bool - claims.subject && claims["scope"]?.try(&.as_s?) && claims["exp"]?.try(&.as_i64?) ? true : false + return false unless claims.subject && claims["scope"]?.try(&.as_s?) && claims["exp"]?.try(&.as_i64?) + + type = claims["token_type"]? + type ? type.as_s? == ACCESS_TOKEN : @legacy_tokens end private def audience_accepted?(claims : Claims) : Bool