You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2017/11/19 23:37:22 UTC
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8595
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
KUDU-2220: GetEndOfChainX509 does not return end-user cert
KUDU-2091 introduced a function GetEndOfChainX509() which was supposed
to return the "end-user" certificate. However, the end-user certificate
is not at the end of the chain, but rather at the beginning of the chain
as specificed by the RFC:
https://tools.ietf.org/html/rfc5246#section-7.4.2
| This is a sequence (chain) of certificates. The sender's certificate MUST
| come first in the list. Each following certificate MUST directly certify
| the one preceding it.
This patch fixes this by changing the GetEndOfChainX509() to
GetTopOfChainX509(). An existing test is modified to test this patch. It does
not pass without this change.
Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake.cc
7 files changed, 65 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/8595/1
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 )
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc
File src/kudu/security/test/test_certs.cc:
http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc@506
PS2, Line 506: CreateTestSSLCertSignedByChain
Which cert do we want do use, actually? Could we drop the old one?
Also, how do we know that GetTopOfChainX509() returns the expected cert, not that GetEndOfChainX509() would return? Does it make sense to add some verification on the parameters of the cert returned by GetTopOfChainX509() ?
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Nov 2017 18:07:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8595
to look at the new patch set (#2).
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
KUDU-2220: GetEndOfChainX509 does not return end-user cert
KUDU-2091 introduced a function GetEndOfChainX509() which was supposed
to return the "end-user" certificate. However, the end-user certificate
is not at the end of the chain, but rather at the beginning of the chain
as specificed by the RFC:
https://tools.ietf.org/html/rfc5246#section-7.4.2
| This is a sequence (chain) of certificates. The sender's certificate MUST
| come first in the list. Each following certificate MUST directly certify
| the one preceding it.
This patch fixes this by changing the GetEndOfChainX509() to
GetTopOfChainX509(). An existing test is modified to test this patch. It does
not pass without this change.
Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake.cc
7 files changed, 65 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/8595/2
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 )
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc
File src/kudu/security/test/test_certs.cc:
http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc@506
PS2, Line 506: CreateTestSSLCertSignedByChain
> Which cert do we want do use, actually? Could we drop the old one?
I basically added the intermediate cert (from 'kCaChainCert' below) to the 'kCert' variable.
The reason is that if there is only one cert in the server/client certificate, then GetTopofChainX509() and GetEndOfChainX509() would both return the correct cert, since there's only one.
But for all PEM files, if they're a chain of certs and not a CA, the sender certificate must come first, followed by a chain of certs that each trust the previous one in the chain.
So, if we leave the function as GetEndOfChainX509() and add this intermediate cert to 'kCert', the test using these certs would fail.
I think validation already happens now here at CheckPrivateKey():
https://github.com/apache/kudu/blob/master/src/kudu/security/cert.cc#L154-L159
If the cert we get back from GetTopOfChainX509() is incorrect, then this function would return an error.
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Nov 2017 18:21:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 )
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc
File src/kudu/security/test/test_certs.cc:
http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc@506
PS2, Line 506: CreateTestSSLCertSignedByChain
> I basically added the intermediate cert (from 'kCaChainCert' below) to the
Yes, I understand that these function does not differ if the only one certificate is provided. I asked because it seemed to me that previously we used self-signed cert here. If an intermediary CA is already present, then it would be all right then.
However, I don't understand the following: how could it work with other than self-signed cert in the previous version if the intermediate cert was not presented by the server?
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Nov 2017 18:49:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 )
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 18:24:25 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 )
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc
File src/kudu/security/test/test_certs.cc:
http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc@506
PS2, Line 506: CreateTestSSLCertSignedByChain
> Yes, I understand that these function does not differ if the only one certi
Ah, yes, I see the server was given both certs. Sorry for misunderstanding.
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Nov 2017 19:05:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8595 )
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
KUDU-2220: GetEndOfChainX509 does not return end-user cert
KUDU-2091 introduced a function GetEndOfChainX509() which was supposed
to return the "end-user" certificate. However, the end-user certificate
is not at the end of the chain, but rather at the beginning of the chain
as specificed by the RFC:
https://tools.ietf.org/html/rfc5246#section-7.4.2
| This is a sequence (chain) of certificates. The sender's certificate MUST
| come first in the list. Each following certificate MUST directly certify
| the one preceding it.
This patch fixes this by changing the GetEndOfChainX509() to
GetTopOfChainX509(). An existing test is modified to test this patch. It does
not pass without this change.
Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Reviewed-on: http://gerrit.cloudera.org:8080/8595
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake.cc
7 files changed, 65 insertions(+), 22 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
Todd Lipcon: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 )
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
Patch Set 2:
> > Would it make more sense to just call it GetServerCertFromChain(),
> > instead of GetTopofChainX509() ?
>
> As I understand, those wrapper functions could be used to work with
> client-side certs, right? If so, then I don't think
> GetServerCertFromChain() is the best choice here.
Yes, you're right. The 'Cert' class is generic and not specific to server certs. I'll leave the name as it is then. Thanks.
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Nov 2017 18:05:25 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 )
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc
File src/kudu/security/test/test_certs.cc:
http://gerrit.cloudera.org:8080/#/c/8595/2/src/kudu/security/test/test_certs.cc@506
PS2, Line 506: CreateTestSSLCertSignedByChain
> Ah, yes, I see the server was given both certs. Sorry for misunderstanding
Yes, both were given in the test. The intermediate and root CAs were passed through the out param 'ca_cert_file'.
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Nov 2017 19:49:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 )
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
Patch Set 2: Code-Review+2
Looks good to me. Perhaps, you want to get some input from Todd as well?
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Nov 2017 07:37:11 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 )
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
Patch Set 2:
Would it make more sense to just call it GetServerCertFromChain(), instead of GetTopofChainX509() ?
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Nov 2017 17:51:09 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8595 )
Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
......................................................................
Patch Set 2:
> Would it make more sense to just call it GetServerCertFromChain(),
> instead of GetTopofChainX509() ?
As I understand, those wrapper functions could be used to work with client-side certs, right? If so, then I don't think GetServerCertFromChain() is the best choice here.
--
To view, visit http://gerrit.cloudera.org:8080/8595
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8595
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Nov 2017 17:55:46 +0000
Gerrit-HasComments: No