-
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
Determine Correct Expansion of AuthenticationProvider interface for WebSocket Proxy #20237
Labels
Comments
1 task
michaeljmarshall
added a commit
that referenced
this issue
May 8, 2023
Fixes #20236 PIP: #19409 ### Motivation In the `AuthenticationService`, we are currently using the deprecated `authenticate` methods. As a result, we hit the `Not Implemented` exception when using the `AuthenticationProviderOpenID`. This PR updates the implementation so that we're able This solution isn't ideal for two reasons. 1. We are not using the `authenticationHttpRequest` method, which seems like the right method for the WebSocket proxy. However, this is not a viable option, as I documented in #20237. 2. We are calling `.get()` on a future. However, it is expected that the `AuthenticationProvider` not block forever, so I think this is acceptable for now. Please let me know if you disagree. ### Modifications * Replace `authenticate` with `authenticateAsync`. ### Verifying this change This change is a trivial rework / code cleanup without any test coverage. ### Documentation - [x] `doc-not-needed` Note that I do have documentation showing that 3.0.x does not support OIDC in the WebSocket Proxy. The `next` docs don't need that limitation since this PR fixes that and targets 3.1.0. apache/pulsar-site#558 ### Matching PR in forked repository PR in forked repository: skipping for this trivial PR
michaeljmarshall
added a commit
to datastax/pulsar
that referenced
this issue
May 8, 2023
Fixes apache#20236 PIP: apache#19409 ### Motivation In the `AuthenticationService`, we are currently using the deprecated `authenticate` methods. As a result, we hit the `Not Implemented` exception when using the `AuthenticationProviderOpenID`. This PR updates the implementation so that we're able This solution isn't ideal for two reasons. 1. We are not using the `authenticationHttpRequest` method, which seems like the right method for the WebSocket proxy. However, this is not a viable option, as I documented in apache#20237. 2. We are calling `.get()` on a future. However, it is expected that the `AuthenticationProvider` not block forever, so I think this is acceptable for now. Please let me know if you disagree. ### Modifications * Replace `authenticate` with `authenticateAsync`. ### Verifying this change This change is a trivial rework / code cleanup without any test coverage. ### Documentation - [x] `doc-not-needed` Note that I do have documentation showing that 3.0.x does not support OIDC in the WebSocket Proxy. The `next` docs don't need that limitation since this PR fixes that and targets 3.1.0. apache/pulsar-site#558 ### Matching PR in forked repository PR in forked repository: skipping for this trivial PR (cherry picked from commit 03dc3db)
The issue had no activity for 30 days, mark with Stale label. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Problem
The current
AuthenticationProvider
interface has the following method:pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
Lines 158 to 174 in fb7f14c
The
HttpServletResponse
appears to have been introduced for multi-stage http authentication, which is only used by theAuthenticationProviderSasl
:pulsar/pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java
Lines 229 to 313 in 82237d3
However, the WebSocket Proxy does not have access to an
HttpServletResponse
. It only has access to aServletUpgradeResponse
, as seen here:pulsar/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/AbstractWebSocketHandler.java
Lines 91 to 122 in 516437e
Therefore, the current API does not allow for multi-phased http authentication.
Observations
ServletUpgradeResponse
is a wrapper forHttpServletResponse
, but it doesn't provide direct access. Does that mean we should add a new method to theAuthenticationProvider
interface just for the WebSocket?For now, the current state is that multi-stage auth is not supported in the WebSocket proxy. This technically aligns with the current limitation that multi-stage auth is not available in the regular pulsar proxy #19291.
The text was updated successfully, but these errors were encountered: