Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DPoP design doc #254

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

DPoP design doc #254

wants to merge 3 commits into from

Conversation

dteleguin
Copy link
Contributor

@dteleguin dteleguin commented Mar 24, 2021

This PR outlines the design of OAuth 2.0 Demonstrating Proof-of-Possession for Keycloak.

design/dpop.md Show resolved Hide resolved
design/dpop.md Show resolved Hide resolved
design/dpop.md Show resolved Hide resolved
design/dpop.md Show resolved Hide resolved
design/dpop.md Show resolved Hide resolved
@tnorimat
Copy link
Contributor

tnorimat commented Apr 2, 2021

Hello @dteleguin, Thank you for your presentation on FAPI-SIG's 16th meeting. I've added some comments. I'm happy if you could check them.

- remove Gatekeeper mentioning
- add a section on refresh tokens and keypair lifetimes
- logout endpoint
- client registration
- relation to other token binding mechanisms
- rearrange milestones
- cleanup
@dteleguin dteleguin marked this pull request as ready for review July 21, 2021 01:29
@dteleguin dteleguin changed the title DPoP design draft DPoP design doc Jul 29, 2021
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dteleguin Thanks for the specification and sorry for the long time to review...

Added few inline points to the specification. I am marking this as "Request Changes" until we resolve the points.

design/dpop.md Show resolved Hide resolved
design/dpop.md Show resolved Hide resolved
design/dpop.md Show resolved Hide resolved
| **Option** | **Description** |
| --- | --- |
| DPoP Proof Lifetime | Time window in which the respective DPoP proof JWT would be accepted |
| DPoP Allowed Clock Skew | To accommodate for clock offsets, the server MAY accept DPoP proofs that carry an `iat` time in the reasonably near future (e.g., a few seconds in the future). |
Copy link
Contributor

@mposolda mposolda Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this config option really needed? Just wondering that ideal would be to avoid many configuration options as each configuration option is adding some complexity (And Keycloak client already has really big amount of configuration options, but even if we do with client policies, one less config option is always good :)). For example JWTClientAuthenticator does not check that "iat" from the token is in the past (see https://github.com/keycloak/keycloak/blob/15.0.2/services/src/main/java/org/keycloak/authentication/authenticators/client/JWTClientAuthenticator.java) and hence it seems to me to have same behaviour for DPoP as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example JWTClientAuthenticator does not check that "iat" from the token is in the past

Did you perhaps mean "in the future"? The only check that is performed against iat is this:

            // KEYCLOAK-2986
            int currentTime = Time.currentTime();
            if (token.getExpiration() == 0 && token.getIssuedAt() + 10 < currentTime) {
                throw new RuntimeException("Token is not active");
            }

That means, we're accepting iat values that are no more than 10 seconds into the past + values set in indefinite future.

Unfortunately, this might not work for DPoP proofs. Let's revisit section 10.1 "DPoP Proof Replay":

If an adversary is able to get hold of a DPoP proof JWT, the adversary could replay that token at the same endpoint (the HTTP endpoint and method are enforced via the respective claims in the JWTs). To prevent this, servers MUST only accept DPoP proofs for a limited time window after their iat time, preferably only for a relatively brief period (on the order of a few seconds).

Servers SHOULD store, in the context of the request URI, the jti value of each DPoP proof for the time window in which the respective DPoP proof JWT would be accepted and decline HTTP requests to the same URI for which the jti value has been seen before. In order to guard against memory exhaustion attacks a server SHOULD reject DPoP proof JWTs with unnecessarily large jti values or store only a hash thereof.

Note: To accommodate for clock offsets, the server MAY accept DPoP proofs that carry an iat time in the reasonably near future (e.g., a few seconds in the future). Because clock skews between servers and clients may be large, servers may choose to limit DPoP proof lifetimes by using server-provided nonce values rather than clock times, yielding intended results even in the face of arbitrarily large clock skews.

Key points are:

  • in order to prevent DPoP proof replays, we must temporarily store the jti values;
  • we should keep the jti values in memory only for the limited time window (aka DPoP proof lifetime), otherwise we'll quickly exhaust memory;
  • DPoP proofs have neither nbf nor exp claims, only iat which is provided by the client; hence, we should enforce limited window of validity based on client-supplied iat only;
  • clock skews can be significant (and could even exceed proof lifetimes, see this), but
  • we cannot allow iats set in indefinite future, since the server will have to store jti until (iat + lifetime) is reached, which creates possibility of conducting memory exhaustion attacks. Hence, allowed clock skew has to be finite.

That said, we should have two server-side parameters, one for DPoP proof lifetime and another one for allowed clock skew. I decided to make them configurable, in order to better suit particular setups and security requirements, similar to what we have in another parts of Keycloak (configurable token lifetimes; configurable clock skews for adapters and IdPs).

The recent version of the DPoP draft offers an alternative mechanism of limiting DPoP proof lifetimes, which is based on server-supplied nonces rather than clock times (see 8. Authorization Server-Provided Nonce and 9. Resource Server-Provided Nonce) and is said to "yield intended results even in the face of arbitrarily large clock skews". However, this method is more complex and won't be featured in the initial PR. And even with this method we'll have to somehow configure lifetimes of server-supplied nonces.

design/dpop.md Show resolved Hide resolved

* org.keycloak.protocol.oidc.OIDCAdvancedConfigWrapper

### Adapters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I would leave adapters as the lowest priority unless there is an explicit request for adding DPoP support to them. Do you have a need for adapters support for your deployment?

Copy link
Contributor Author

@dteleguin dteleguin Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed; no, we do not have any requirements for the adapters.

I think we can focus this document solely on the AS side of things, as the RS side should probably belong to another design document. OK if I remove adapter-specific info and mention the current focus?


As per the spec,

> Servers SHOULD store, in the context of the request URI, the `jti` value of each DPoP proof for the time window in which the respective DPoP proof JWT would be accepted and decline HTTP requests to the same URI for which the `jti` value has been seen before.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you pointed, this is quite challenging to support especially in the cluster environment. I would personally not support this requirement for now (Specification says "SHOULD") unless there is very strong requirement of it. I see there is some security gain, however the price to pay store additional data on the resource servers side is the challenge (especially for the clustering environment).

Copy link
Contributor Author

@dteleguin dteleguin Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This should probably also belong to a RS-specific design document.

@tnorimat
Copy link
Contributor

@dteleguin , @mposolda
The one of the discussion points is to whether we add client settings for DPoP or not.

By adding client settings, we can configure DPoP operation per client, but we still have vast number of client settings so that keycloak admin needs to pay costs for maintaining these client settings value being valid.

By using client policies, we can enforce some operations (mode : required/optional/disabled, DPoP Proof Lifetime, DPoP Allowed Clock Skew) on several clients, which makes us not to add additional client settings.

My vote is to use client policies. I am familiar with client policies so that I can help for implementing DPoP with client policies.

@VinodAnandan
Copy link

@mrpatrickpilch
Copy link

Are there still plans to implement this?

I think it would be a great security addition, since mobile devices' secure enclaves can generate non exportable private keys and enable very effective device binding.

@dteleguin
Copy link
Contributor Author

@mrpatrickpilch yes, this is a work in progress now, and has very high priority. You can track the progress under the draft PR.

It is fairly usable now, bar some functions like client metadata and support for DPoP at UserInfo. If you're ready to engage in some testing and provide feedback, that would be much appreciated!

Copy link
Contributor

@tnorimat tnorimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dteleguin @mposolda I re-reviewed it and add one comment about client settings. Could you check it?


There are the following DPoP-related client settings:

* DPoP mode (enabled/optional/disabled)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To simplify the implementation, it is only needed to add one client settings:

  • dpop_bound_access_tokens (true/false)
    dpop_bound_access_tokens is a client metadata defined in the DPoP specification.
    The boolean value is easy to handle compared with ternary (enabled/optional/disabled).

As for DPoP proof lifetime and clock skew, it might be better to use default values for all clients. It might be better not to add new client settings because keycloak has already a lot of such settings and these settings are difficult to be managed.

If we add client settings at this time and we add some method (e.g., client policies) to remove necessities of the settings afterwards, we cannnot delete these settings because we need to keep backward compatibility.

Therefore, at this time, it might be preferable to use default values only for DPoP proof lifetime and clock skew, and add some mechanism to change these values per client or set of clients without adding client settings afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants