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

jwt credential does not have "username" but "key" #4993

Merged
merged 4 commits into from
Nov 26, 2019
Merged

Conversation

davinwang
Copy link
Contributor

@davinwang davinwang commented Sep 4, 2019

Summary

The jwt credential according to its database definition, does not have username but key. The key can be used to represents the authenticated jwt_credential.

Full changelog

  • X-Credential-Username is correctly set

Issues resolved

X-Credential-Username is correctly set with jwt credential.key (i.e. iss - jwt issuer) which represents the authenticated jwt credential. Therefore other plugins relying on X-Credential-Username will work.

Fix #XXX

the key (i.e. iss - jwt issuer) represents the authenticated jwt credential
@hishamhm
Copy link
Contributor

hishamhm commented Sep 9, 2019

@davinwang hi, that is indeed dead code because credential.username doesn't exist so that if statement never enters (so the if condition needs to be changed as well). What use-case do you have in mind in this? Looks like this header code was copied from the ldap-auth plugin and never really worked.

change credential.username to credential.key in if condition
@davinwang
Copy link
Contributor Author

@hishanmhm hi, my bad, forgot to change the credential.username in if condition ;-)
The scenario is : there is an application server behind Kong, it will extract "x-credential-username" along with "x-consumer-username" from http request header to identify the consumer and credential, regardless the detail authentication method. Some consumer may pass authentication with Basic Auth, some by Jwt Auth, etc. Much appreciate if Kong can provide a consistent behavior between plugins.

@hishamhm
Copy link
Contributor

@davinwang We end up in a strange situation because the JWT key is not really a "username"... I don't know if it wouldn't cause more confusion than the benefit of consistency between plugins.

@davinwang
Copy link
Contributor Author

@hishamhm I think the consistency is about how to define X-Credential-Username. For me, it represents the identity of the authenticated key, just like X-Consumer-Username represents for the identity of authenticated consumer. Whether its name is "key" or "username" is not the most important problem to me. Basic-auth and hmac-auth use "username", key-auth and jwt-auth use "key", unless we migrate the database schema.
And btw, the key-auth plugin is also broken.

@kikito
Copy link
Member

kikito commented Sep 30, 2019

Hi, we have discussed this internally and we think X-Credential-Username is not appropriate for "key" plugins. Instead we think it would be better to add a new header called X-Credential-Identifier on every auth plugin, and at the same time keep X-Credential-Username on the plugins which already use it.

@davinwang
Copy link
Contributor Author

@kikito @hishamhm Hi, will Kong team add this 'X-Credential-Identifier' or let the community do it? Would you suggest a timeline for this feature?

@kikito
Copy link
Member

kikito commented Oct 21, 2019

Hi, what I was suggesting is that you changed the header in your PR to X-Credential-Identifier.

Apologies if this wasn't clear.

Add header X-Credential-Identifier according to Kong#4993 , representing the identity of the verified credential
replace X-Credential-Username with X-Credential-Identifier according to Kong#4993
@davinwang
Copy link
Contributor Author

@kikito Hi, I changed the PR according to above discussion, please feel free to advise

@kikito kikito changed the base branch from master to next October 28, 2019 13:22
@kikito
Copy link
Member

kikito commented Oct 28, 2019

Hi @davinwang, thanks for your changes. Do you mind changing the target branch from master to next on your PR, so Travis picks it up?

Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

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

I'll add some tests after this is merged.

@bungle bungle merged commit 647c409 into Kong:next Nov 26, 2019
@bungle
Copy link
Member

bungle commented Nov 26, 2019

Thank you @davinwang, the PR is now merged! Please go ahead and grab a t-shirt: https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributor-t-shirt

:-)

bungle added a commit that referenced this pull request Nov 26, 2019
### Summary

Adds test for a feature contributed by community member @davinwang.
bungle added a commit that referenced this pull request Jan 31, 2020
…redential-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `Basic Auth Plugin`.
bungle added a commit that referenced this pull request Jan 31, 2020
…dential-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `Key Auth Plugin`.
bungle added a commit that referenced this pull request Jan 31, 2020
…edential-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `HMAC Auth Plugin`.
bungle added a commit that referenced this pull request Jan 31, 2020
…edential-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `LDAP Auth Plugin`.
bungle added a commit that referenced this pull request Jan 31, 2020
…ntial-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `OAuth2 Plugin`.

This feature was originally implemented by @lucasmoreno on PR #5201. Thank you, Lucas,
grab your T-shirt:
https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributor-t-shirt!

I just refactored @lucasmoreno's code to use our generic `X-Credential-Identifier`
header instead of the proposed `X-Credential-Client-Id`.

### Issues Resolved

Closes #5201
bungle added a commit that referenced this pull request Feb 3, 2020
…dential-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `Key Auth Plugin`.
bungle added a commit that referenced this pull request Feb 3, 2020
…edential-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `HMAC Auth Plugin`.
bungle added a commit that referenced this pull request Feb 3, 2020
…edential-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `LDAP Auth Plugin`.
bungle added a commit that referenced this pull request Feb 3, 2020
…ntial-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `OAuth2 Plugin`.

This feature was originally implemented by @lucasmoreno on PR #5201. Thank you, Lucas,
grab your T-shirt:
https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributor-t-shirt!

I just refactored @lucasmoreno's code to use our generic `X-Credential-Identifier`
header instead of the proposed `X-Credential-Client-Id`.

### Issues Resolved

Closes #5201
bungle added a commit that referenced this pull request Feb 4, 2020
…redential-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `Basic Auth Plugin`.
bungle added a commit that referenced this pull request Feb 4, 2020
…dential-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `Key Auth Plugin`.
bungle added a commit that referenced this pull request Feb 4, 2020
…edential-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `HMAC Auth Plugin`.
bungle added a commit that referenced this pull request Feb 4, 2020
…edential-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `LDAP Auth Plugin`.
bungle added a commit that referenced this pull request Feb 4, 2020
…ntial-Username)

### Summary

The PR #4993 implemented `X-Credential-Identifier` for `JWT Plugin` and it was decided
at time that we should add support for this less opinionated field name on other auth
plugins too. This commit adds it to `OAuth2 Plugin`.

This feature was originally implemented by @lucasmoreno on PR #5201. Thank you, Lucas,
grab your T-shirt:
https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributor-t-shirt!

I just refactored @lucasmoreno's code to use our generic `X-Credential-Identifier`
header instead of the proposed `X-Credential-Client-Id`.

### Issues Resolved

Closes #5201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants