smallrye / smallrye-jwt

Apache License 2.0
75 stars 48 forks source link

Add DPoP proofing mechanism #165

Open acoburn opened 4 years ago

acoburn commented 4 years ago

Now that custom token schemes are supported, this issue is to discuss adding a DPoP verifier to the smallrye-jwt code. We could also discuss implementing this as a quarkus extension, but I'd like to propose implementing the verifier at this layer.

The approach that I'd like to propose would be implemented as a i.s.j.auth.jaxrs.DPoPAuthenticationFilter class. This filter would have a @Priority(AUTHENTICATION) and perform the following validation steps:

  1. Extract a JWT from the DPoP header, if one exists
  2. Parse that token and verify that the embedded public key can be used to verify its signature
  3. Verify that the htm (HTTP Method) claim matches the method from the JAX-RS request context
  4. Verify that the htu (HTTP URI) claim matches the URI from the JAX-RS request context
  5. Verify (this is the crucial step) that the thumbprint of the public key from the DPoP header matches the thumbprint in the cnf (confirmation) claim of the access token (the injected JsonWebToken)

The DPoP draft specification does not define how to handle error conditions, should validation fail, but I would propose returning a 401 Unauthorized, since that's what this library already returns when a token can't be validated.

The Proof-of-Possession Key Semantics specification defines several ways that an access token can represent confirmation claims, including:

  1. Thumprint: "cnf": { "jkt": "SHA-256 thumbprint" }
  2. Embedded public key: "cnf": { "jwk": { ... } }
  3. The location of a public key: "cnf": { "jku": "URL of JWKS", "kid": "key-identifier" }
  4. Key ID reference: "cnf": { "kid": "key-identifier" }

I would like to propose supporting only the first two options. One of the nice features of DPoP is that it is self-contained and makes use of ephemeral keys, and the third option conflicts with both of those features. The fourth option is complicated because it would require that resource servers have extra out-of-band knowledge and it has questionable practicality in this context.

One part of this that could use some discussion relates to how to prevent against downgrade attacks. Since it would be important to continue supporting regular Bearer tokens in addition to DPoP, one should consider the case where an Authorization: DPoP <token> header gets replayed as Authorization: Bearer <token> without the added DPoP header. Typically, one would want to reject this request, but this assumes that the cnf claim is not being used for any other purpose, and I'm not sure that's a safe assumption in the general case. I suspect the simplest way to address this is the following:

By default, DPoP proofing is entirely disabled, but it can be enabled via an MP-config value (e.g. smallrye.jwt.enable.dpop). If enabled and if the access token contains a cnf claim, then the DPoP filter becomes relevant. At that point, if the token scheme is Bearer, the request can be denied. But if the access token does not contain a cnf claim, then the token is treated as a simple bearer token and DPoP validation doesn't come into play at all.

If this seems like a reasonable approach, I can implement it.

sberyozkin commented 4 years ago

@acoburn Hi, thanks for this proposal. FYI, these smallrye-jwt JAX-RS filters are currently not used in Quarkus/Thorntail, ex, quarkus-smallrye-jwt does not have any JAX-RS dependencies. However, indeed lets talk here, as having a filter at this level would be good. In fact we can have a JAX-RS filter with the bulk of the DPoP code being in the abstract class, which the authentication mechanisms in Quarkus (in quarkus-smallrye-jwt and quarkus-oidc/BearerAuthenticationMechanism), WildFly, etc could extend independently.

Please give me some time before I will comment further. Cheers CC @darranl @MikeEdgar

sberyozkin commented 4 years ago

@acoburn I've read the description, it is very clearly typed, and makes sense IMHO. The option 3 may also make sense because MP JWT allows to support HTTPS based JWK sets, but starting with the first 2 options makes sense. My only suggestion at this stage is to implement most of this code in the abstract class, similarly to AbstractBearerTokenExtractor, where this abstract class would have the abstract HTTP method and request URI getters. The DPop JAX-RS filter extending it will indeed be located in this library while Quarkus, WildFly, etc extensions could use Vertx/Undertow API etc to get the method/URI Please start experimenting with the PR :-) Thanks

acoburn commented 4 years ago

@sberyozkin thanks for the feedback. Using an abstract class, as you propose, makes a lot of sense, and that should be reasonably straight-forward to implement.

sberyozkin commented 4 years ago

@acoburn Hi, have you had a chance to do some prototype ? thanks

acoburn commented 4 years ago

@sberyozkin thanks for the ping. I have done some initial work on this. I'll try to wrap that up and submit as part of a PR

sberyozkin commented 4 years ago

@acoburn Nice, would be good to see it, please take your time