You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2018/02/21 02:03:14 UTC

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB 

Hello Todd Lipcon,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/9374

to review the following change.


Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................

KUDU-2259: add real user to AuthenticationCredentialsPB

WIP: the Java client needs a similar change

This commit adds the 'real user' to the authn credentials token, which
is used when negotiating connections with SASL PLAIN authentication.
This is useful when scan tokens are being sent to remote tasks, it's not
possible to authenticate with a signed authn token to the remote
server[1], coarse-grained ACLs have been set, and the 'planner' and
'executor' processes are being run with different users.

[1]: this most often occurs because the remote server has encryption disabled.

Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.proto
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/user_credentials.cc
M src/kudu/rpc/user_credentials.h
M src/kudu/util/user.cc
14 files changed, 102 insertions(+), 10 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/9374/1
-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9374 )

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................

KUDU-2259: add real user to AuthenticationCredentialsPB

This commit adds the 'real user' to the authn credentials token, which
is used when negotiating connections with SASL PLAIN authentication.
This is useful when scan tokens are being sent to remote tasks, it's not
possible to authenticate with a signed authn token to the remote
server[1], coarse-grained ACLs have been set, and the 'planner' and
'executor' processes are being run with different users.

This problematic scenario might also have been solved by allowing tokens
to be used in all scenarios, even when encryption is disabled, but the
approach taken by this commit allows that invariant to remain.

[1]: this most often occurs because the remote server has encryption disabled.

Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Reviewed-on: http://gerrit.cloudera.org:8080/9374
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.proto
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/user_credentials.cc
M src/kudu/rpc/user_credentials.h
M src/kudu/util/user.cc
17 files changed, 236 insertions(+), 34 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9374 )

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................


Patch Set 3: Code-Review+1

(2 comments)

lgtm, just some whitespace nits to prevent future merge conflict pain

http://gerrit.cloudera.org:8080/#/c/9374/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:

http://gerrit.cloudera.org:8080/#/c/9374/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@155
PS3, Line 155:                 "Caller is responsible for refreshing credentials.",
nit: goofy re-indentation here


http://gerrit.cloudera.org:8080/#/c/9374/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@204
PS3, Line 204:               "before expiration.");
same (and same a few places below)



-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 14 Mar 2018 23:55:45 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9374 )

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................


Patch Set 3:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@156
PS2, Line 156: getKerbero
> Not sure I understand: the condition for this 'if' closure is 'pb.hasAuthnT
I misunderstood, thanks for pointing that out.


http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java:

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java@73
PS2, Line 73:     try (KuduClient client =
> wrap
Done


http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java@77
PS2, Line 77: 
> yea, probably.... guess it's worth a JIRA
Done


http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java@82
PS2, Line 82:     // Try again with a correct real user.
> mind wrapping this?
Done


http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-internal.h@233
PS2, Line 233:   // is built.
> can you comment that this never changes after construction? ie it's effecti
Done


http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-test.cc@5359
PS2, Line 5359: 
> ASSERT_OK ?
Done


http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-test.cc@5362
PS2, Line 5362:   table_creator->table_name(kTableName)
> yea seems like probably should.  maybe put this in the same JIRA as the oth
Done


http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client.proto
File src/kudu/client/client.proto:

http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client.proto@116
PS2, Line 116: root
> nit: if I'm not mistaken, some time ago there was an idea to keep whole cer
Isn't that kind of redundant?  I thought with PKI you typically just trust root certs, and then intermediate / leaf certs are required to ship around any intermediate signing certs with themselves.



-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 13 Mar 2018 19:04:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9374 )

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................


Patch Set 4: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 15 Mar 2018 00:39:21 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9374 )

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................


Patch Set 2:

(4 comments)

I'm still unsure about some of the changes in this PR from a design-perspective, and I think there's a non-zero chance that it may have unintended side-effects, so I think we should hold off on getting it in for the immediately upcoming release.

My design hesitations are around the fact that it potentially duplicates the real-user field in AuthenticationCredentialsPB when the token is set: the new 'realUser' field and the signed token could have different users.  Should we only set one or the other?  I'm also somewhat struggling to come up with a coherent high-level explanation for this change.

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@61
PS2, Line 61:   private String realUser;
> probably worth a comment.
I think this is a typo, it should indeed never be null.


http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@117
PS2, Line 117: public synchronized byte[] exportAuthenticationCredentials() {
> It seems the previous version returned null if either authn token or certs 
Yes, that's true.  Even though the implementation has changed to never return null, I didn't want to change the actual interface, since changing the guarantee later would be more difficult.


http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@156
PS2, Line 156: authnToken
> What if authnToken was null?
the hasAuthnToke() check takes care of that.


http://gerrit.cloudera.org:8080/#/c/9374/1/src/kudu/client/client.proto
File src/kudu/client/client.proto:

http://gerrit.cloudera.org:8080/#/c/9374/1/src/kudu/client/client.proto@116
PS1, Line 116: root 
> nit: IIRC, some time ago there was an idea to put the whole certificate cha
Is that in the case of external PKI?  With internal-PKI I don't think we ever really have a 'chain', or at least there are no intermediates.



-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 02 Mar 2018 18:43:08 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9374 )

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@61
PS2, Line 61:   private String realUser;
probably worth a comment.

Why is this nullable, considering it's initialized with the system property user.name? Is it possible that is null? If it were null would other stuff just crash later anyway with an NPE?


