Skip to content

[Issue #6711]: add audience verify in AuthenticationProviderToken #6716

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.

Sign up for GitHub

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

Jump to bottom
Merged
jiazhai merged 6 commits into from
Apr 16, 2020
Merged

[Issue #6711]: add audience verify in AuthenticationProviderToken #6716

jiazhai merged 6 commits into from
Apr 16, 2020

Conversation

@jiazhai
Copy link
Member

@jiazhai jiazhai commented Apr 11, 2020

Fixes #6711

Motivation

User like to be able to configure the JWT authentication provider to verify the audience on incoming tokens. I believe this will improve security because it would prevent a spoofer from reusing a token that was intended for another purpose (yet signed by the same issuer). RFC 6749 section 4.1.3 has some guidance on this. In my scenario, the token is an OAuth 2.0 token, and OAuth 2.0 makes extensive use of the audience claim (ref).

  1. a configurable audience claim name (e.g. aud).
  2. if audience isn't configured, do not validate the audience (for back-compatibility).
  3. if audience is configured, validate that the value is present in the token.

Modifications

  • Add the logic in AuthenticationProviderToken.
  • Add related tests.

Verifying this change

  • Ut passed

@jiazhai
Copy link
Member Author

jiazhai commented Apr 11, 2020

Also @EronWright, Would you please help review it?

@sijie sijie added this to the 2.6.0 milestone Apr 11, 2020
@sijie sijie added the doc-required Your PR changes impact docs and you will update later. label Apr 11, 2020
@sijie
Copy link
Member

sijie commented Apr 13, 2020

@jiazhai there are test failures.

	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:124)
	at org.testng.internal.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:54)
	at org.testng.internal.InvokeMethodRunnable.run(InvokeMethodRunnable.java:44)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

[ERROR] testNoBrokerTokenAudience(org.apache.pulsar.broker.authentication.AuthenticationProviderTokenTest)  Time elapsed: 0.004 s  <<< FAILURE!
org.testng.TestException: 

Expected exception of type class javax.naming.AuthenticationException but got java.lang.IllegalArgumentException: Token Audience Claim [aud] configured, but Audience stands for this broker not.
	at org.testng.internal.ExpectedExceptionsHolder.wrongException(ExpectedExceptionsHolder.java:68)
	at org.testng.internal.Invoker.considerExceptions(Invoker.java:1130)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:615)
	at org.testng.internal.Invoker.retryFailed(Invoker.java:839)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1010)

@jiazhai
Copy link
Member Author

jiazhai commented Apr 16, 2020

Thanks @sijie, rebased with latest master, and fixed the ut error, which caused by exception not match

@jiazhai jiazhai merged commit d6709ae into apache:master Apr 16, 2020
@Huanli-Meng Huanli-Meng mentioned this pull request Jun 11, 2020
Closed
@Huanli-Meng Huanli-Meng mentioned this pull request Jul 9, 2020
Merged
jiazhai pushed a commit that referenced this pull request Jul 10, 2020
@Huanli-Meng
configure reference doc (#7491)
6a4c512 
Motivation
This doc PR is updated for configurations for PRs:
#6716
#6853
#6074

1: The broker configuration (for #6716) is updated by Jia Zhai.

2: Add other supported configurations to the client, standlone and proxy configuration docs based on the client.config, standlone.config and proxy.config files.

Modifications
1: Add TLS with keystore type config in standlone and proxy configuration file.
2: update reference > pulsar configuration > client for PIP-55: Refresh Authentication Credentials
Add other supported configurations to the standlone and proxy configuration files based on the standlone.config and proxy.config files.
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
@jiazhai @huangdx0726
[Issue apache#6711]: add audience verify in AuthenticationProviderTok…
60af3c6 
…en (apache#6716)

Fixes apache#6711

### Motivation
User like to be able to configure the JWT authentication provider to verify the audience on incoming tokens.  I believe this will improve security because it would prevent a spoofer from reusing a token that was intended for another purpose (yet signed by the same issuer).  [RFC 6749 section 4.1.3](https://tools.ietf.org/html/rfc7519#section-4.1.3) has some guidance on this.  In my scenario, the token is an OAuth 2.0 token, and OAuth 2.0 makes extensive use of the audience claim ([ref](https://auth0.com/docs/tokens/guides/validate-access-tokens#check-additional-standard-claims)).

1. a configurable audience claim name (e.g. `aud`).
2. if audience isn't configured, do not validate the audience (for back-compatibility).
3. if audience is configured, validate that the value is present in the token.

### Modifications
- Add the logic in AuthenticationProviderToken.
- Add related tests.

### Verifying this change
- Ut passed
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
@Huanli-Meng @huangdx0726
configure reference doc (apache#7491)
22ef806 
Motivation
This doc PR is updated for configurations for PRs:
apache#6716
apache#6853
apache#6074

1: The broker configuration (for apache#6716) is updated by Jia Zhai.

2: Add other supported configurations to the client, standlone and proxy configuration docs based on the client.config, standlone.config and proxy.config files.

Modifications
1: Add TLS with keystore type config in standlone and proxy configuration file.
2: update reference > pulsar configuration > client for PIP-55: Refresh Authentication Credentials
Add other supported configurations to the standlone and proxy configuration files based on the standlone.config and proxy.config files.
@Anonymitaet Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-required Your PR changes impact docs and you will update later. labels Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sijie sijie sijie approved these changes

@merlimat merlimat Awaiting requested review from merlimat

@tuteng tuteng Awaiting requested review from tuteng

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

Assignees

@jiazhai jiazhai

Labels

area/security doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.

Projects

None yet

Milestone

2.6.0

Development

Successfully merging this pull request may close these issues.

Support for audience claim in JWT token

4 participants

@jiazhai @sijie @wolfstudy @Anonymitaet