-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add introspection endpoint for s2s flow #2567
Conversation
b7d76fa
to
463be60
Compare
@@ -133,8 +137,8 @@ func (r Wrapper) createAccessToken(issuer did.DID, issueTime time.Time, presenta | |||
}, nil | |||
} | |||
|
|||
func (r Wrapper) accessTokenStore(issuer did.DID) storage.SessionStore { | |||
return r.storageEngine.GetSessionDatabase().GetStore(accessTokenValidity, "s2s", issuer.String(), "accesstoken") | |||
func (r Wrapper) s2sAccessTokenStore() storage.SessionStore { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This removes the tenant. Now an access token would be introspectable across tenant borders and thus usable on all tenants. It would depend on what the AAA does, but best is to return an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introspection EP currently doesn't know who is making the call, so the tenant is unknown. We need to discus what options we have here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is indeed a fundamental change. Do we know for sure that the aud
in the access token request is transferred to the iss
/sub
? This makes the access token introspection determine the tenant. That would be OK, but best to check A-Z if we can't leak anything across tenant boundaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The presentation definition should extract tenant (did) from the vp_token to the PDPMap as discussed so it can be validated at the AAA. That is out of scope for this PR though
docs/_static/auth/iam.yaml
Outdated
@@ -456,11 +456,24 @@ components: | |||
scope: | |||
type: string | |||
description: granted scopes | |||
vcs: | |||
pdp_map: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mixing short form and long form now, gotta choose one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could take a look at https://www.iana.org/assignments/jwt/jwt.xhtml for ideas. verified_claims
might be a good fit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think verified_claims
is a good fit (or any other registered claim name). It comes from the OpenID Connect realm and is focussed on user identity. The required trust_framework
field per claims object also adds unnecessary complexity IMO. I prefer to reserve this for user identity as it also provides a standardized output on the assurance level of the claims about the user.
We'd probably have to customize the claim for our purpose so I prefer to just use a custom claim. This is an internal endpoint so a custom claim does not impact interoperability. I've changed the name to input_descriptor_constraint_id_map
. Very verbose, but I am open to changes
cfa34d8
to
54fed45
Compare
closes #2541
I've copied the v1 introspection OAPI and edited it. There are still a couple of fields unused for the user authentication part.
I've also removed the issuer key from the s2s access token store since its not available during introspection