apache / apache/pulsar

Simplify authentication state management in Broker's ServerCnx class when networking with proxy

Open
#19,332 4 comments 0 reactions 0 assignees View on GitHub
Stale type/enhancement
Dominant language
Java
Stars
15.3k
Forks
3.8k
Avg merge
1d 14h
Merged PRs (30d)
160

Description

### Search before asking

- [X] I searched in the [issues](https://github.com/apache/pulsar/issues) and found nothing similar.

### Motivation

The current management is confusing and is not working correctly. We should improve it.

TL;DR: The values for `authState` and `authenticationData` are ambiguous and can have different meanings depending on configuration and the lifecycle of the connection.

## Current State of Client/Proxy/Broker Protocol Interaction

Here are the current cases based on proxy configuration:

A. Proxy sends connect command with only `originalPrincipal` (Update: this type of connection is not eligible for refreshing authentication data)
B. Proxy sends connect command with `originalPrincipal` and `originalAuthData`

Here are the two kinds of connections a proxy opens with the broker:

1. `ProxyLookupRequests` - proxy handles lookup requests.
2. `ProxyConnectionToBroker` - proxy handles all of other requests.

In both states, the proxy first connects with the above connect command.

In case 1, the proxy always translates requests, and as such, `AuthChallenge` commands sent from broker to proxy are handled by the proxy. In case A, the proxy replies with its own `AuthData`. In case B, the proxy forwards the `AuthChallenge` to the client, gets the response, and forwards it to the broker.

In case 2, after the initial connection is established (and authenticated), the proxy forwards bytes between client and broker without inspection. As such, for both cases A and B, all `AuthChallenge` commands are sent to the client.

## The problem

The broker's `ServerCnx` class has the following variables:

In case A, the broker tracks variables named `authState`, `authRole`, `authenticationData`, and `originalPrincipal`. The only role that is meant for the client is the `originalPrincipal`. However, that is not how it actually works. In case 1, `authState`, `authRole`, `authenticationData` are all proxy values for the lifetime of the handler. In case 2, `authState`, `authRole`, `authenticationData` start out as proxy values but upon auth refresh, the values are updated to use the client's values. In order to get around some of the potential issues, we have the following code:

https://github.com/apache/pulsar/blob/b3432f4ed9c9c19eef4ed696253eb2c18ebbf59d/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L718-L720

However, this makes it hard to reason about the actual state because we're somewhat dependent on whether or not a refresh has taken place yet.

In case B, it is slightly more confusing. Case B2 is the same as case A2. ~However, case 1A started out with the proxy not forwarding auth data, but when the broker refreshes the auth data, the `AuthChallenge` is sent to the client, so now the auth data is sent to the broker anyway.~ That condition isn't possible. Note that B1 is handled by the proxy forwarding the auth challenge to the client.

## Consequences

The primary issues present come though when doing authorization.

## Other problems

The `AuthResponse` command does not indicate which entity supplied the auth data. As such, we don't actually know if we should update the "original" auth data or the proxy's auth data.

The current design does not allow for multistage authentication in certain cases.

### Solution

My proposed solution is to remove the `originalAuthState` and `originalAuthData` variables. We can still authenticate both the client's and the proxy's auth data, but we don't need to make it look like we're actively tracking the state for both the proxy and the client.

As a result, we'll be able to simplify the authorization logic.

### Alternatives

Update the protocol to let make it clearer who's `AuthData` we're receiving.

### Anything else?

_No response_

### Are you willing to submit a PR?

- [ ] I'm willing to submit a PR!

Contributor guide

Open the contributing guide

Research direction

Start with pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java, especially the authentication handling around lines 718-720, and trace the proxy/client cases described in the issue. Done means removing the proposed original authentication state and data variables while preserving authentication flows and simplifying authorization state handling.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
authentication, authorization, backend, networking
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.