feat: token exchange rfc8693 in impersonation mode

[saxenautkarsh]

Related Issue or Design Document

Implemented rfc8693 token exchange for the impersonation flow as mentioned here. Sorry for coming back so late 🙇🏼

Checklist

• I have read the contributing guidelines and signed the CLA.
• I have referenced an issue containing the design document if my change introduces a new feature.
• I have read the security policy.
• I confirm that this pull request does not address a security vulnerability.
  If this pull request addresses a security vulnerability,
  I confirm that I got approval (please contact security@ory.sh) from the maintainers to push the changes.
• I have added tests that prove my fix is effective or that my feature works.
• I have added the necessary documentation within the code base (if appropriate).

Further comments

[saxenautkarsh]

@aeneasr Sorry but is it possible to take a look at this PR?

[vivshankar]

Just a thought here. Rather than have the list hard coded here, would it be better to have an interface for TokenType that allows you to implement different token types? The interface would effectively have two functions - Validate and Issue.

You would then add a ConfigProvider interface with a function called GetSupportedTokenTypes that could be used within this function.

It allows for greater extensibility IMO.

Examples from my own implementation of token exchange on top of Fosite -

type TokenType interface {
	GetName(ctx context.Context) string

	GetType(ctx context.Context) string

	Validate(ctx context.Context, config *TokenExchangeConfig, ar fosite.AccessRequester, token string) (map[string]interface{}, error)

	Issue(ctx context.Context, config *TokenExchangeConfig, ar fosite.AccessRequester, response fosite.AccessResponder) error
}

Global config provider:

type ConfigProvider interface {
	GetTokenTypes(ctx context.Context) map[string]TokenType

	GetDefaultRequestedTokenType(ctx context.Context) string
}

[vivshankar]

And an example for how AccessTokenType might be implemented:

func (c *AccessTokenType) Validate(ctx context.Context, config *TokenExchangeConfig, request fosite.AccessRequester, token string) (map[string]interface{}, error) {
	client := request.GetClient()

	sig := config.CoreStrategy.AccessTokenSignature(token)
	or, err := config.Storage.GetAccessTokenSession(ctx, sig, request.GetSession())
	if err != nil {
		return nil, errors.WithStack(fosite.ErrRequestUnauthorized.WithDebug(err.Error()))
	} else if err := config.CoreStrategy.ValidateAccessToken(ctx, or, token); err != nil {
		return nil, err
	}

	subjectTokenClientID := or.GetClient().GetID()
	// forbid original subjects client to exchange its own token
	if client.GetID() == subjectTokenClientID {
		return nil, errors.WithStack(fosite.ErrRequestForbidden.WithHint("Clients are not allowed to perform a token exchange on their own tokens"))
	}

	// Scope check
	for _, scope := range request.GetRequestedScopes() {
		if !config.ScopeStrategy(or.GetGrantedScopes(), scope) {
			return nil, errors.WithStack(fosite.ErrInvalidScope.WithHintf("The subject token is not granted \"%s\" and so this scope cannot be requested.", scope))
		}
	}

	// Convert to flat session with only access token claims
	var tokenObject map[string]interface{}
	if session, ok := or.GetSession().(Session); ok {
		tokenObject = session.AccessTokenClaimsMap()
		tokenObject["client_id"] = or.GetClient().GetID()
		tokenObject["scope"] = or.GetGrantedScopes()
		tokenObject["aud"] = or.GetGrantedAudience()
	} else {
		tokenObject = map[string]interface{}{
			"client_id": or.GetClient().GetID(),
			"scope":     or.GetGrantedScopes(),
			"sub":       or.GetSession().GetSubject(),
			"username":  or.GetSession().GetUsername(),
			"aud":       or.GetGrantedAudience(),
		}
	}

	return tokenObject, nil
}

[saxenautkarsh]

@vivshankar Thanks for the feedback. This makes it more flexible & extensible and can in a way support all the subject token types

The only worrying point I have is that when implementing AccessTokenType the user will need to implement parts of the RFC that should in principle be provided in the library itself.

[saxenautkarsh]

My assumption when doing the implementation was that the support for the remaining token types will be added gradually. But I agree that this approach makes it hard to use other subject token types without modifying fosite.

[vivshankar]

@saxenautkarsh We re-use the strategies, so this isn't actually duplicating the handlers outside of a couple of minor bits. I can submit a sample on my fork for you to look at, if interested, but it will take until at least this weekend.

We have implemented both external JWT and access token, which an intention of adding external access token and other token types. The interfaces can probably be fine-tuned further.

[saxenautkarsh]

@vivshankar How about additionally we provide a default implementation of AccessTokenType interface?

That will help in keeping it extensible for other token types and some custom implementations and at the same time easy to use because of the default implementation. Also, the default implementation can be made better with time to address all token types.
What do you think?

[asv]

@saxenautkarsh any news? this is a highly anticipated feature...

[saxenautkarsh]

This is a highly anticipated feature...

@asv Thank you! Sorry for the delay, I will fix the above-mentioned issue this week.

[vivshankar]

Shouldn't this condition be clientID == cid? If my understanding is right, you want to check if the requesting client is allowed to exchange the subject token issued to another client. or.GetClient() will return the subject token's client?

[vivshankar]

One other thing you might consider adding is a check to prevent a client from using token exchange to replace refresh token flow. The purpose of token exchange is for a different client to exchange one token for another. I am not sure this is in the spec but I feel it should be added.

        tokenClientID := or.GetClient().GetID()
	// forbid original subjects client to exchange its own token
	if clientID == tokenClientID {
		return nil, errors.WithStack(fosite.ErrRequestForbidden.WithHint("Clients are not allowed to perform a token exchange on their own tokens."))
	}

The alternative is to add a Storage function to perform any additional validation. Unfortunately, adding this in ValidateAccessToken won't be practical because it is lacking the necessary parameters.

[saxenautkarsh]

Shouldn't this condition be clientID == cid?

Yes, in followup with the other comment, it should be clientID == cid.

One other thing you might consider adding is a check to prevent a client from using token exchange to replace refresh token flow. The purpose of token exchange is for a different client to exchange one token for another. I am not sure this is in the spec but I feel it should be added.

Yes this point is quite important, I gave it a thought but since it is not mentioned in the spec, I left it to the respective implementations. Overall I don't expect the storage to return allowedClientIDs containing the client for the subject token itself. But since it is not in the spec, I don't intend to impose it.

[vivshankar]

I am not sure we should clone the subject token's session here because it might contain extra claims that have nothing to do with the token requested. Also, wouldn't this modify the client_id, aud and other claims in the introspection response (maybe it doesn't)?

A more controlled change might be to introduce a Session interface for this package and add a SetSubject(string) to it.

[vivshankar]

Would it make sense to create a rfc8693.Client interface with GetAllowedClientIDsForTokenExchange() instead of putting this in storage? This is going to be part of the client configuration?

[saxenautkarsh]

@vivshankar Sorry I realized there is an error here. It should be

allowClientIDs, err := c.Storage.GetAllowedClientIDs(ctx, or.GetClient().GetID());

The client ID used here should come from the subject token. Because here I want to query the storage for the clients that are allowed to perform token exchange with the given subject token.

In the storage, for each client that is allowing other clients to perform token exchange, we will store a "1 to many" mapping.

[vivshankar]

@saxenautkarsh Thanks. I made a minor change here to use a client function instead -

// TokenExchangeAllowed checks if the subject token client allows the specified client
// to perform the exchange
TokenExchangeAllowed(client fosite.Client) bool

The reasoning is this gives greater flexibility to the implementer. In reality, if you have a fair number of clients, configuring the matrix of clients is going to be rather difficult. Instead, if a categorizaton mechanism exists (like free-form tags or a category property on the client object), that would allow for an easier mechanism to configure this "grouping". It can be implemented in a custom handler certainly but I felt it served the same purpose with more flexibility.

[vivshankar]

@saxenautkarsh I have taken elements of what you have in this PR and incorporated this in a PR that contains support for more token types and also adds delegation. Would you be willing to collaborate? I have the PR here if you would like to take a look - vivshankar#1. It is missing quite a few tests and I am not too happy currently with the JWT validation approach for the custom JWT type.

[vvakame]

@aeneasr hi! Thanks for the interesting framework fosite! I'm enjoying creating IdPs as a hobby now, and I found this PR when I had the idea to combine GitHub Actions with token-exchange. What do you think about this PR?

oh, I tried this patch set, I found outdated part. e.g. CanHandleTokenEndpointRequest signature has ctx as 1st argument now.
Please let me know if there is anything I can do to help.

[mitar]

So what is the status of this? Work moved to vivshankar#1?

[vivshankar]

Given I heard nothing back from the original author, I did not advance this. You can however find an implementation in https://github.com/vivshankar/fosite/tree/v0.44.x.

[vivshankar]

@mitar Actually I didn't port the code into my fosite fork for token exchange. I have both impersonation and delegation written for a variety of different token types, including the device_secret as an actor token for native app SSO spec (draft). It runs in two of the products I work on and is used in different scenarios in production deployments. If there's interest, I am happy to invest the time to port this into my fork first and then create a PR here.

I just wanted to make sure @saxenautkarsh was in the loop given he started this work here.

I will defer to @aeneasr on how he would like to proceed.

[saxenautkarsh]

@vivshankar Thanks a tonne for your continued interest in this.
Sorry I have been off from here for so long. Since creating this PR, I have moved jobs and changed the dev domain.
But I would like to help finish this.

If there's interest, I am happy to invest the time to port this into my fork first and then create a PR here.

Sure. I am happy if this helps in your development and would like to help in any way I can. I now have some spare time over the weekends.