-
Notifications
You must be signed in to change notification settings - Fork 351
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
oid_introspection: add SetOIDCClaims #3311
base: master
Are you sure you want to change the base?
Conversation
This method allows third-party filters to set the oidcClaimsCacheKey which enables the use of the oidcClaimsQuery filter. Signed-off-by: Adrien Surée <[email protected]>
4a0c06c
to
c893a51
Compare
filters/auth/oidc_introspection.go
Outdated
@@ -42,6 +42,12 @@ func NewOIDCQueryClaimsFilter() filters.Spec { | |||
} | |||
} | |||
|
|||
func MakeTokenContainer(h map[string]interface{}) tokenContainer { |
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.
Its an exported method that returns non-exported type - not good.
The original issue was:
However, when working with a third-party filter, it is not possible to create a tokenContainer value because this type is private to the filters/auth package. Therefore it is not possible to set the OIDC claims cache key to a value that would be accepted by oidcClaimsQuery, since the filter expects a value of type tokenContainer to be stored there.
This prevents the reuse of oidcClaimsQuery when the authentication is done in a third-party filter.
and proposed solution:
A function should be exported that sets the OIDC claims cache key (given a map[string]interface{} claims value).
But this change does not implement the proposal - user of MakeTokenContainer still needs to rely on a known value of statebag key.
I think the proper change should provide an utility to store token container into statebag, it should be documented and existing code should be changed to use it.
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're right, I remembered incorrectly what I was supposed to implement. I'll change it to what was proposed in the issue.
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.
"existing code should be changed to use it" do you mean the auth filters in skipper that store the token container in the statebag should use this new method? They wouldn't be able to do that with the proposed interface since they store more than just claims. But on the other hand it doesn't seem like any field other than claims is used when reading from the statebag.
Signed-off-by: Adrien Surée <[email protected]>
a2c324c
to
4f4de08
Compare
This method allows third-party filters to set the oidcClaimsCacheKey which enables the use of the oidcClaimsQuery filter.
Closes #3139