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.

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

[fix][broker] Continue using the next provider for http authentication if one fails #23842

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

nodece
Copy link
Member

@nodece nodece commented Jan 13, 2025

Motivation

When using Pulsar admin version 2.10.x with JWT authentication and a broker running 3.0.x (forked from Apache Pulsar) configured with both Kerberos and JWT authentication providers, we encounter the following issue:

ERROR org.apache.pulsar.broker.web.AuthenticationFilter - [172.19.0.17] Error performing authentication for HTTP
2024-12-30 09:52:32 java.lang.IllegalStateException: Header token should exist if no role token.
2024-12-30 09:52:32     at com.google.common.base.Preconditions.checkState(Preconditions.java:512) ~[com.google.guava-guava-32.1.1-jre.jar:?]
2024-12-30 09:52:32     at org.apache.pulsar.broker.authentication.AuthenticationProviderSasl.authenticateHttpRequest(AuthenticationProviderSasl.java:275) ~[com.ascentstream.pulsar-pulsar-broker-auth-sasl-3.0.8.1.jar:3.0.8.1]
2024-12-30 09:52:32     at org.apache.pulsar.broker.authentication.AuthenticationService.authenticateHttpRequest(AuthenticationService.java:133) ~[com.ascentstream.pulsar-pulsar-broker-common-3.0.8.1.jar:3.0.8.1]
2024-12-30 09:52:32     at org.apache.pulsar.broker.web.AuthenticationFilter.doFilter(AuthenticationFilter.java:59) ~[com.ascentstream.pulsar-pulsar-broker-common-3.0.8.1.jar:3.0.8.1]
2024-12-30 09:52:32     at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:193) ~[org.eclipse.jetty-jetty-servlet-9.4.56.v20240826.jar:9.4.56.v20240826]
2024-12-30 09:52:32     at org.eclipse.jetty.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1626) ~[org.eclipse.jetty-jetty-servlet-9.4.56.v20240826.jar:9.4.56.v20240826]
2024-12-30 09:52:32     at org.apache.pulsar.broker.intercept.BrokerInterceptor.onFilter(BrokerInterceptor.java:224) ~[com.ascentstream.pulsar-pulsar-broker-3.0.8.1.jar:3.0.8.1]
2024-12-30 09:52:32     at org.apache.pulsar.broker.web.ProcessHandlerFilter.doFilter(ProcessHandlerFilter.java:46) ~[com.ascentstream.pulsar-pulsar-broker-3.0.8.1.jar:3.0.8.1]
2024-12-30 09:52:32     at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:193) ~[org.eclipse.jetty-jetty-servlet-9.4.56.v20240826.jar:9.4.56.v20240826]

When a request is without the authentication method name in the HTTP header(#14044 improves that), the broker iterates through each authentication provider to authenticate the request, if authentication data is valid, the broker acts on the request. In this scenario, I ensured that the JWT provider was configured correctly and the token was valid. However, I still encountered an authentication error.

The root cause is that the org.apache.pulsar.broker.authentication.AuthenticationService#authenticateHttpRequest(javax.servlet.http.HttpServletRequest, org.apache.pulsar.broker.authentication.AuthenticationDataSource) and org.apache.pulsar.broker.authentication.AuthenticationService#authenticateHttpRequest(javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse) only catches the AuthenticationExecption, if one provider throws an exception of a different type, the authentication process will be terminated.

Related to #23797, which fixes the pulsar chain authentication, not HTTP authentication.

Modifications

  • Use LinkedHashMap instead of HasMap for provider ordering.
  • Catch type Exception, which can catch any exceptions.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@nodece nodece self-assigned this Jan 13, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 13, 2025
@nodece nodece marked this pull request as draft January 13, 2025 06:32
…n if one fails

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece marked this pull request as ready for review January 13, 2025 08:35
@nodece nodece closed this Jan 13, 2025
@nodece nodece reopened this Jan 13, 2025
Copy link
Member

@dao-jun dao-jun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nodece nodece merged commit f1f65a5 into apache:master Jan 14, 2025
71 of 76 checks passed
nodece added a commit that referenced this pull request Jan 14, 2025
…n if one fails (#23842)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

(cherry picked from commit f1f65a5)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nodece added a commit that referenced this pull request Jan 14, 2025
…n if one fails (#23842)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit f1f65a5)
nodece added a commit that referenced this pull request Jan 14, 2025
…n if one fails (#23842)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit f1f65a5)
@nodece nodece deleted the fix-http-auth branch January 14, 2025 08:34
@nodece nodece added this to the 4.1.0 milestone Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants