-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[improve][authentication] Support for get token from HTTP params #16650
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
102b99b
to
10ad71a
Compare
You can use |
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 curl "http://localhost:8080/admin/v2/brokers/configuration?token=ASDFGH...." Thanks in advance! |
Build Pulsar requires JDK 17.
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 Do not know if this could be addressed easily, maybe checking in the But not in a "general" one, so maybe just check for 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 about this. Thank you again and sorry for not exposing clearly all my thoughts before! |
@acortes-okode Thanks for your explanation about security!
You are right, we should add a filter to check the WebSocket connection URL.
Hi @michaeljmarshall @lhotari, I think you will be interested in this, could you share your thoughts? |
…che#16650) (cherry picked from commit 822f897) Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@BewareMyPower I submitted #16871 for branch-2.8. |
Move the |
Move |
#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. |
Move |
Move |
Signed-off-by: Zixuan Liu nodeces@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, 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
HttpServletRequestWrapper
to support for get token from HTTP paramsDocumentation
doc-not-needed