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

["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