You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/02/22 21:55:49 UTC

[kudu-CR] IPKI: change X509 attribute fields 

Hello Dan Burkert, Alexey Serbin,

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

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

to review the following change.

Change subject: IPKI: change X509 attribute fields
......................................................................

IPKI: change X509 attribute fields

This switches the attributes of the IPKI certs as follows:

CN = FQDN of host
userId = short user name
<custom OID> = long kerberos principal (if kerberized)

Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert-test.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/security-test-util.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/server/server_base.cc
15 files changed, 310 insertions(+), 60 deletions(-)


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

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

[kudu-CR] IPKI: change X509 attribute fields

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: IPKI: change X509 attribute fields
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6116/3/src/kudu/security/init.cc
File src/kudu/security/init.cc:

Line 66: krb5_context g_krb5_ctx;
> yea, I see your point... we could consider some refactoring in the coming w
Yah.  I'm not entirely convinced we need globals here.  We could return the local krb principal from the initialization, and then pipe that through to the self-signed cert (it's all happening at server startup).  Again, not critical right now.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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-HasComments: Yes

[kudu-CR] IPKI: change X509 attribute fields

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.

Change subject: IPKI: change X509 attribute fields
......................................................................


IPKI: change X509 attribute fields

This switches the attributes of the IPKI certs as follows:

CN = FQDN of host
userId = short user name
<custom OID> = long kerberos principal (if kerberized)

Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
Reviewed-on: http://gerrit.cloudera.org:8080/6116
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert-test.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/security-test-util.cc
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/server/server_base.cc
18 files changed, 315 insertions(+), 62 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Todd Lipcon: Verified



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

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

[kudu-CR] IPKI: change X509 attribute fields

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: IPKI: change X509 attribute fields
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6116/1/src/kudu/security/cert-test.cc
File src/kudu/security/cert-test.cc:

Line 112: 
> Is it worth adding a reference cert with the userId and KuduKerberosPrincip
that's tested in cert_management-test.


http://gerrit.cloudera.org:8080/#/c/6116/1/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

PS1, Line 56:   int nid = OBJ_txt2nid(kKuduKerberosPrincipalOidStr);
            :   if (nid != NID_undef) return nid;
> do we expect any races here?
perhaps, but I checked and calling it twice just results in two 'nids' for the same object, and it still seems to work. If you feel strongly I'll add a std::call_once here


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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-HasComments: Yes

[kudu-CR] IPKI: change X509 attribute fields

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: IPKI: change X509 attribute fields
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6116/3/src/kudu/security/init.cc
File src/kudu/security/init.cc:

Line 66: krb5_context g_krb5_ctx;
> I'm a little bit nervous about having these globals in a non-server specifi
yea, I see your point... we could consider some refactoring in the coming weeks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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-HasComments: Yes

[kudu-CR] IPKI: change X509 attribute fields

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: IPKI: change X509 attribute fields
......................................................................


Patch Set 4:

Same flake. Will prioritize fixing that ASAP but not due to this patch.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] IPKI: change X509 attribute fields

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

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

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

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

Change subject: IPKI: change X509 attribute fields
......................................................................

IPKI: change X509 attribute fields

This switches the attributes of the IPKI certs as follows:

CN = FQDN of host
userId = short user name
<custom OID> = long kerberos principal (if kerberized)

Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert-test.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/security-test-util.cc
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/server/server_base.cc
18 files changed, 315 insertions(+), 62 deletions(-)


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

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

[kudu-CR] IPKI: change X509 attribute fields

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: IPKI: change X509 attribute fields
......................................................................


Patch Set 4: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] IPKI: change X509 attribute fields

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

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

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

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

Change subject: IPKI: change X509 attribute fields
......................................................................

IPKI: change X509 attribute fields

This switches the attributes of the IPKI certs as follows:

CN = FQDN of host
userId = short user name
<custom OID> = long kerberos principal (if kerberized)

Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert-test.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/security-test-util.cc
M src/kudu/security/test/mini_kdc-test.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/server/server_base.cc
18 files changed, 315 insertions(+), 62 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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] IPKI: change X509 attribute fields

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: IPKI: change X509 attribute fields
......................................................................


Patch Set 3: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6116/1/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:

Line 121:   MessengerBuilder& enable_inbound_tls();
Nice


http://gerrit.cloudera.org:8080/#/c/6116/3/src/kudu/security/init.cc
File src/kudu/security/init.cc:

Line 66: krb5_context g_krb5_ctx;
I'm a little bit nervous about having these globals in a non-server specific module, but since it's already here I won't block this patch over it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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-HasComments: Yes

[kudu-CR] IPKI: change X509 attribute fields

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: IPKI: change X509 attribute fields
......................................................................


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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-HasComments: No

[kudu-CR] IPKI: change X509 attribute fields

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: IPKI: change X509 attribute fields
......................................................................


Patch Set 3: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6116/1/src/kudu/security/cert-test.cc
File src/kudu/security/cert-test.cc:

Line 112: 
> that's tested in cert_management-test.
yes, indeed.


http://gerrit.cloudera.org:8080/#/c/6116/1/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

PS1, Line 56:   int nid = OBJ_txt2nid(kKuduKerberosPrincipalOidStr);
            :   if (nid != NID_undef) return nid;
> perhaps, but I checked and calling it twice just results in two 'nids' for 
Then it seems to be fine, I think.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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-HasComments: Yes

[kudu-CR] IPKI: change X509 attribute fields

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: IPKI: change X509 attribute fields
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6116/1/src/kudu/security/cert-test.cc
File src/kudu/security/cert-test.cc:

Line 112: 
Is it worth adding a reference cert with the userId and KuduKerberosPrincipalName?


http://gerrit.cloudera.org:8080/#/c/6116/1/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

PS1, Line 56:   int nid = OBJ_txt2nid(kKuduKerberosPrincipalOidStr);
            :   if (nid != NID_undef) return nid;
do we expect any races here?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b759c327ea7758a98e675b12ff5ef49294679
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes