add dpop (rfc 9449) support#1794
Open
gkemmey wants to merge 12 commits into
Open
Conversation
also includes a `doorkeeper:dpop` generator for installing the migration and the results of installing it in the dummy app. the migration indexes `dpop_jkt` to support a query like ```sql update oauth_access_tokens set revoked_at = now() where dpop_jkt = '...'; ``` in the event a client's dpop key is compromised.
adds configuration option to `force_dpop` on access token requests and an accompanying `force_dpop?` helper. adds configuration options for `dpop_signature_algorithms` and `dpop_iat_leeway`. considers dpop supported when the `access_token_model` has a `dpop_jkt` column. when supported, prepends `from_dpop_authorization` to the default `access_token_methods`.
…op proof header this introduces a model for working with dpop proofs so we can validate the dpop header and derive the public key thumbprint for either binding or token validation. additionally, this adds the same `jwt` gem the latest release (0.4.2) of `doorkeeper-jwt` [uses][] as a runtime dependency. we ensure it's present when `OAuth::DPoPProof` autoloads. at some point, it may make sense to ensure the DPoP binding can be stored directly in the token if `doorkeeper-jwt` is being used. [uses]: https://redirect.github.com/doorkeeper-gem/doorkeeper-jwt/blob/v0.4.2/doorkeeper-jwt.gemspec#L25
…equest` this centralizes request parameter handling in `BaseRequest` and updates the oauth request classes to forward a `parameters` keyword argument instead of a positional hash argument. this creates space to pass through other request details.
…uer` `BaseRequest` mixes in `Validations`, and `ClientCredentialsRequest` previously short-circuited that flow by overriding `valid?`. it's more correct, and better aligned with the public api `Validations` exposes, to override `error` and `validate` to involve the `issuer` and leave `valid?` unchanged. additionally, `Validations#valid?` now relies on the `error` reader method, instead of using `@error` directly. that creates space for including classes to override `error` and compose additional error sources. while there are no validations defined on `ClientCredentialsRequest` itself, this leaves room for them to exist and cooperate with the validations the `issuer` invokes (through its `validator`).
oauth extension error code defined in [rfc 9449, 12.2][] [rfc 9449, 12.2]: https://datatracker.ietf.org/doc/html/rfc9449#section-12.2
…word flows this builds an `OAuth::DPoPProof` from the request in `Server` and injects it into each `OAuth::XRequest` class. when the access token model supports dpop, each request validates the dpop proof when dpop is required or a proof is present; otherwise validation is skipped. when dpop is supported and a valid dpop proof is provided, the access token is bound to the proof key by persisting the jwk thumbprint as `dpop_jkt` on the token, and the token response returns `token_type=DPoP`. when dpop is not supported or a dpop proof is not provided, dpop proof validation is skipped and bearer tokens are issued. when dpop is supported and a provided proof is invalid, token requests are invalidated. when dpop is required (`force_dpop?`), token requests without a dpop proof are invalidated. note: the refresh token flow will be handled in a separate commit. note: the implicit flow is unsupported. there's [an expired internet draft][] that allows for passing a dpop proof via a query param, but it's not a part of rfc 9449. [an expired internet draft]: https://datatracker.ietf.org/doc/html/draft-jones-oauth-dpop-implicit-00
this injects an `OAuth::DPoPProof` into the `RefreshTokenRequest` and applies dpop binding / validation when the access token model supports dpop. when dpop is supported and a valid dpop proof is provided, refreshed access tokens are bound to the proof key by persisting the jwk thumbprint as `dpop_jkt` on the new token, and the token response returns `token_type=DPoP`. refreshing a token that already uses dpop is handled differently based on client type: - confidential clients can refresh without presenting a dpop proof, because client authentication effectively sender-constrains the refresh request. the new access token remains dpop-bound, preserving the existing `dpop_jkt`, while still allowing an authenticated refresh to rotate the binding when a new valid dpop proof is provided. - public clients must prove possession on refresh when the refresh token is already dpop-bound: the request must include a valid dpop proof and it must match the existing binding; missing proofs or key mismatches invalidate the request. when dpop is not supported, dpop proof validation is skipped and bearer tokens are issued. finally, this commit supports upgrading an unbound access token to a dpop-bound access token on refresh when a valid proof is provided. when dpop is required (`force_dpop?`), refresh requests that would otherwise mint an unbound access token (i.e. without a dpop proof) are invalidated.
…s token when the introspected token uses dpop, the response now includes a `cnf` object with the token's jwk thumbprint (`jkt`), per [rfc 9449, section 6.2][]. [rfc 9449, section 6.2]: https://datatracker.ietf.org/doc/html/rfc9449#name-jwk-thumbprint-confirmation-
…rom `OAuthToken` in anticipation of needing the extra authentication details, adds an `authenticate3` method that returns a triplet: the `access_token_method`, the `plaintext_token`, and the `access_token` record. the public `authenticate` method that just returns the `access_token` is preserved, and is implemented in terms of `authenticate3`. this is meant to loosely mirror ruby's Open3 apis.
…en rails and grape removes the grape-specific `doorkeeper_token` implementation and instead relies on the shared rails helpers (`Rails::Helpers` is included in `Grape::Helpers`). introduces `__doorkeeper_request__` which can be overridden to customize what gets passed into `OAuth::Token.authenticate`: rails returns its raw request, while grape wraps its request in `AuthorizationDecorator`.
…cted resources adds support to `doorkeeper_authorize!` for requiring dpop (`dpop: :required`). the helper-level hook forces dpop on a specific endpoint regardless of global configuration. when dpop is required -- either because `:from_dpop_authorization` is the only available access token method or via `dpop: :required` -- we only authenticate via the dpop scheme; we always validate the dpop proof and the token's binding to the proof's public key, per rfc 9449. when dpop is optional, behavior is driven by Doorkeeper's configuration: if the access token model reports dpop support (`dpop_supported?`), we accept either bearer or dpop authentication. if the request uses the dpop scheme, we require a valid proof and ensure the presented token is bound to that proof, and we reject downgrade attempts. if dpop is unsupported, we fall back to bearer behavior. for errors, this introduces `OAuth::InvalidDPoPProofResponse` for invalid / missing proofs, and adds an `invalid_dpop_key_binding` `invalid_token` variant for mismatched bindings. additionally, `www-authenticate` challenges are now dependent on the `access_token_method` and whether dpop was required or supported, so we pass those details through the error responses. note: rfc 6750 discourages including an error code in `www-authenticate` when the request includes no authentication information and rfc 9449 recommends the same for dpop challenges. doorkeeper historically returned Bearer errors in this case, so we keep that behavior for consistency while adding the additional dpop challenge.
Insecure Processing of Data (1)
More info on how to fix Insecure Processing of Data in Ruby. 👉 Go to the dashboard for detailed results. 📥 Happy? Share your feedback with us. |
Contributor
|
This seems to miss all dpop-nonce handling and any tracking of previously used |
Contributor
Author
|
@ThisIsMissEm, thanks for looking! 🙏 those are worthwhile improvements, no doubt, but i think they can be handled separately. as is, we still get some (imperfect) defense against a stolen access token being used outside the issuing oauth application. just to reiterate, the goal of the pr is minimal spec compliance. regarding replay, the rfc says servers "must only accept DPoP proofs for a limited time after their creation" (section 11.1). that's the baseline mitigation and this pr enforces a configurable totally agree that additional measures like server-provided nonces and tracking previously-seen |
|
For those of us that are using doorkeeper-jwt and validating tokens at the resource server, we would have to handle tracking used So I would love to see this get included. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
summary
adds a minimally spec-compliant implementation of oauth 2.0 demonstrating proof of possession (dpop).
closes #1655
overview
dpop is a sender-constraining mechanism that binds access tokens to a client's cryptographic key pair. unlike bearer tokens, a dpop-bound token can't be used by an attacker who intercepts it -- they'd also need the private key that corresponds to the public key presented when the token was issued.
this covers both halves of doorkeeper: issuing dpop-bound access tokens (authorization server) and enforcing the key binding when authenticating (resource server).
the feature is fully opt-in. nothing changes for existing installations unless you run
rails generate doorkeeper:dpopand migrate.how it works
on the authorization server side, when a token request includes a
DPoPheader orforce_dpop?is configured, the proof is validated and the resulting access token is bound to the public key via its jwk sha-256 thumbprint.on the resource server side, when a dpop-bound token is presented or required (by either configuration or the endpoint), the accompanying proof must be valid and its key must match the one the token was originally bound to.
refresh token flow
the refresh token flow follows the spec's distinction between confidential and public clients.
a confidential client that refreshes a dpop-bound token can optionally include a new dpop proof:
a public client must always include a valid dpop proof whose key matches the original binding.
force_dpopvsaccess_token_methods == %i[from_dpop_authorization]when doorkeeper is acting as an authorization server,
force_dpoprequires every token request to include a valid dpop proof and ensures the minted access token is bound to that proof's public key.when acting as a resource server, dpop is considered required if
from_dpop_authorizationis the only configured access token method, or if the endpoint has explicitly required it viadoorkeeper_authorize!(dpop: :required).www-authenticateheadersthe error response
www-authenticateheaders are context-sensitive. when dpop is required, only the dpop scheme is advertised. when both bearer and dpop are supported, the header includes both schemes along with the supported algorithms. when a specific access token method was used to authenticate, the header reflects that method. this follows the guidance in the spec around telling clients what the server actually accepts.other information
optional specifications this skips
this intentionally omits a few optional parts of the spec:
jtitracking to prevent dpop proof replays (section 11.1)all of these would be reasonable follow-ups but aren't required for a baseline implementation, and the first two each introduce storage / coordination concerns that deserve their own consideration.
stores
dpop_jkton theoauth_access_tokenstablethe spec describes storing the bound public key in the token itself when the format allows for it (i.e., it's a jwt). however, doorkeeper doesn't natively issue structured tokens, and storing the thumbprint in the database is in keeping with the surrounding gem's approach.
the spec allows for other association methods in section 6, but doesn't describe any beyond the jwt-based approach. that said, at some point it may make sense to allow the dpop binding to be stored directly in the token if
doorkeeper-jwtis being used.jwtdependencythis adds the
jwtgem as a runtime dependency (not in the gemspec, required when the dpop proof class loads) to validate and decode dpop proofs.