PIP-55: Refresh Authentication Credentials #6074

merlimat · 2020-01-17T05:06:34Z

Motivation

Implemented the authentication refreshing functionality.

Notes:

Works with mutual auth providers
Works through Pulsar proxy (when client credentials are forwarded to broker)

jerrypeng · 2020-01-17T05:14:46Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

+    // In case of proxy, if the authentication credentials are forwardable,
+    // it will hold the credentials of the original client
+    AuthenticationState originalAuthState;
+    private boolean pendingAuthChallengeResponse = false;


I think this needs to be marked volatile?

This variable will be always accessed only the connection IO thread so there won't be any race condition.

jerrypeng · 2020-01-17T05:17:19Z

@merlimat awesome feature! Overall LGTM. One small comment.

sijie · 2020-01-17T05:55:57Z

@jiazhai can you review this change since you were implementing SASL authentication before?

…h proxy

ivankelly

One comment needs to be updated. Otherwise looks good.

ivankelly · 2020-01-23T08:11:34Z

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java

+            if (jwt.getBody().getExpiration() != null) {
+                this.expiration = jwt.getBody().getExpiration().getTime();
+            } else {
+                this.expiration = 0;


nit: Why not set to Long.MAX_VALUE and make the check simpler?

ivankelly · 2020-01-23T08:17:20Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarChannelInitializer.java

    private final NettySslContextBuilder sslCtxRefresher;
    private final ServiceConfiguration brokerConf;

+    private final Cache<SocketAddress, ServerCnx> connections = Caffeine.newBuilder()


nit: Why not Cache<ServerCnx, ServerCnx> and avoid possibly allocating the SocketAddress? We don't care about the keys in any case. Maybe add a comment that the you're using Cache to avoid having to clean stuff up.

The socket address is already kept in the ServerCnx itself, so it will not be a new allocation here.

Added comment

ivankelly · 2020-01-23T08:20:53Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PulsarChannelInitializer.java

+            try {
+                cnx.refreshAuthenticationCredentials();
+            } catch (Throwable t) {
+                log.warn("[{}] Failed to refresh auth credentials", cnx.getRemoteAddress());


Since you're using Cache to hold the connections, and the lifetime of the entry in connections is based on whether it's been GC'd, this will likely spam the logs. Maybe only log if after throwing, the cnx is still in connected state.

When we get the ref to cnx, it will be a strong ref to it, so we're not going to loose it during the iteration. Then, inside that method, i'm checking for

if (getState() != State.Connected || !isActive) { // Connection is either still being established or already closed. return; }

to avoid getting errors on a connections that's already gone

ivankelly · 2020-01-23T08:27:14Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

+        String authRole = useOriginalAuthState ? originalPrincipal : this.authRole;
        AuthData brokerData = authState.authenticate(clientData);
+
        // authentication has completed, will send newConnected command.


This comment is now wrong. I think the flow should change a little here, to only call completeConnect if it's the initial auth. There seems to be enough difference in the flows to have them completely separate.

Fixed the comment and tried to make the flow a bit clearer

ivankelly · 2020-01-23T08:30:22Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

            authState = authenticationProvider.newAuthState(clientData, remoteAddress, sslSession);
            authenticationData = authState.getAuthDataSource();
-            doAuthentication(clientData, clientProtocolVersion, clientVersion);
+            state = doAuthentication(clientData, clientProtocolVersion, clientVersion);


not for this patch, but it seems strange that this is a synchronous call, given that authentication could be calling out to a remote system.

Yes, initially all auth methods were completely stateless and therefore didn't require any asynchronousness in the provider API. This is not true now in general and we'll have to make a bit of a refactor to achieve that.

ivankelly · 2020-01-23T08:35:56Z

pulsar-common/src/main/proto/PulsarApi.proto

+    optional FeatureFlags feature_flags = 10;
+}
+
+message FeatureFlags {


why a feature flag rather than bumping the version? Because some auth will not support reauth?

Bumping the version is a very primitive way of checking the supported features. It worked well when the client implementation was only Java and started to be problematic after that.

The reality is that all the clients out there will have a different set of supported features. Also, if someone just updates thePulsarApi.proto it looks like that client is now at the latest version, while in reality it might not be able to understand a specific command.

Exchanging the list of supported features will allow client to selectively implement features.

adding FeatureFlags makes sense. 👍

rdhabalia

let's also close #3705 once this PR is merged. No one reviewed that PR when it was introduced last year when we didn't have other auth-mechanism implemented.

rdhabalia · 2020-01-24T21:46:24Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

+        category = CATEGORY_AUTHENTICATION,
+        doc = "Interval of time for checking for expired authentication credentials"
+    )
+    private int authenticationRefreshCheckSeconds = 60;


we should disable it by default.

It will "disabled by default" in the sense that the current credentials are not expiring as of now.

rdhabalia · 2020-01-24T22:00:51Z

pulsar-common/src/main/proto/PulsarApi.proto

+    optional FeatureFlags feature_flags = 10;
+}
+
+message FeatureFlags {


adding FeatureFlags makes sense. 👍

rdhabalia · 2020-01-24T22:07:57Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

+
+            if (!supportsAuthenticationRefresh()) {
+                log.warn("[{}] Closing connection because client doesn't support auth credentials refresh", remoteAddress);
+                ctx.close();


if client doesn't support then let's not change the behavior and don't close the connection, because closing connection will interrupt the client side processing and it requires them to upgrade client lib immediately if client wants to fix auto disconnect. So, we don't want our client to complain for such changed behavior which requires them to upgrade client lib.

If we don't close the connection for older clients, it becomes a security loophole.

Also, keep in mind that this is only apply if the credentials are expiring. In that case, a client will typically be reading the credentials each time are needed (eg: from a file or through a supplier function).
If that was not the case, it would have trouble even without this change. (eg: when a broker gets restarted it will not be able to reconnect anymore).

After the connection gets closed, an older client will be anyway able to immediately reconnect and refresh the authentication.

this change will not work for us because we have usecase where application creates large number of producers on one connection and if the client library is not upgraded then it closes the connection and disrupts all producers connection. this application is latency sensitive and we would like to avoid reconnection.

if the client library is not upgraded then it closes the connection and disrupts all producers connection.

This is only if the credentials are expiring and if the authentication provider enforces that (which right now will only be the token auth provider).

Avoiding to challenge older client defeats the purpose of this feature in that it opens a backdoor to completely avoid the check.

If older clients are a concern, I'd suggest to either:

not use this feature (or similarly working feature)

use non-expiring credentials for older clients

use credentials with long expire time to minimize the number of reconnections

force clients to upgrade version

I honestly don't see any better solution for that. I'd categorically exclude the "ignore old clients" for the false sense of security and the massive sec issue it opens.

sijie · 2020-02-04T01:52:09Z

@jerrypeng @rdhabalia can you review this pull request again? Matteo has addressed your comments.

sijie · 2020-02-10T07:56:23Z

@rdhabalia any thoughts on Matteo's last comment?

merlimat · 2020-02-10T16:58:57Z

@rdhabalia any thoughts on Matteo's last comment?

@sijie I think he's on vacation now. I'll sync-up with him once he's back.

tuteng · 2020-03-21T04:30:12Z

Add label release-2.5.1, due to #6178 dependency

* PIP-55: Refresh Authentication Credentials * Fixed import order * Do not check for original client credential if it's not coming through proxy * Fixed import order * Fixed mocked test assumption * Addressed comments * Avoid to print NPE on auth refresh check if auth is disabled (cherry picked from commit 4af5223)

This PR is to update docs for PIP-55: #6074 ### Motivation provide general doc description about implementing the authentication refreshing functionality. ### Modifications Update the Security overview for PIP 55. the `authenticationRefreshCheckSeconds` config has been added through the PR: #6074

Motivation This doc PR is updated for configurations for PRs: #6716 #6853 #6074 1: The broker configuration (for #6716) is updated by Jia Zhai. 2: Add other supported configurations to the client, standlone and proxy configuration docs based on the client.config, standlone.config and proxy.config files. Modifications 1: Add TLS with keystore type config in standlone and proxy configuration file. 2: update reference > pulsar configuration > client for PIP-55: Refresh Authentication Credentials Add other supported configurations to the standlone and proxy configuration files based on the standlone.config and proxy.config files.

This PR is to update docs for PIP-55: #6074 ### Motivation provide general doc description about implementing the authentication refreshing functionality. ### Modifications Update the Security overview for PIP 55. the `authenticationRefreshCheckSeconds` config has been added through the PR: #6074 (cherry picked from commit 29b81ed)

* PIP-55: Refresh Authentication Credentials * Fixed import order * Do not check for original client credential if it's not coming through proxy * Fixed import order * Fixed mocked test assumption * Addressed comments * Avoid to print NPE on auth refresh check if auth is disabled

This PR is to update docs for PIP-55: apache#6074 ### Motivation provide general doc description about implementing the authentication refreshing functionality. ### Modifications Update the Security overview for PIP 55. the `authenticationRefreshCheckSeconds` config has been added through the PR: apache#6074

Motivation This doc PR is updated for configurations for PRs: apache#6716 apache#6853 apache#6074 1: The broker configuration (for apache#6716) is updated by Jia Zhai. 2: Add other supported configurations to the client, standlone and proxy configuration docs based on the client.config, standlone.config and proxy.config files. Modifications 1: Add TLS with keystore type config in standlone and proxy configuration file. 2: update reference > pulsar configuration > client for PIP-55: Refresh Authentication Credentials Add other supported configurations to the standlone and proxy configuration files based on the standlone.config and proxy.config files.

PIP-55: Refresh Authentication Credentials

0c356d6

merlimat added the type/feature The PR added a new feature or issue requested a new feature label Jan 17, 2020

merlimat added this to the 2.6.0 milestone Jan 17, 2020

merlimat requested review from ivankelly, jerrypeng, rdhabalia, sijie and srkukarni January 17, 2020 05:06

merlimat self-assigned this Jan 17, 2020

jerrypeng reviewed Jan 17, 2020

View reviewed changes

Fixed import order

c198308

sijie requested a review from jiazhai January 17, 2020 05:55

Do not check for original client credential if it's not coming throug…

1dd0fb7

…h proxy

merlimat mentioned this pull request Jan 17, 2020

PIP-55: Refresh auth credentials apache/pulsar-client-go#176

Merged

merlimat force-pushed the token-refresh-auth-master branch from 31fc3c2 to f959116 Compare January 17, 2020 17:52

Fixed import order

d07a926

merlimat force-pushed the token-refresh-auth-master branch from f959116 to d07a926 Compare January 17, 2020 19:29

Fixed mocked test assumption

e5e1fbb

jiazhai approved these changes Jan 22, 2020

View reviewed changes

ivankelly requested changes Jan 23, 2020

View reviewed changes

Addressed comments

d8e8854

rdhabalia reviewed Jan 24, 2020

View reviewed changes

ivankelly approved these changes Jan 27, 2020

View reviewed changes

sijie added the doc-required Your PR changes impact docs and you will update later. label Feb 4, 2020

jerrypeng approved these changes Feb 5, 2020

View reviewed changes

Avoid to print NPE on auth refresh check if auth is disabled

e10b038

merlimat merged commit 4af5223 into apache:master Feb 10, 2020

tuteng added the release/2.5.1 label Mar 21, 2020

This was referenced Jun 11, 2020

[Doc]-Precise topic publish rate limiting #7244

Closed

[PIP-55][Doc]--Update security overview #7302

Merged

Huanli-Meng mentioned this pull request Jul 9, 2020

[Doc]-Precise topic publish rate limiting #7491

Merged

jiazhai mentioned this pull request Nov 16, 2020

Support Token refresh for Token Provider and client streamnative/pulsar-archived#1687

Closed

cbornet mentioned this pull request Dec 10, 2020

Support credentials refreshing (PIP 55) cbornet/pulsar-grpc#4

Open

Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Feb 21, 2022

PIP-55: Refresh Authentication Credentials #6074

PIP-55: Refresh Authentication Credentials #6074

Uh oh!

Conversation

merlimat commented Jan 17, 2020

Motivation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerrypeng commented Jan 17, 2020

Uh oh!

sijie commented Jan 17, 2020

Uh oh!

ivankelly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdhabalia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sijie commented Feb 4, 2020

Uh oh!

sijie commented Feb 10, 2020

Uh oh!

merlimat commented Feb 10, 2020

Uh oh!

tuteng commented Mar 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants