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

feat: rfc8693 token exchange impersonation flow (#1218) #516

Closed
wants to merge 13 commits into from
Closed

feat: rfc8693 token exchange impersonation flow (#1218) #516

wants to merge 13 commits into from

Conversation

mpnunes
Copy link

@mpnunes mpnunes commented Oct 20, 2020

ory/hydra#1218

Proposed changes

Added token exchange flow for impersonation as described in rfc8693.

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • 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 green light (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 necessary documentation within the code base (if appropriate).

Further comments

The majority of the implementation was done following a similar structure as for the already existing flows.
After several internal discussions about how to implemented the check of who is allowed to exchange a token (besides requiring the corresponding grant type - see 4.4 in rfc8693) we opted to attach this permission to the subjects client under may_act by adding the client ids of those that should be allowed exchange its token. This decision was made with the purpose of enabling a client to revoke a previously given permission to exchange its token.

@CLAassistant
Copy link

CLAassistant commented Oct 20, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ mpnunes
❌ Miguel Paulos Nunes


Miguel Paulos Nunes seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@@ -50,6 +54,9 @@ type Client interface {

// GetAudience returns the allowed audience(s) for this client.
GetAudience() Arguments

// GetMayAct returns the allowed exchange actor(s) for this client
GetMayAct() Arguments
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should move this to a new interface, like TokenExchangeClient?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean by extending the Client interface?
What would be the advantage of that?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, not changing the Client interface, but creating a new interface. Otherwise any existing implementation of Client interface will break because you added an additional method.

Copy link
Author

Choose a reason for hiding this comment

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

I believe you miss understood me, I meant a new interface that extends Client.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it is easier to change the interface, but that does bring issues with existing implementations of Client interface. If I upgrade to a new version of fosite with this change, my local Client implementation will not work anymore. Is this acceptable? The fix is trivial though (just implement this extra method).

Copy link
Author

@mpnunes mpnunes Oct 23, 2020

Choose a reason for hiding this comment

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

I agree, the fix is trivial, but will have several implications on the remaining implementation. I'll check on those.
On the other side, I think I got your point.

Copy link
Member

Choose a reason for hiding this comment

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

We've done this before, if you need some ideas:

Copy link
Author

Choose a reason for hiding this comment

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

Modified as suggested.

@@ -41,3 +44,7 @@ func NewAccessRequest(session Session) *AccessRequest {
func (a *AccessRequest) GetGrantTypes() Arguments {
return a.GrantTypes
}

func (a *AccessRequest) SetDelegatingClient(client Client) {
Copy link
Contributor

@mitar mitar Oct 22, 2020

Choose a reason for hiding this comment

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

I guess you should define this in an interface as well? TokenExchangeAccessRequestor?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can leave that as is. While it is a breaking change, almost no-one implements their own AccessRequest implementation (afaik).

Copy link
Member

Choose a reason for hiding this comment

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

Another question is if this is delegation or impersonation, as the code comments suggest we only support impersonation right now?

Copy link
Author

Choose a reason for hiding this comment

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

This is impersonation only.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so this would set the OAuth2 Client which the subject_token was granted for in the Token Exchange request, correct? If so, maybe we rename it to SetSubjectTokenClient to allow further extension (in the future) for SetActorTokenClient? And then I think it makes sense to define another interface as @mitar suggested:

// in handler/rfc8693/access_request.go

type interface {
  SetDelegatingClient(client Client)
}

Copy link
Author

Choose a reason for hiding this comment

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

Extracted interface but kept it in same file/package due to otherwise resulting cyclical dependencies.

oauth2.go Outdated
}

// AccessRequester is a token endpoint's request context.
type AccessRequester interface {
// GetGrantType returns the requests grant type.
GetGrantTypes() (grantTypes Arguments)

SetDelegatingClient(client Client)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not backwards compatible to change interfaces like this.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Extracted interface but kept it in same file/package due to otherwise resulting cyclical dependencies, as stated in previous comment.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Just two things that sprung my eye. Unfortunately I had not time to dive into this deeper. I first have to read the RFC and will then revisit. I hope I can get this done next week. Is there a test/conformity suite we can use to test the implementation?

@@ -41,3 +44,7 @@ func NewAccessRequest(session Session) *AccessRequest {
func (a *AccessRequest) GetGrantTypes() Arguments {
return a.GrantTypes
}

func (a *AccessRequest) SetDelegatingClient(client Client) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can leave that as is. While it is a breaking change, almost no-one implements their own AccessRequest implementation (afaik).

@@ -50,6 +54,9 @@ type Client interface {

// GetAudience returns the allowed audience(s) for this client.
GetAudience() Arguments

// GetMayAct returns the allowed exchange actor(s) for this client
GetMayAct() Arguments
Copy link
Member

Choose a reason for hiding this comment

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

We've done this before, if you need some ideas:

if !c.ScopeStrategy(request.GetClient().GetScopes(), scope) {
if !c.ScopeStrategy(request.GetClient().GetScopes(), scope) &&
// if it is a refresh of an exchanged token, check scopes against those of the delegating client
(delegatingClient == nil || !c.ScopeStrategy(delegatingClient.GetScopes(), scope)) {
return errors.WithStack(fosite.ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope \"%s\".", scope))
Copy link
Member

Choose a reason for hiding this comment

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

I think this will be super-duper hard to debug for people. Maybe it makes sense to have two separate error messages here instead where one clearly states that the delegating client has insufficient privileges

Copy link
Author

Choose a reason for hiding this comment

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

I think there may be a miss understanding.
Here we check that the granted scopes are either available on the requesting client as previously (in the case this is a refresh of an exchanged token, this would be the client that performed the token exchange), or on the scopes of the delegating client (the client that provided the token that was exchanged).
This was an implementation decision we made in which we allow a client (performing the token exchange) to request scopes available on the delegating client or on its own client.
If there is a delegating client (implies that the refresh flow is being done for an exchanged token) the requested scopes can be declared on either of the clients.
At this point we cannot distinguish, if the requested scopes should be on the delegating client or on the own client.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you! This looks very well thought out and written.

I finally had some time off to dive deep into this PR and the accompanying spec. I left a lot of comments - please don't feel discouraged :) But as you can imagine, this is a pretty serious security component and as such it needs tough and thorough review. Please be prepared to walk through several code review iterations before this can be merged and let me know beforehand if you do not have the time to do them. :)

Besides code comments, I also have some general questions. The spec asys:

In the absence of one-time-use or other semantics specific to the
token type, the act of performing a token exchange has no impact on
the validity of the subject token or actor token. Furthermore, the
exchange is a one-time event and does not create a tight linkage
between the input and output tokens, so that (for example) while the
expiration time of the output token may be influenced by that of the
input token, renewal or extension of the input token is not expected
to be reflected in the output token's properties. It may still be
appropriate or desirable to propagate token-revocation events.
However, doing so is not a general property of the STS protocol and
would be specific to a particular implementation, token type, or
deployment.

To me it looks like token revokation would currently not have an impact on impersonated or delegated tokens using this flow. Is this by design, and if so why?

An authorization server may be unwilling or unable to fulfill any
token request, but the likelihood of an unfulfillable request is
significantly higher when very broad access rights are being
solicited. As such, in the absence of specific knowledge about the
relationship of systems in a deployment, clients should exercise
discretion in the breadth of the access requested, particularly the
number of target services. An authorization server can use the
"invalid_target" error code, defined in Section 2.2.2, to inform a
client that it requested access to too many target services
simultaneously.

The invalid_target error should be in the default errors exposed by fosite in my opinion.

@@ -0,0 +1,170 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

I think this file should move into a new package called handler/rfc8693

Copy link
Author

Choose a reason for hiding this comment

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

Moved.

handler/oauth2/flow_token_exchange.go Outdated Show resolved Hide resolved
handler/oauth2/flow_token_exchange.go Outdated Show resolved Hide resolved
handler/oauth2/flow_token_exchange.go Outdated Show resolved Hide resolved
handler/oauth2/flow_token_exchange.go Outdated Show resolved Hide resolved
@@ -41,3 +44,7 @@ func NewAccessRequest(session Session) *AccessRequest {
func (a *AccessRequest) GetGrantTypes() Arguments {
return a.GrantTypes
}

func (a *AccessRequest) SetDelegatingClient(client Client) {
Copy link
Member

Choose a reason for hiding this comment

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

Another question is if this is delegation or impersonation, as the code comments suggest we only support impersonation right now?

@@ -90,8 +94,27 @@ func (c *RefreshTokenGrantHandler) HandleTokenEndpointRequest(ctx context.Contex
request.SetRequestedScopes(originalRequest.GetRequestedScopes())
request.SetRequestedAudience(originalRequest.GetRequestedAudience())

var delegatingClient fosite.Client = nil
if originalRequest.GetDelegatingClient() != nil {
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this would only check for the first delegation (or impersonation?) that happened and would not follow the whole chain of delegation? If that's true, why was it implemented in that way?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not quite sure what you mean.
Here we get the original client/principal in order to validate its scopes and may_act values.
From your point of view, what would be the reason to keep/follow the whole chain of delegation?

Comment on lines 123 to 126
request.GetSession().SetExpiresAt(fosite.AccessToken, time.Now().UTC().Add(c.AccessTokenLifespan))
if c.RefreshTokenLifespan > -1 {
request.GetSession().SetExpiresAt(fosite.RefreshToken, time.Now().UTC().Add(c.RefreshTokenLifespan).Round(time.Second))
}
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is needed because RefreshTokenGrantHandler doesn't handle this grant_type? However, to me it also looks like requested_token_type should be responsible for setting this.

Copy link
Author

@mpnunes mpnunes Nov 4, 2020

Choose a reason for hiding this comment

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

The logic we use here is pretty much the same as the the one already used e.g. flow_authorize_code_token.
I disagree, regarding the use of requested_token_type.

requested_token_type
OPTIONAL . An identifier, as described in Section 3, for the type of the requested security
token
. ...

In my understanding, a refresh token is not really a security token. I also do not see the point of making an exchange request just to get an refresh token.
Perhaps I miss understood you?

return errors.WithStack(fosite.ErrServerError.WithDebug(err.Error()))
}
}
response.SetExtra("refresh_token", refresh)
Copy link
Member

Choose a reason for hiding this comment

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

It isn't quite clear to me why we would return a refresh_token here? The spec says:

OPTIONAL. A refresh token will typically not be issued when the
exchange is of one temporary credential (the subject_token) for a
different temporary credential (the issued token) for use in some
other context. A refresh token can be issued in cases where the
client of the token exchange needs the ability to access a
resource even when the original credential is no longer valid
(e.g., user-not-present or offline scenarios where there is no
longer any user entertaining an active session with the client).
Profiles or deployments of this specification should clearly
document the conditions under which a client should expect a
refresh token in response to "urn:ietf:params:oauth:grant-
type:token-exchange" grant type requests.

So it appears to be an edge case that should be well documented - or when in doubt left out!

Copy link
Author

Choose a reason for hiding this comment

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

I'm not so sure if this should be seen as an edge scenario. As a matter of fact it is a scenario that we have.
The logic we use here is pretty much the same as the the one already used e.g. flow_authorize_code_token
How are such aspects to be documented here?

}

// HandleTokenEndpointRequest implements https://tools.ietf.org/html/rfc8693#section-2.1 (currently impersonation only)
func (c *TokenExchangeGrantHandler) HandleTokenEndpointRequest(ctx context.Context, request fosite.AccessRequester) error {
Copy link
Member

Choose a reason for hiding this comment

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

To me it looks like actor_token and actor_token_type are currently ignored. If they are not implemented, we should throw an error instead of silently accepting their values (and potentially returning a success). To be honest, I also don't really understand what the value of actor_token would be and neither its implications. But as far as I can tell those are values used when performing delegation, and this code currently only supports impersonation.

Copy link
Author

Choose a reason for hiding this comment

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

If they are not implemented, we should throw an error instead of silently accepting their values

I agree and will add the checks for that.

To be honest, I also don't really understand what the value of actor_token would be and neither its implications

We had exactly the same problem. After several discussions on that we came up with a scenario for which it may make sense.
E.g. User A want to allow User B to access service D over service C. Assuming Service C has tokens of both users, it exchanges the token from B (actor) with that from A (subject) and calls D with the resulting token.
Unfortunately I could not find further information around to clarify that.

mpnunes and others added 4 commits October 27, 2020 17:45
@aeneasr
Copy link
Member

aeneasr commented Oct 29, 2020

@mpnunes let me know when this is good for another 👀

mpnunes and others added 3 commits October 30, 2020 11:23
Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>
…enExchangeAccessRequester, updated mocks, moved token exchange flow to own package.
@mpnunes
Copy link
Author

mpnunes commented Nov 4, 2020

To me it looks like token revokation would currently not have an impact on impersonated or delegated tokens using this flow. Is this by design, and if so why?

To be honest I did't thought much about that. Perhaps you can provide some hints about existing mechanisms work in fosite?

The invalid_target error should be in the default errors exposed by fosite in my opinion.

Added and returned in case of invalid audience

@aeneasr
Copy link
Member

aeneasr commented Nov 5, 2020

To be honest I did't thought much about that. Perhaps you can provide some hints about existing mechanisms work in fosite?

iirc we have a request id which we track across all the refresh and access tokens. I think impersonated tokens should also be revoked when the original token is revoked. This makes the most sense to me from a security perspective.

I was finally able to run and automate the oidc conformance test suite for #509 so I will be working on this in the next days or so to fix all the pesky bugs and spec violations. Once that's done, I will be able to work FT on this PR as it needs a lot of attention to detail.

Unfortunately, the OIDC Conformity suite doesn't cover RFC8693. But maybe you know a test suite which we could port to fosite from another library that has support for RFC8693 in a different language? Imi a e2e/spec test suite not written by us is mandatory for merging this PR as the stuff that's being touched has a high impact on security.

@aeneasr
Copy link
Member

aeneasr commented Nov 11, 2020

Thank you for the call @mpnunes today! I read through the KeyCloak documentation which is the most expansive documentation and implementation of Token Exchange. A few things I observed:

  1. They support different token exchange flows - some of them with very custom parameters not specified in the RFC.
  2. They do not use the original access token expiry but use the 3600 value - similar to how you solved it in your PR at the moment.
  3. They do not really explain how revokation works. I assume they simply create a completely new "strain" of tokens that are separate from the original token.
  4. The feature is a preview and needs to be enabled, and has several danger signs.
  5. Public clients must not be allowed to exchange tokens.

Regarding my points where we have the original token A, and then do token exchange for token B, and then another token exchange for B to C: This would only be possible if client A has may_act for B, and B has may_act for C. So this is ok. What is absolutely NOT OK is for client A to do token exchange on itself, as this would mean that the client A could just refresh tokens as it may see fit. So please ensure that client A can not do token exchange with itself!

@mpnunes
Copy link
Author

mpnunes commented Nov 11, 2020

Regarding my points where we have the original token A, and then do token exchange for token B, and then another token exchange for B to C: This would only be possible if client A has may_act for B, and B has may_act for C. So this is ok.

Currently, for that delegation chain to work, the client A would need to have may_act for B and C. In my opinion it should also stay like that. After the exchanged B and C would be acting as A so I disagree that B should be able to enable C to act as A.

What is absolutely NOT OK is for client A to do token exchange on itself, as this would mean that the client A could just refresh tokens as it may see fit. So please ensure that client A can not do token exchange with itself!

Good point, I missed that one. Will implement as asap.

PS. Thank you also for making the call possible

@aeneasr
Copy link
Member

aeneasr commented Nov 18, 2020

No problem! Let me know if you need further help/guidance :)

@aeneasr
Copy link
Member

aeneasr commented Nov 25, 2020

Is there anything I can help you with to get this out? :)

@aeneasr
Copy link
Member

aeneasr commented Jan 11, 2021

Are you still up for the changes? :) If you need any help, let us know!

@aeneasr aeneasr added the stale Feedback from one or more authors is required to proceed. label Jan 11, 2021
@mpnunes
Copy link
Author

mpnunes commented Jan 11, 2021

Are you still up for the changes? :) If you need any help, let us know!

I am :) I've just been busy with other stuff.

@aeneasr aeneasr removed the stale Feedback from one or more authors is required to proceed. label Feb 17, 2021
@aeneasr aeneasr added the feat New feature or request. label Feb 17, 2021
@aeneasr aeneasr marked this pull request as draft February 17, 2021 16:11
@aeneasr
Copy link
Member

aeneasr commented Feb 17, 2021

This is currently blocked but will eventually be worked on.

@Veil
Copy link

Veil commented Apr 9, 2021

Hey, I have a vested interest in this RFC's availability in hydra, digging through this pull request am I correct in stating that the may_act semantics only consider a client's permission to exchange rather than that of a subject's permission? For example subject "bob@example.com" is happy for subject "alice@example.com" to impersonate him, regardless of the client performing the exchange?

If I have missed this use case or have entirely misunderstood the RFC, please let me know, but I don't remember seeing this use case handled in the code entering the pull request and I can't see anywhere in the RFC which states that may_act is restricted to just client ids.

Can I also ask what the reasoning behind blanket preventing public clients from performing token exchange is? Surely the decision behind whether a public client is or isn't allowed to perform a token exchange can be made using the same may_act semantics as previously described. There may definitely be valid use cases where one public client may want to delegate to another.

@mpnunes
Copy link
Author

mpnunes commented Apr 9, 2021

Hey, I have a vested interest in this RFC's availability in hydra, digging through this pull request am I correct in stating that the may_act semantics only consider a client's permission to exchange rather than that of a subject's permission? For example subject "bob@example.com" is happy for subject "alice@example.com" to impersonate him, regardless of the client performing the exchange?

If I have missed this use case or have entirely misunderstood the RFC, please let me know, but I don't remember seeing this use case handled in the code entering the pull request and I can't see anywhere in the RFC which states that may_act is restricted to just client ids.

Can I also ask what the reasoning behind blanket preventing public clients from performing token exchange is? Surely the decision behind whether a public client is or isn't allowed to perform a token exchange can be made using the same may_act semantics as previously described. There may definitely be valid use cases where one public client may want to delegate to another.

Interesting that you bring up that question because I've been also thinking about the current may_act semantic implementation. I believe you are correct in your interpretation of the RFC. Nevertheless I have currently no good solution for two questions around that. The first one is how to configure/set the may_act at when first retrieving a token. The other one is how to allow multiple subjects to exchange the token, one after another as the examples provided in the RFC only "allow" the declaration of one subject within the claim. It looks to me that the specification is somehow inconsistent on that, but perhaps I'm misunderstanding something.
Please fell free to correct me or make a suggestion on how to address those questions.
Furthermore the PR is currently on hold due to organizational reasons but it would be great to ear some ideas on how to address the questions above whenever I can continue the work on it.

import (
"context"
"github.com/ory/fosite/handler/oauth2"
"github.com/ory/fosite/storage"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, the order of imports here is not good.

@pike1212
Copy link

What is the status of this feature?

@aeneasr
Copy link
Member

aeneasr commented Jul 27, 2021

I believe @mpnunes is waiting for approval of legal in order to be able to sign the CLA

@mpnunes
Copy link
Author

mpnunes commented Aug 9, 2021

I believe @mpnunes is waiting for approval of legal in order to be able to sign the CLA

That is correct

@vicaetano
Copy link

any updates on this feature?

@aeneasr
Copy link
Member

aeneasr commented Oct 11, 2021

I was informed that @mpnunes's company is not able to sign the SLA which essentially means that we can not merge these changes into the upstream :/

@mpnunes mpnunes closed this Oct 14, 2021
@mpnunes mpnunes deleted the rfc8693_token_exchange_impersonation branch October 14, 2021 15:06
@saxenautkarsh
Copy link
Contributor

@aeneasr @mpnunes We need this implementation for several projects. I want to know if there is a way we could use it. Also, we have no problem signing the CLA (if that is needed)

@mpnunes
Copy link
Author

mpnunes commented Oct 22, 2021

@saxenautkarsh I cannot give the permission myself, it does not depend on me. I'm hopping to get the ok to be able to create new PR with an improved implementation on the current baseline, but I cannot say if or when I'll get it.

@saxenautkarsh
Copy link
Contributor

@mpnunes Thanks for the response. Considering the timeline we have is short, I want to know who else can we ask for permission.

@mpnunes
Copy link
Author

mpnunes commented Oct 22, 2021

@saxenautkarsh I'm afraid that will not be possible :(
Sorry but it does not depend on me. If it would this feature would be through long ago.

@saxenautkarsh
Copy link
Contributor

@aeneasr We ended up implementing this rfc on our own for the use case. I am thinking of implementing it here too.
The issue is already there, it is okay if I create a PR directly?

@aeneasr
Copy link
Member

aeneasr commented Jun 13, 2022

Thank you, that's great! Does the PR follow the discussion we had on this PR? I think there were some valuable insights :)

If you make your PR, can you please make it against this branch: #667

@saxenautkarsh
Copy link
Contributor

@aeneasr Yes, I will follow the discussions in this PR. Also, our implementation has some things specific to our use case so I will change them to make it more generic.

If you make your PR, can you please make it against this branch: #667

sure!

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

Successfully merging this pull request may close these issues.

8 participants