You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/04/11 04:55:21 UTC
[GitHub] [pulsar] jiazhai opened a new pull request #6716: [Issue #6711]:
add audience verify in AuthenticationProviderToken
jiazhai opened a new pull request #6716: [Issue #6711]: add audience verify in AuthenticationProviderToken
URL: https://github.com/apache/pulsar/pull/6716
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](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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] jiazhai commented on issue #6716: [Issue #6711]: add
audience verify in AuthenticationProviderToken
Posted by GitBox <gi...@apache.org>.
jiazhai commented on issue #6716: [Issue #6711]: add audience verify in AuthenticationProviderToken
URL: https://github.com/apache/pulsar/pull/6716#issuecomment-614350067
Thanks @sijie, rebased with latest master, and fixed the ut error, which caused by exception not match
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] sijie commented on issue #6716: [Issue #6711]: add
audience verify in AuthenticationProviderToken
Posted by GitBox <gi...@apache.org>.
sijie commented on issue #6716: [Issue #6711]: add audience verify in AuthenticationProviderToken
URL: https://github.com/apache/pulsar/pull/6716#issuecomment-612778125
@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)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] sijie commented on a change in pull request #6716: [Issue
#6711]: add audience verify in AuthenticationProviderToken
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #6716: [Issue #6711]: add audience verify in AuthenticationProviderToken
URL: https://github.com/apache/pulsar/pull/6716#discussion_r407090659
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -126,9 +137,40 @@ private static String validateToken(final String token) throws AuthenticationExc
@SuppressWarnings("unchecked")
private Jwt<?, Claims> authenticateToken(final String token) throws AuthenticationException {
try {
- return Jwts.parser()
+ Jwt<?, Claims> jwt = Jwts.parser()
.setSigningKey(validationKey)
.parse(token);
+
+ if (audienceClaim != null) {
+ if (audience == null){
Review comment:
I think we need to move the configuration validation logic to right after line 86. Since it indicates iellgall configuration settings.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] jiazhai merged pull request #6716: [Issue #6711]: add
audience verify in AuthenticationProviderToken
Posted by GitBox <gi...@apache.org>.
jiazhai merged pull request #6716: [Issue #6711]: add audience verify in AuthenticationProviderToken
URL: https://github.com/apache/pulsar/pull/6716
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] jiazhai commented on issue #6716: [Issue #6711]: add
audience verify in AuthenticationProviderToken
Posted by GitBox <gi...@apache.org>.
jiazhai commented on issue #6716: [Issue #6711]: add audience verify in AuthenticationProviderToken
URL: https://github.com/apache/pulsar/pull/6716#issuecomment-612327911
Also @EronWright, Would you please help review it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] jiazhai commented on a change in pull request #6716:
[Issue #6711]: add audience verify in AuthenticationProviderToken
Posted by GitBox <gi...@apache.org>.
jiazhai commented on a change in pull request #6716: [Issue #6711]: add audience verify in AuthenticationProviderToken
URL: https://github.com/apache/pulsar/pull/6716#discussion_r407209153
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -126,9 +137,40 @@ private static String validateToken(final String token) throws AuthenticationExc
@SuppressWarnings("unchecked")
private Jwt<?, Claims> authenticateToken(final String token) throws AuthenticationException {
try {
- return Jwts.parser()
+ Jwt<?, Claims> jwt = Jwts.parser()
.setSigningKey(validationKey)
.parse(token);
+
+ if (audienceClaim != null) {
+ if (audience == null){
+ throw new JwtException("Token Audience Claim [" + audienceClaim
+ + "] configured, but Audience stands for this broker not.");
+ }
+
+ Object object = jwt.getBody().get(audienceClaim);
+ if (object == null) {
+ throw new JwtException("Found null Audience in token, for claimed field: " + audienceClaim);
Review comment:
Right. currently, the JwtException will be wrapped into a AuthenticationException in line 174 below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] sijie commented on a change in pull request #6716: [Issue
#6711]: add audience verify in AuthenticationProviderToken
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #6716: [Issue #6711]: add audience verify in AuthenticationProviderToken
URL: https://github.com/apache/pulsar/pull/6716#discussion_r407090833
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -126,9 +137,40 @@ private static String validateToken(final String token) throws AuthenticationExc
@SuppressWarnings("unchecked")
private Jwt<?, Claims> authenticateToken(final String token) throws AuthenticationException {
try {
- return Jwts.parser()
+ Jwt<?, Claims> jwt = Jwts.parser()
.setSigningKey(validationKey)
.parse(token);
+
+ if (audienceClaim != null) {
+ if (audience == null){
+ throw new JwtException("Token Audience Claim [" + audienceClaim
+ + "] configured, but Audience stands for this broker not.");
+ }
+
+ Object object = jwt.getBody().get(audienceClaim);
+ if (object == null) {
+ throw new JwtException("Found null Audience in token, for claimed field: " + audienceClaim);
Review comment:
If the broker configures the audience claim and receives a token that doesn't contain the audience claim, we should throw AuthenticationException.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] sijie commented on a change in pull request #6716: [Issue
#6711]: add audience verify in AuthenticationProviderToken
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #6716: [Issue #6711]: add audience verify in AuthenticationProviderToken
URL: https://github.com/apache/pulsar/pull/6716#discussion_r407090897
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -126,9 +137,40 @@ private static String validateToken(final String token) throws AuthenticationExc
@SuppressWarnings("unchecked")
private Jwt<?, Claims> authenticateToken(final String token) throws AuthenticationException {
try {
- return Jwts.parser()
+ Jwt<?, Claims> jwt = Jwts.parser()
.setSigningKey(validationKey)
.parse(token);
+
+ if (audienceClaim != null) {
+ if (audience == null){
+ throw new JwtException("Token Audience Claim [" + audienceClaim
+ + "] configured, but Audience stands for this broker not.");
+ }
+
+ Object object = jwt.getBody().get(audienceClaim);
+ if (object == null) {
+ throw new JwtException("Found null Audience in token, for claimed field: " + audienceClaim);
+ }
+
+ if (object instanceof List) {
+ List<String> audiences = (List<String>) object;
+ // audience not contains this broker, throw exception.
+ if (!audiences.stream().anyMatch(audienceInToken -> audienceInToken.equals(audience))) {
+ throw new AuthenticationException("Audiences in token: [" + String.join(", ", audiences)
+ + "] not contains this broker: " + audience);
+ }
+ } else if (object instanceof String) {
+ if (!object.equals(audience)) {
+ throw new AuthenticationException("Audiences in token: [" + object
+ + "] not contains this broker: " + audience);
+ }
+ } else {
+ // should not reach here.
+ throw new AuthenticationException("Audiences in token is not in String format: " + object);
Review comment:
```suggestion
throw new AuthenticationException("Audiences in token is not in expected format: " + object);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] sijie commented on a change in pull request #6716: [Issue
#6711]: add audience verify in AuthenticationProviderToken
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #6716: [Issue #6711]: add audience verify in AuthenticationProviderToken
URL: https://github.com/apache/pulsar/pull/6716#discussion_r407252983
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -126,9 +137,40 @@ private static String validateToken(final String token) throws AuthenticationExc
@SuppressWarnings("unchecked")
private Jwt<?, Claims> authenticateToken(final String token) throws AuthenticationException {
try {
- return Jwts.parser()
+ Jwt<?, Claims> jwt = Jwts.parser()
.setSigningKey(validationKey)
.parse(token);
+
+ if (audienceClaim != null) {
+ if (audience == null){
Review comment:
I think the check here should be moved to `initialize` method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services