Potential incoherent issue and expiry instants in Oauth2 tokens

[ch4mpy]

As pointed in #6634 (and #6557), it seems possible to build tokens with issue and expiry instants and differing with what is exposed in claims (or attributes for opaque tokens).

I propose here a rather radical way to prevent this: removing issue and expiration instants members from AbstractOAuth2Token in favor of claims.

I've no illusion this PR will be rejected at this stage, just a support for discussion.

[jgrandja]

Closing - see comments in #6808

[ch4mpy]

I understand you don't want to introduce breaking changes at this stage. Few remarks on the working branch I linked to this ticket:

• neither marking getIssuedAt() and getExpiresAt() as abstract (and providing implementations retrieving it from attributes in concreate classes) nor removing issuedAt and expiresAt properties in AbstractOAuth2Token requires breaking changes
• I don't intend to author anything concerning core objects. To me this is team responsibility: I have neither skills nor background for that. This branch is just a support to explore possibilities and get an idea of impacts. Don't expect atomic changes on it, again it is not intended to be integrated in your codebase.
• breaking changes on this branch are due to other potential modifications orthogonal to initial concern:
  • promoting attributes as AbstractOAuth2Token as all OAuth2 tokens types have some (even refresh-tokens are issued and might expire)
  • making AbstractOAuth2Token a Principal. I was first surprised that principal property in AbstractOAuth2TokenAuthenticationToken is neither a java.security.Principal nor an AuthenticatedPrincipal. This allows to change that and quite a few things suddenly get clearer and simpler to me
• most changes are due to many test fixtures updates and the code here was written in a big working day (so nothing that huge, actually...)

[ch4mpy]

On a purely theoretical point of vue, I'm always puzzled when I have more than a single representation of a value in an app.

Here we have two different representations for issue and expiration dates in the same classes: one as AbstractOAuth2Token members (and as so in each subclass) and one as claims (or attributes for introspection tokens).

IMO this is a design smell. One of the two should be removed and I think it's "members".

To prevent breaking changes, it's possible to (just a suggestion):

• declare getIssuedAt() and getExpiresAt() as abstract in parent class and implement it in concreate ones, accessing the right claims / attributes
• add some consistency checks at construction to safely merge values from constructor args and claims

[jgrandja]

@ch4mpy As already mentioned in this comment

...the concept of claims was introduced in the OpenID Connect related specifications NOT standard OAuth 2.0...

If you look at the hierarchy of AbstractOAuth2Token you will notice 4 sub-classes:

• OAuth2AccessToken - represents an OAuth 2.0 Access Token
• OAuth2RefreshToken - represents an OAuth 2.0 Refresh Token
• OidcIdToken - represents an OpenID Connect 1.0 ID Token, which has claims
• Jwt - represents a JSON Web Token, which has claims

To re-iterate, there is no concept of claims in the standard OAuth 2.0 Authorization Framework. The claims concept is introduced in extension specifications like OpenID Connect 1.0, Jwt (and JOSE framework specs), Token Introspection, etc. Promoting a claims attribute into AbstractOAuth2Token would make OAuth2AccessToken and OAuth2RefreshToken inherit the member and this simply does not align with the representation of an OAuth 2.0 Access Token or Refresh Token as defined by the spec. I would suggest that you review the OAuth 2.0 Authorization Framework to understand where I'm coming from. The current design is correct as it aligns with the related specifications.

[ch4mpy]

@jgrandja please refer to the ticket title. My motivation to open it was "Potential incoherent issue and expiry instants in Oauth2 tokens". Not promote claims / attributes as AbstractOAuth2Token member (which is only one way to achieve that and the result of my explorations to get a "closer to the RFC" implementation on a branch of mine I shared just as a support for discussion, not for it to be merged)

@jzheaux as this ticket is originated from one of our discussions (when removing consistency check while building unit-test Jwt), please help me clarify the need.

I'll try to make very short & easy to reference assertions I base my understanding on. This might help figure out where I'm going wrong. Please correct, complete or further break down:

1. RFC-6749 section 5.1 describes the structure of a successful response containing the access token, not the token itself. It makes a distinction between:
   1.a. required parameters: access_token and token_type
   1.b. recommended or optional parameters: expires_in, refresh_token, scope
   1.c. parameters with unrecognized name (ignored)
   1.d. as a deduction to 1.c., parameters with recognized name
2. access-token structure is described in RFC-6749 section 1.4. Three statements captured my interest:
   2.a. An access token is a string representing an authorization issued to the client.
   2.b. The string is usually opaque to the client.
   2.c. Access token attributes [...] are beyond the scope of this specification
3. the spec describes JSON payloads between actors (authorization server, client, resource server and resource owner user-agent), not internal representations for each of those actors
4. As per 1., some parameters are associated to the access-token (and refresh-token) in successful responses. There are at least two ways to tie it together in Java impl:
   4.a. make those parameters AbstractOAuth2Token members
   4.b. create an OAuth2Authorization containing the AbstractOAuth2Token and other parameters listed at 1.
5. as (a) some are optional and (b) it is legal to recognize unspecified parameters, there are at least to ways to implement it without redundancy:
   5.a. use a single Map<String, Object> parameters (oups, a third name for claims and attributes)
   5.b. use named and typed properties for specified parameters and Map<String, Object> additionalParameters for other recognized ones
6. an implementation with specified parameters being stored twice (once as properties and a second time as Map<String, Object> parameters) is:
   6.a. a waste of resource (minor issue)
   6.b. a risk of incoherent objects if values de-synchronize
7. it is possible to remove issue and expiration instants redundancy in:
   7.a. OidcIdToken and Jwt without breaking changes by making getIssuedAt() and getExpiresAt() as abstract in AbstractOAuth2Token and accessing the values from claims in descendents
   7.b. as OAuth2AccessTokenand OAuth2AccessToken attributes are currently stored outside of it (tokenAttributes in OAuth2IntrospectionAuthenticationToken), it's going to be more difficult to solve without breaking change and maybe the best that can be done for those two is enforcing a consistency check at OAuth2IntrospectionAuthenticationToken build time. As attributes is immutable and both issuedAt & expiresAt are final, this should be enough

As a side note, other assertions which could lead to additional OAuth2 token / authorization design modifications, but are out of scope of this ticket
8. AbstractOAuth2Token and descendants properties structure, names and types differ from what is described in RFC-6749, but as per 3., that might be partially desirable:
8.a. using richer types than "string" and "number" is added value
8.b. expires_in is acceptable for a very short lived payload only but needs to be either turned into expires_at or completed with an issued_at for longer processing or before being stored on the client for further requests
8.c other changes than described at 8.b. in names or structure introduce some fuzz to anyone coming from the spec (or used to a tighter implementation)
9. As per 2.b. It is very likely authorization and resource server have very different internal representation for access-token compared to client and resource-owner user-agent: first details the granted authorization, second is just an opaque string with some meta-data (expiration, scope, refresh-token, and unspecified parameters)