Skip to content
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

Add support for OpenID Connect in WebSocket Proxy #20236

Closed
2 tasks done
michaeljmarshall opened this issue May 5, 2023 · 1 comment · Fixed by #20238
Closed
2 tasks done

Add support for OpenID Connect in WebSocket Proxy #20236

michaeljmarshall opened this issue May 5, 2023 · 1 comment · Fixed by #20238
Assignees
@michaeljmarshall
Labels
area/authn type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Comments

@michaeljmarshall
Copy link
Member

Search before asking

  • I searched in the issues and found nothing similar.

Motivation

With #19849, we have support for OpenID Connect Authentication. That feature is not yet available in the WebSocket proxy because PIP 97 #12105 has not yet been implemented for the WebSocket Proxy's code path.

Solution

Implement PIP 97 #12105 in the websocket proxy. Here is the problematic code:

pulsar/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/AbstractWebSocketHandler.java

Lines 102 to 107 in 516437e

if (authenticationState != null) {
authRole = service.getAuthenticationService()
.authenticateHttpRequest(request, authenticationState.getAuthDataSource());
} else {
authRole = service.getAuthenticationService().authenticateHttpRequest(request);
}

Alternatives

No response

Anything else?

I plan on working to implement this soon.

Are you willing to submit a PR?

  • I'm willing to submit a PR!
@michaeljmarshall michaeljmarshall added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/authn labels May 5, 2023
@michaeljmarshall
Copy link
Member Author

The error presents itself as:

2023-05-05T19:33:11,659+0000 [pulsar-websocket-web-1-7] DEBUG org.apache.pulsar.broker.authentication.AuthenticationService - Authentication failed for provider token: Not supported
javax.naming.AuthenticationException: Not supported

@michaeljmarshall michaeljmarshall self-assigned this May 5, 2023
@michaeljmarshall michaeljmarshall mentioned this issue May 5, 2023
Merged
1 task
michaeljmarshall added a commit that referenced this issue May 8, 2023
@michaeljmarshall
[feat][ws] Use async auth method to support OIDC (#20238)
03dc3db 
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
@michaeljmarshall
[feat][ws] Use async auth method to support OIDC (apache#20238)
02d6c16 
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@michaeljmarshall michaeljmarshall

Labels
area/authn type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[feat][ws] Use async auth method to support OIDC michaeljmarshall/pulsar
1 participant
@michaeljmarshall