feat: token exchange rfc8693 in impersonation mode #725

saxenautkarsh · 2022-12-06T19:27:01Z

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 · 2023-01-30T05:37:40Z

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

vivshankar · 2023-02-01T08:08:17Z

handler/rfc8693/handler.go

+	}
+
+	// Verify subject token.
+	switch params.subjectTokenType {


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 }

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 }

@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.

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.

@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.

@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?

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

This is a highly anticipated feature...

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

…the handler package

vivshankar · 2023-05-17T06:06:08Z

handler/rfc8693/handler.go

+	}
+
+	for _, cid := range allowClientIDs {
+		if or.GetClient().GetID() == cid {


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?

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.

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 · 2023-05-17T06:25:28Z

handler/rfc8693/handler.go

+		if err != nil {
+			return errorsx.WithStack(fosite.ErrInvalidRequest.WithHintf("not allowed to token exchange by at: %v", err))
+		}
+		requester.SetSession(or.GetSession().Clone())


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 · 2023-05-17T07:05:48Z

handler/rfc8693/handler.go

+		return nil, err
+	}
+
+	allowClientIDs, err := c.Storage.GetAllowedClientIDs(ctx, clientID)


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?

@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.

@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 · 2023-05-30T16:39:31Z

@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 · 2023-11-02T13:23:26Z

@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 · 2024-02-15T08:41:02Z

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

vivshankar · 2024-02-15T13:51:22Z

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 · 2024-02-17T03:32:16Z

@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 · 2024-02-21T14:23:26Z

@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.

saxenautkarsh force-pushed the token-exchange branch from 16120dd to 1044690 Compare December 6, 2022 19:40

saxenautkarsh changed the title ~~add(handler): Token Exchange rfc8693 in impersonation mode~~ feat: Token Exchange rfc8693 in impersonation mode Dec 6, 2022

saxenautkarsh changed the title ~~feat: Token Exchange rfc8693 in impersonation mode~~ feat: token exchange rfc8693 in impersonation mode Dec 6, 2022

saxenautkarsh force-pushed the token-exchange branch 3 times, most recently from 0bd0c7c to d6522da Compare December 13, 2022 17:08

saxenautkarsh marked this pull request as ready for review December 13, 2022 17:34

saxenautkarsh requested a review from aeneasr as a code owner December 13, 2022 17:34

saxenautkarsh force-pushed the token-exchange branch from d6522da to 9f78a51 Compare December 27, 2022 14:12

vivshankar reviewed Feb 1, 2023

View reviewed changes

add(handler): token exchange rfc8693 in impersonation mode

17fb0ff

saxenautkarsh force-pushed the token-exchange branch from c3aa32b to 1f8ff9b Compare April 22, 2023 13:26

saxenautkarsh added 2 commits April 22, 2023 21:46

change(handler): token exchange related stategy should not be inside …

e0bd8f6

…the handler package

change(handler): update license to 2023 and fix mockgen

7fc351a

saxenautkarsh force-pushed the token-exchange branch from 1f8ff9b to 7fc351a Compare April 22, 2023 14:18

vivshankar reviewed May 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: token exchange rfc8693 in impersonation mode #725

feat: token exchange rfc8693 in impersonation mode #725

saxenautkarsh commented Dec 6, 2022 •

edited

saxenautkarsh commented Jan 30, 2023

vivshankar Feb 1, 2023

vivshankar Feb 1, 2023

saxenautkarsh Feb 6, 2023

saxenautkarsh Feb 6, 2023

vivshankar Feb 6, 2023 •

edited

saxenautkarsh Feb 7, 2023 •

edited

asv Mar 22, 2023

saxenautkarsh Apr 4, 2023

vivshankar May 17, 2023

vivshankar May 17, 2023

saxenautkarsh Jun 11, 2023

vivshankar May 17, 2023

vivshankar May 17, 2023

saxenautkarsh Jun 11, 2023

vivshankar Jun 12, 2023

vivshankar commented May 30, 2023

vvakame commented Nov 2, 2023 •

edited

mitar commented Feb 15, 2024

vivshankar commented Feb 15, 2024

vivshankar commented Feb 17, 2024 •

edited

saxenautkarsh commented Feb 21, 2024

feat: token exchange rfc8693 in impersonation mode #725

Are you sure you want to change the base?

feat: token exchange rfc8693 in impersonation mode #725

Conversation

saxenautkarsh commented Dec 6, 2022 • edited

Related Issue or Design Document

Checklist

Further comments

saxenautkarsh commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivshankar Feb 6, 2023 • edited

Choose a reason for hiding this comment

saxenautkarsh Feb 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivshankar commented May 30, 2023

vvakame commented Nov 2, 2023 • edited

mitar commented Feb 15, 2024

vivshankar commented Feb 15, 2024

vivshankar commented Feb 17, 2024 • edited

saxenautkarsh commented Feb 21, 2024

saxenautkarsh commented Dec 6, 2022 •

edited

vivshankar Feb 6, 2023 •

edited

saxenautkarsh Feb 7, 2023 •

edited

vvakame commented Nov 2, 2023 •

edited

vivshankar commented Feb 17, 2024 •

edited