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 2022/07/18 10:29:35 UTC
[GitHub] [pulsar] nodece opened a new pull request, #16650: [improve][authentication] Support for get token from HTTP params
nodece opened a new pull request, #16650:
URL: https://github.com/apache/pulsar/pull/16650
Signed-off-by: Zixuan Liu <no...@gmail.com>
Fixes #16626
### Motivation
A scenario that running the standalone Pulsar with WebSocket service, then using the `token` request param in a WebSocket URL gives an unauthorized response, it seems is not working as intended in the [documentation](https://pulsar.apache.org/docs/client-libraries-websocket/#query-param-1), but it works using a separate WebSocket service.
The root cause is that we did not check whether the token is in the HTTP parameters.
### Modifications
- Add `HttpServletRequestWrapper` to support for get token from HTTP params
- Add a test for this changes
### Documentation
- [x] `doc-not-needed`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- merged pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #16650:
URL: https://github.com/apache/pulsar/pull/16650
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1200662895
Move `release/2.7.5` label to #16892
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1207784296
Move `release/2.10.2` to https://github.com/apache/pulsar/pull/16986.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1207739335
@nodece I think we don't need to cherry-pick this one to release branches? PR https://github.com/apache/pulsar/pull/14044 only happened on master branch
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1207792589
Move `release/2.9.4` label to https://github.com/apache/pulsar/pull/16987.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] acortes-okode commented on a diff in pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
acortes-okode commented on code in PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#discussion_r923329841
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java:
##########
@@ -387,4 +387,26 @@ public boolean isExpired() {
return expiration < System.currentTimeMillis();
}
}
+
+ private static final class HttpServletRequestWrapper extends javax.servlet.http.HttpServletRequestWrapper {
+ private final HttpServletRequest request;
+
+ public HttpServletRequestWrapper(HttpServletRequest request) {
+ super(request);
+ this.request = request;
+ }
+
+ @Override
+ public String getHeader(String name) {
+ // The browser javascript WebSocket client couldn't add the auth param to the request header, use the
+ // query param `token` to transport the auth token for the browser javascript WebSocket client.
+ if (name.equals(HTTP_HEADER_NAME) && request.getHeader(HTTP_HEADER_NAME) == null) {
+ String token = request.getParameter(TOKEN);
+ if (token != null && !token.startsWith(HTTP_HEADER_VALUE_PREFIX)) {
Review Comment:
If we are checking the token for not starting with the "Bearer " prefix ir order to return `"Bearer " + token` as the header value, shouldn't we return the `token` as is if it already starts with "Bearer "? I mean, something like this:
```java
String token = request.getParameter(TOKEN);
if (token != null) {
return !token.startsWith(HTTP_HEADER_VALUE_PREFIX) ? HTTP_HEADER_VALUE_PREFIX + token : token;
}
```
If what is intended is to encourage users to not sending the "Bearer " prefix as part of the `token` query param, then I'm not sure about performing the 'startsWith' check since it will already fail. Or maybe we could have the check and use it to log a warning or error message explaining the cause why the `token` request param value was not taken as the `Authorization` header value?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1199133499
> I found many code changes of this PR relies on #14044. @nodece Could you help verify whether could this PR cherry-picked into branch-2.8?
I submitted https://github.com/apache/pulsar/pull/16871 for branch-2.8.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1187571391
> Hi, it's first time for me submitting a review so I'm not sure if I'm doing it correctly. I have written only one consideration about checking "Bearer " prefix and I tried to test the code by building it locally but it gave me compilation errors on 'pulsar-common' (I'm unable to make it work sorry)
You can use `mvn clean install -Pcore-modules,-main -DskipTests -Dspotbugs.skip` command to build this project.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1190091918
@acortes-okode Thanks for your explanation about security!
> But I think then that the token query param check should only be applied on WebSocket connection URIs (/ws/ and /ws/v2/) and not the rest.
You are right, we should add a filter to check the WebSocket connection URL.
> And I'm sure security here depends a lot on how Apache Pulsar is managed and if the users use that query param for other requests rather than the WebSocket ones but maybe it could be a security risk in some situations. I'm referencing an [OWASP post](https://owasp.org/www-community/vulnerabilities/Information_exposure_through_query_strings_in_url) about this.
Hi @michaeljmarshall @lhotari, I think you will be interested in this, could you share your thoughts?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] acortes-okode commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
acortes-okode commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1188813828
> You can use `mvn clean install -Pcore-modules,-main -DskipTests -Dspotbugs.skip` command to build this project.
Thanks @nodece, I was able to pass the compilation problem on 'pulsar-common' but now I have others in 'managed-ledget', I think is something regarding lombok maybe since it says it cannot find some 'get' symbols. I'll try to figure out if I can later.
Anyways, I was trying to build it to test something I had in mind but maybe you could know it firsthand. The thing is, I'm not sure if `AuthenticationProviderToken#newHttpAuthState` is called for every HTTP request (correct me if I'm wrong) but, if it is, then I think one could manage to authenticate to other requests rather than the WebSocket one by using the `token` request query param (also not sure if this is intended). E.g.
```sh
curl "http://localhost:8080/admin/v2/brokers/configuration?token=ASDFGH...."
```
Thanks in advance!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1207756600
> @nodece I think we don't need to cherry-pick this one to release branches? PR #14044 only happened on the master branch and will release in 2.11.0
#14044 improves the HTTP authentication that adds a new methods to passing the HTTP data, but this issue is still presents in the old version, so we need to cherry-pick to release branches.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1188853663
> > You can use `mvn clean install -Pcore-modules,-main -DskipTests -Dspotbugs.skip` command to build this project.
>
> Thanks @nodece, I was able to pass the compilation problem on 'pulsar-common' but now I have others in 'managed-ledget', I think is something regarding lombok maybe since it says it cannot find some 'get' symbols. I'll try to figure out if I can later.
Build Pulsar requires JDK 17.
> Anyways, I was trying to build it to test something I had in mind but maybe you could know it firsthand. The thing is, I'm not sure if `AuthenticationProviderToken#newHttpAuthState` is called for every HTTP request (correct me if I'm wrong) but, if it is, then I think one could manage to authenticate to other requests rather than the WebSocket one by using the `token` request query param (also not sure if this is intended). E.g.
>
> ```shell
> curl "http://localhost:8080/admin/v2/brokers/configuration?token=ASDFGH...."
> ```
Good idea, I verified that it worked.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#discussion_r923439228
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java:
##########
@@ -387,4 +387,26 @@ public boolean isExpired() {
return expiration < System.currentTimeMillis();
}
}
+
+ private static final class HttpServletRequestWrapper extends javax.servlet.http.HttpServletRequestWrapper {
+ private final HttpServletRequest request;
+
+ public HttpServletRequestWrapper(HttpServletRequest request) {
+ super(request);
+ this.request = request;
+ }
+
+ @Override
+ public String getHeader(String name) {
+ // The browser javascript WebSocket client couldn't add the auth param to the request header, use the
+ // query param `token` to transport the auth token for the browser javascript WebSocket client.
+ if (name.equals(HTTP_HEADER_NAME) && request.getHeader(HTTP_HEADER_NAME) == null) {
+ String token = request.getParameter(TOKEN);
+ if (token != null && !token.startsWith(HTTP_HEADER_VALUE_PREFIX)) {
Review Comment:
You are right!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] acortes-okode commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
acortes-okode commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1190027673
> > Anyways, I was trying to build it to test something I had in mind but maybe you could know it firsthand. The thing is, I'm not sure if `AuthenticationProviderToken#newHttpAuthState` is called for every HTTP request (correct me if I'm wrong) but, if it is, then I think one could manage to authenticate to other requests rather than the WebSocket one by using the `token` request query param (also not sure if this is intended). E.g.
> > ```shell
> > curl "http://localhost:8080/admin/v2/brokers/configuration?token=ASDFGH...."
> > ```
>
> Good idea, I verified that it worked.
Hi @nodece, thanks for checking that!
Sorry since I think that I didn't expose correctly my concerns about that. They are regarding security, I think there is no other option than putting `token` information in the query param when there is a WebSocket request due to browser API limitations. But I think then that the `token` query param check should only be applied on WebSocket connection URIs (`/ws/` and `/ws/v2/`) and not the rest.
Do not know if this could be addressed easily, maybe checking in the `AuthorizationProviderToken` for ws paths? I see references to this paths on `pulsar-websocket` classes:
- [WebSocketConsumerServlet](https://github.com/apache/pulsar/blob/master/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/WebSocketConsumerServlet.java#L27-L28)
- [WebSocketReaderServlet](https://github.com/apache/pulsar/blob/master/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/WebSocketReaderServlet.java#L27-L28)
- [WebSocketProducerServlet](https://github.com/apache/pulsar/blob/master/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/WebSocketProducerServlet.java#L27-L28)
- [WebSocketPingPongServlet](https://github.com/apache/pulsar/blob/master/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/WebSocketPingPongServlet.java#L27-L28)
But not in a "general" one, so maybe just check for `/ws/**` and `/ws/v2/**` in the same `AuthorizationProviderToken` is valid.
And I'm sure security here depends a lot on how Apache Pulsar is managed and if the users use that query param for other requests rather than the WebSocket ones but maybe it could be a security risk in some situations. I'm referencing an [OWASP post](https://owasp.org/www-community/vulnerabilities/Information_exposure_through_query_strings_in_url) about this.
Thank you again and sorry for not exposing clearly all my thoughts before!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1198946193
I found many code changes of this PR relies on #14044. @nodece Could you help verify whether could this PR cherry-picked into branch-2.8?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #16650: [improve][authentication] Support for get token from HTTP params
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16650:
URL: https://github.com/apache/pulsar/pull/16650#issuecomment-1200335381
Move the `release/2.8.4` label to https://github.com/apache/pulsar/pull/16871
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org