Skip to content

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

Merged
nodece merged 1 commit into
apache:masterfrom
nodece:fix-http-auth
Jan 14, 2025
Merged

[fix][broker] Continue using the next provider for http authentication if one fails#23842
nodece merged 1 commit into
apache:masterfrom
nodece:fix-http-auth

Conversation

@nodece

@nodece nodece commented Jan 13, 2025

Copy link
Copy Markdown
Member

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

@dao-jun dao-jun left a comment

Copy link
Copy Markdown
Member

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
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
poorbarcode pushed a commit to poorbarcode/pulsar that referenced this pull request Jan 23, 2025
…n if one fails (apache#23842)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jan 31, 2025
…n if one fails (apache#23842)

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

(cherry picked from commit f1f65a5)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 8181177)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Feb 3, 2025
…n if one fails (apache#23842)

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

(cherry picked from commit f1f65a5)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 8181177)
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
…n if one fails (apache#23842)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
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.

3 participants