search

arrikto / oidc-authservice

This is a fork/refactoring of the ajmyyra/ambassador-auth-oidc project
MIT License
87 stars 66 forks source link

feature: add an id token authenticator #44

Closed yhwang closed 3 years ago

yhwang commented 4 years ago

The id token authenticator verifies the token in the header. The header name of id token is specified by the ID_TOKEN_HEADER config. The authenticator retrieves the USERID_CLAIM after the verification. If there is no id token header, the verification failes or the USERID_CLAIM does not exist, the request is passed to the next authenticator.

yhwang commented 4 years ago

@yanniszark I saw there is an unused variable UserIDTokenHeader in config struct (settings.go). I don't know what's the purpose of it. Therefore, I create a new variable for the id-token header.

yhwang commented 4 years ago

normally, unauthenticated request would be routed to oidc-authservice and it also contains the client_id, client_secret information, it would be good to have id-token validation as one of the authenticators in oidc-authservice

sudivate commented 4 years ago

Thank you @yhwang for covering this scenario. I have tested this flow with Azure AD and works perfectly for ID tokens.

I was looking at this PR from an extensibility aspect for supporting access token in the future. Especially, access tokens created through grant-type client credentials.

  1. Instead of adding a custom header (X-ID-Token) specific to the ID token, can we have this format Authorization: <type> <credentials> This will enable different credentials in the future without adding more custom headers.

  2. The current implementation to verify id tokens can also we used to verify access tokens as both tokens are JWT. The only diference in their verification would be the claims. For instance, access token obtained through client credentials flow might not have email and thus the verification will involve claims like aud and iss. If will allow supporting these different scenarios through configurations to enable verification for different types of tokens.

yhwang commented 4 years ago

@sudivate

Instead of adding a custom header (X-ID-Token) specific to the ID token, can we have this format Authorization: This will enable different credentials in the future without adding more custom headers.

Based on my understanding, currently, the Authorization is used by session authenticator. it uses the value of authservice_session. If we are going to use the same header, we need a mechanism to distinguish different value types in the session authenticator. Currently, the type should be Bearer for both session and id-token. In this way, it's not able to use the auth-scheme to tell the value type. Info for auth-scheme: https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml. I like the idea of using the same header and adding extension on top of it. But we are limited by some constrains. Therefore, I created another authenticator and use different header. If you have some ideas on this, please let me know.

The current implementation to verify id tokens can also we used to verify access tokens as both tokens are JWT. The only diference in their verification would be the claims. For instance, access token obtained through client credentials flow might not have email and thus the verification will involve claims like aud and iss. If will allow supporting these different scenarios through configurations to enable verification for different types of tokens.

In the id-token authenticator I implemented, it uses USERID_CLAIM to get userid. Maybe adding another option specific for id-token authenticator should be fine, otherwise it just falls back to use USERID_CLAIM. I will wait for @yanniszark 's review and he may has some ideas/comments on this.

animeshsingh commented 4 years ago

@yanniszark can this be merged? Has been pending for a while.

cc @shawnzhu

yanniszark commented 4 years ago

@animeshsingh @yhwang apologies, I haven't had the time to start reviewing this PR yet. I hope to get to it next week. Thanks for the ping! :)

tsuna commented 3 years ago

Ping :)

shawnzhu commented 3 years ago

In the context of Istio, I just want to say this is a good feature (JWT claim based request routing) to have. This is some resources to share to see how others practice this feature:

animeshsingh commented 3 years ago

@cvenets have tried reaching to @yanniszark - is the plan to deprecate this? This PR is pending for couple of months, and is needed by us.

yanniszark commented 3 years ago

Hi everyone, sorry for the lack of activity on this PR! I am oversubscribed in this period of time, that's why I haven't had the chance to review and test this yet. Sorry for the wait, I will try to squeeze this in asap. Thanks for the efforts of everyone involved :)

yhwang commented 3 years ago

@yanniszark thanks for the review

Also, have you tested a build to confirm it works for your intended use?

Yes, I tested it before I created the PR. I also verified it again after I incorporated your comments.

One interesting behavior I found and maybe you know the answer: When I use Authorization to carry the id-token, the id-token authenticator gets empty value from the header. However, when I use header other then Authorization, for example: X-ID-TOKEN, id-token authenticator is able to get the header and verify the id-token properly. any idea about the root cause?

BTW, when I use X-ID-TOKEN header, I also modified envoyfilter: authn-filter accordingly.

sudivate commented 3 years ago

@yanniszark thanks for the review

Also, have you tested a build to confirm it works for your intended use?

Yes, I tested it before I created the PR. I also verified it again after I incorporated your comments.

One interesting behavior I found and maybe you know the answer: When I use Authorization to carry the id-token, the id-token authenticator gets empty value from the header. However, when I use header other then Authorization, for example: X-ID-TOKEN, id-token authenticator is able to get the header and verify the id-token properly. any idea about the reason?

BTW, when I use X-ID-TOKEN header, I also modified envoyfilter: authn-filter accordingly.

@yhwang you will have to replace this with your function to extract id token the current implementation extracts token from format Bearer <Token>

yhwang commented 3 years ago

@sudivate it can retrieve both Bearer <Token> or <Token> based on the implementation. The situation I found is the Authentication header is empty value.

yanniszark commented 3 years ago

When I use Authorization to carry the id-token, the id-token authenticator gets empty value from the header. However, when I use header other then Authorization, for example: X-ID-TOKEN, id-token authenticator is able to get the header and verify the id-token properly. any idea about the root cause?

@yhwang this is strange. Did you update the EnvoyFilter to allow the Authorization header? Can you print the headers when the request arrives to see if it's there?

yhwang commented 3 years ago

@yanniszark

Did you update the EnvoyFilter to allow the Authorization header? Can you print the headers when the request arrives to see if it's there?

Yes, I added authorization into allowHeaders of authn-filter EnvoyFilter. It's working now. I guess I did some other testings and messed up my testing environment.

To summarize, I tested my latest change, it works with and without ID_TOKEN_HEADER parameter. Please review the change again. Thanks!

yanniszark commented 3 years ago

@yhwang everything looks good! The only thing missing is that you hadn't signed-off your commit. I added a signoff to make it easier for you (you don't have to do any other changes), but I need to ask for your permission before merging. Are you ok with the current commit and with the signoff with your name?

yhwang commented 3 years ago

@yanniszark perfect to me. thanks!