PIP-30: interface and mutual change authentication #3677

jiazhai · 2019-02-23T13:09:24Z

Motivation

This is to implement the mutual auth api discussed in PIP-30: change authentication provider API to support mutual authentication

Modifications

Describe the modifications you've done.

Verifying this change

jiazhai · 2019-02-23T13:10:11Z

@eolivelli would you please also help look at this. Thanks.

eolivelli · 2019-02-23T13:23:46Z

Sure

eolivelli

Good work.
I left a couple of suggestions

...r-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationDataSource.java

...broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java

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

pulsar-common/src/main/java/org/apache/pulsar/common/api/Commands.java

sijie

overall looks good. left a few comments around the interface. I think there are still improvements to make the interface cleaner.

...broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java

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

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java

eolivelli

I will take another look tomorrow.
What about the http API ?

ivankelly

Looks good in general. Requested a few changes.

...broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java

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

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

jiazhai · 2019-03-01T09:16:56Z

run integration tests

jiazhai · 2019-03-01T09:17:42Z

@eolivelli @ivankelly updated, Would you please help review again.

eolivelli

We are close to be ready.
I left a couple of comments.

...r-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationDataSource.java

...oker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java

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

pulsar-client-api/src/main/java/org/apache/pulsar/common/api/AuthData.java

jiazhai · 2019-03-01T13:01:45Z

@eolivelli Thanks for the comments, updated again.

ivankelly

I haven't reviewed fully, but one thing I saw quickly was that the challenge goes from the client to the broker. It should be the other way around. The client initiates the connection, the server challenges the client and the client responds to the server's challenge.

jiazhai · 2019-03-02T05:02:01Z

@ivankelly Thanks, updated.

jiazhai · 2019-03-02T13:15:04Z

run java8 tests

ivankelly · 2019-03-04T11:08:15Z

@jiazhai added @jerrypeng as he's been making some other auth changes lately.

...ker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java

...oker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java

...broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java

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

...broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java

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

pulsar-common/src/main/java/org/apache/pulsar/common/api/Commands.java

jiazhai · 2019-03-04T13:49:49Z

Thanks @ivankelly , updated it again.

jiazhai · 2019-03-05T06:22:59Z

run java8 tests

jiazhai · 2019-03-05T08:04:26Z

run java8 tests

for error:
org.apache.pulsar.client.api.BrokerServiceLookupTest.testWebserviceServiceTls

ivankelly

This change is missing unit tests. It should be fairly easy to mock up a AuthProvider to do multi stage auth.

ivankelly · 2019-03-05T09:44:37Z

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

+    private void doingAuthentication(AuthData clientData,
+                                     int clientProtocolVersion,
+                                     String clientVersion) throws Exception {
+        AuthData brokerData = authState.authenticate(clientData);


yes, what I find strange is that we make a call to mutate authState, and then immediately query authState to see what that mutation resulted in. It feels like this would be better done in a single call, especially if multiple threads come into play at a later date.

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

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

pulsar-common/src/main/java/org/apache/pulsar/common/api/Commands.java

jiazhai · 2019-03-06T09:01:40Z

@ivankelly updated again. would you please help review it again

ivankelly

On the client side, where does it get the initial data to sent in the ConnectCommand?

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

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java

...ker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java

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

pulsar-broker/src/test/java/org/apache/pulsar/client/api/MutualAuthenticationTest.java

jiazhai · 2019-03-06T13:42:02Z

@ivankelly Thanks for the comments. update it again.
as you mentioned, "init" is set when newConnectCommand in client side. in Kerberos, it is null. Here it just indicate that this is the start.

jiazhai · 2019-03-07T01:38:54Z

rerun java8 tests

jiazhai · 2019-03-07T06:52:50Z

rerun java8 tests

sijie · 2019-03-08T16:53:21Z

@ivankelly @eolivelli can you guys please review this again? we have to push this forward to unblock subsequent changes.

...ker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java

...broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java

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

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java

pulsar-broker/src/test/java/org/apache/pulsar/client/api/MutualAuthenticationTest.java

jiazhai · 2019-03-12T05:18:35Z

@ivankelly Thanks for the comments, updated

jiazhai · 2019-03-12T08:07:04Z

run integration tests
run java8 tests

org.apache.pulsar.broker.admin.PersistentTopicsTest.cleanup


org.apache.pulsar.tests.integration.functions.PulsarFunctionsThreadTest.pulsar-test-suite

ivankelly

One small comment. Otherwise it's ready to go.

pulsar-client-api/src/main/java/org/apache/pulsar/common/api/AuthData.java

ivankelly

lgtm. good work Jia!

Fixes #3652 **Motivation** Currently both Zookeeper and BookKeeper could be secured by using Kerberos, It would be good to support Kerberos in Pulsar Broker and Client. This is the sub-task for issue in #3491 to support Kerberos in Pulsar Broker and Client. Will add proxy and web resource support in following prs. The Kerberos authentication is similar to that in Zookeeper and BookKeeper, which leverage SASL and GSSAPI, so reused some of the code there. PR #3658 is the first version of PR before #3677 . **Changes** provide both client and broker side support for authentication api; add unit test.

jiazhai requested review from merlimat and sijie February 23, 2019 13:09

eolivelli requested changes Feb 23, 2019

View reviewed changes

sijie reviewed Feb 25, 2019

View reviewed changes

jiazhai changed the title ~~[WIP]PIP-30: interface and mutual change authentication~~ PIP-30: interface and mutual change authentication Feb 27, 2019

eolivelli reviewed Feb 27, 2019

View reviewed changes

ivankelly requested changes Feb 28, 2019

View reviewed changes

eolivelli requested changes Mar 1, 2019

View reviewed changes

ivankelly requested changes Mar 1, 2019

View reviewed changes

jiazhai force-pushed the pip_auth branch from 6291d9f to 9312473 Compare March 2, 2019 06:23

ivankelly requested a review from jerrypeng March 4, 2019 11:07

ivankelly requested changes Mar 4, 2019

View reviewed changes

jiazhai force-pushed the pip_auth branch from 6aebe6b to d7e04e2 Compare March 5, 2019 01:28

ivankelly requested changes Mar 5, 2019

View reviewed changes

ivankelly requested changes Mar 6, 2019

View reviewed changes

jiazhai added 2 commits March 6, 2019 22:47

interface and mutual authn

1ae426e

align code in client/server.cnx

f40b5fc

jiazhai added 5 commits March 6, 2019 22:47

change following enrico's comments

b33791e

change following ivan's comments

7c6ffae

change follow ivan's comments

22c5d98

change following ivan's comments

b639edd

change following ivan's comments

aa162d2

jiazhai force-pushed the pip_auth branch from 0f911a2 to aa162d2 Compare March 6, 2019 14:47

ivankelly requested changes Mar 11, 2019

View reviewed changes

change follow ivan's comments

61bdfee

ivankelly requested changes Mar 12, 2019

View reviewed changes

pulsar-client-api/src/main/java/org/apache/pulsar/common/api/AuthData.java Outdated Show resolved Hide resolved

change following ivan's comments

6edd19b

ivankelly approved these changes Mar 12, 2019

View reviewed changes

sijie assigned jiazhai Mar 12, 2019

sijie added area/client type/feature The PR added a new feature or issue requested a new feature area/broker area/security labels Mar 12, 2019

sijie added this to the 2.4.0 milestone Mar 12, 2019

sijie approved these changes Mar 12, 2019

View reviewed changes

jiazhai merged commit 09e3ed8 into apache:master Mar 13, 2019

This was referenced Mar 14, 2019

[WIP]kerberos authentication between client and broker #3658

Closed

kerberos: authentication between client and broker #3821

Merged

rdhabalia mentioned this pull request Oct 24, 2019

[pulsar-broker] fix broken custom auth-provider that uses authenticationData #5462

Merged

klaatu01 mentioned this pull request Feb 8, 2023

Add support for on-the-fly AuthChallenge streamnative/pulsar-rs#256

Merged

michaeljmarshall mentioned this pull request Feb 24, 2023

[fix][broker] Only send AuthChallenge when client supports it #19626

Draft

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIP-30: interface and mutual change authentication #3677

PIP-30: interface and mutual change authentication #3677

jiazhai commented Feb 23, 2019 •

edited

Loading

jiazhai commented Feb 23, 2019

eolivelli commented Feb 23, 2019

eolivelli left a comment

sijie left a comment

eolivelli left a comment

ivankelly left a comment

jiazhai commented Mar 1, 2019

jiazhai commented Mar 1, 2019

eolivelli left a comment

jiazhai commented Mar 1, 2019

ivankelly left a comment

jiazhai commented Mar 2, 2019

jiazhai commented Mar 2, 2019

ivankelly commented Mar 4, 2019

jiazhai commented Mar 4, 2019

jiazhai commented Mar 5, 2019

jiazhai commented Mar 5, 2019 •

edited

Loading

ivankelly left a comment

ivankelly Mar 5, 2019

jiazhai commented Mar 6, 2019

ivankelly left a comment

jiazhai commented Mar 6, 2019

jiazhai commented Mar 7, 2019

jiazhai commented Mar 7, 2019

sijie commented Mar 8, 2019

jiazhai commented Mar 12, 2019

jiazhai commented Mar 12, 2019 •

edited

Loading

ivankelly left a comment

ivankelly left a comment

PIP-30: interface and mutual change authentication #3677

PIP-30: interface and mutual change authentication #3677

Conversation

jiazhai commented Feb 23, 2019 • edited Loading

Motivation

Modifications

Verifying this change

jiazhai commented Feb 23, 2019

eolivelli commented Feb 23, 2019

eolivelli left a comment

Choose a reason for hiding this comment

sijie left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

ivankelly left a comment

Choose a reason for hiding this comment

jiazhai commented Mar 1, 2019

jiazhai commented Mar 1, 2019

eolivelli left a comment

Choose a reason for hiding this comment

jiazhai commented Mar 1, 2019

ivankelly left a comment

Choose a reason for hiding this comment

jiazhai commented Mar 2, 2019

jiazhai commented Mar 2, 2019

ivankelly commented Mar 4, 2019

jiazhai commented Mar 4, 2019

jiazhai commented Mar 5, 2019

jiazhai commented Mar 5, 2019 • edited Loading

ivankelly left a comment

Choose a reason for hiding this comment

ivankelly Mar 5, 2019

Choose a reason for hiding this comment

jiazhai commented Mar 6, 2019

ivankelly left a comment

Choose a reason for hiding this comment

jiazhai commented Mar 6, 2019

jiazhai commented Mar 7, 2019

jiazhai commented Mar 7, 2019

sijie commented Mar 8, 2019

jiazhai commented Mar 12, 2019

jiazhai commented Mar 12, 2019 • edited Loading

ivankelly left a comment

Choose a reason for hiding this comment

ivankelly left a comment

Choose a reason for hiding this comment

jiazhai commented Feb 23, 2019 •

edited

Loading

jiazhai commented Mar 5, 2019 •

edited

Loading

jiazhai commented Mar 12, 2019 •

edited

Loading