http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java:

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java@73
PS2, Line 73:     try (KuduClient client = new KuduClient.KuduClientBuilder(miniCluster.getMasterAddresses()).build()) {
wrap


http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java@77
PS2, Line 77: NotAuthorized
yea, probably.... guess it's worth a JIRA


http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java@82
PS2, Line 82:     try (KuduClient client = new KuduClient.KuduClientBuilder(miniCluster.getMasterAddresses()).build()) {
mind wrapping this?


http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-internal.h@233
PS2, Line 233:   rpc::UserCredentials user_credentials_;
can you comment that this never changes after construction? ie it's effectively const? was a bit nervous about thread-safety but that appears to be the case, right?


http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-test.cc@5362
PS2, Line 5362:   // TODO(dan): should this be NotAuthorized?
yea seems like probably should.  maybe put this in the same JIRA as the other one in the Java side?



-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 23 Feb 2018 20:49:17 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9374 )

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................


Patch Set 4:

Looks like a flake, but just to be sure I retriggered.


-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 15 Mar 2018 00:53:54 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/9374

to look at the new patch set (#3).

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................

KUDU-2259: add real user to AuthenticationCredentialsPB

This commit adds the 'real user' to the authn credentials token, which
is used when negotiating connections with SASL PLAIN authentication.
This is useful when scan tokens are being sent to remote tasks, it's not
possible to authenticate with a signed authn token to the remote
server[1], coarse-grained ACLs have been set, and the 'planner' and
'executor' processes are being run with different users.

This problematic scenario might also have been solved by allowing tokens
to be used in all scenarios, even when encryption is disabled, but the
approach taken by this commit allows that invariant to remain.

[1]: this most often occurs because the remote server has encryption disabled.

Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.proto
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/user_credentials.cc
M src/kudu/rpc/user_credentials.h
M src/kudu/util/user.cc
17 files changed, 243 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/9374/3
-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9374 )

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@156
PS2, Line 156: authentica
> the hasAuthnToke() check takes care of that.
Not sure I understand: the condition for this 'if' closure is 'pb.hasAuthnToken()', which comes from input bytes 'authnData'.  Meanwhile, 'authnToken' is a member of the class which might be null at this point, no?


http://gerrit.cloudera.org:8080/#/c/9374/1/src/kudu/client/client.proto
File src/kudu/client/client.proto:

http://gerrit.cloudera.org:8080/#/c/9374/1/src/kudu/client/client.proto@116
PS1, Line 116: root 
> Is that in the case of external PKI?  With internal-PKI I don't think we ev
Yes, the idea was to use the sequence for the certificate chain, if we had that.  The chain is needed for validation.

As of now, there is no multiple root CA certificates.



-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 02 Mar 2018 19:05:46 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/9374

to look at the new patch set (#2).

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................

KUDU-2259: add real user to AuthenticationCredentialsPB

This commit adds the 'real user' to the authn credentials token, which
is used when negotiating connections with SASL PLAIN authentication.
This is useful when scan tokens are being sent to remote tasks, it's not
possible to authenticate with a signed authn token to the remote
server[1], coarse-grained ACLs have been set, and the 'planner' and
'executor' processes are being run with different users.

[1]: this most often occurs because the remote server has encryption disabled.

Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.proto
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/user_credentials.cc
M src/kudu/rpc/user_credentials.h
M src/kudu/util/user.cc
17 files changed, 233 insertions(+), 33 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/9374/2
-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9374 )

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9374/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:

http://gerrit.cloudera.org:8080/#/c/9374/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@155
PS3, Line 155:                 "Caller is responsible for refreshing credentials.",
> nit: goofy re-indentation here
Done


http://gerrit.cloudera.org:8080/#/c/9374/3/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@204
PS3, Line 204:               "before expiration.");
> same (and same a few places below)
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 15 Mar 2018 00:18:34 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/9374

to look at the new patch set (#4).

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................

KUDU-2259: add real user to AuthenticationCredentialsPB

This commit adds the 'real user' to the authn credentials token, which
is used when negotiating connections with SASL PLAIN authentication.
This is useful when scan tokens are being sent to remote tasks, it's not
possible to authenticate with a signed authn token to the remote
server[1], coarse-grained ACLs have been set, and the 'planner' and
'executor' processes are being run with different users.

This problematic scenario might also have been solved by allowing tokens
to be used in all scenarios, even when encryption is disabled, but the
approach taken by this commit allows that invariant to remain.

[1]: this most often occurs because the remote server has encryption disabled.

Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurityContextRealUser.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.proto
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/user_credentials.cc
M src/kudu/rpc/user_credentials.h
M src/kudu/util/user.cc
17 files changed, 236 insertions(+), 34 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/9374/4
-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9374 )

Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java
File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java:

http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@117
PS2, Line 117: public synchronized byte[] exportAuthenticationCredentials() {
It seems the previous version returned null if either authn token or certs were missing.  The new version always returns non-null.  Is it necessary to update call sites on this methos in that regard?


http://gerrit.cloudera.org:8080/#/c/9374/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@156
PS2, Line 156: authnToken
What if authnToken was null?


http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client-test.cc@5359
PS2, Line 5359: cluster_->StartSync
ASSERT_OK ?


http://gerrit.cloudera.org:8080/#/c/9374/1/src/kudu/client/client.proto
File src/kudu/client/client.proto:

http://gerrit.cloudera.org:8080/#/c/9374/1/src/kudu/client/client.proto@116
PS1, Line 116: root 
nit: IIRC, some time ago there was an idea to put the whole certificate chain into this sequence.


http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client.proto
File src/kudu/client/client.proto:

http://gerrit.cloudera.org:8080/#/c/9374/2/src/kudu/client/client.proto@116
PS2, Line 116: root
nit: if I'm not mistaken, some time ago there was an idea to keep whole cert chain in there.



-- 
To view, visit http://gerrit.cloudera.org:8080/9374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45
Gerrit-Change-Number: 9374
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 26 Feb 2018 20:47:50 +0000
Gerrit-HasComments: Yes