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/04/05 06:46:16 UTC
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
Sailesh Mukil has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6555
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
KUDU-1965: Allow user provided TLS certificates to work with KRPC
This patch adds a bool in the Cert class to keep track of whether
the certificate was user provided or not.
If it was user provided, the Kudu specific X509 extensions are not
checked for in the negotiation phase (since they most likely will
not be present).
Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
---
M src/kudu/rpc/server_negotiation.cc
M src/kudu/security/cert.h
M src/kudu/security/tls_context.cc
3 files changed, 17 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6555/1
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#2).
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
KUDU-1965: Allow user provided TLS certificates to work with KRPC
This patch adds a bool in the Cert class to keep track of whether
the certificate was user provided or not.
If it was user provided, the Kudu specific X509 extensions are not
checked for in the negotiation phase (since they most likely will
not be present).
Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
---
M src/kudu/rpc/server_negotiation.cc
M src/kudu/security/cert.h
M src/kudu/security/tls_context.cc
3 files changed, 19 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6555/2
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6555/3/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 629: if (!cert.is_user_provided()) {
> Ideally Kudu could support either IPKI certs or external certs in the same
In that case, I guess we'd need to extend the 'authenticated_user_' part to include the type of cert?
What shortname (unix user equivalent) would we assign to a host cert?
Also we should be a little bit careful here, because IIRC the way Impala works today is that it is using GSSAPI SASL plus certs, but the authentication info is coming from GSSAPI. With KRPC, if we have certs on both sides, we'll try to use the cert for authentication, and not SASL. This is "good" but could end up with a wider set of authenticatable users than intended.
Essentially we dont want to silently switch authentication mechanisms on users.
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6555/3/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 629: if (!cert.is_user_provided()) {
> I guess it depends how we configure the handshake from the server side. Cur
Ah I see. Thanks for pointing that out, I wasn't aware of that.
I was working on making messenger options configurable:
https://gerrit.cloudera.org/#/c/6520/
But I can see how this patch, the above patch and doing hostname verification are all related.
It looks like the hostname verification in TlsHandshake can be enabled if host-based PKI certs are provided as a first step.
Enabling it for IPKI certs seems like a larger project based on the description in KUDU-1886.
I tried out both IPKI and host-based PKI certs after uncommenting out the hostname verification and sure enough, it worked for host-based PKI and failed for IPKI, as expected.
Line 629: if (!cert.is_user_provided()) {
> In that case, I guess we'd need to extend the 'authenticated_user_' part to
I'm not very familiar with that part of the code, but I assume you're talking about using one of these SetAuthenticatedBy*() methods?
https://github.com/apache/kudu/blob/master/src/kudu/rpc/remote_user.h#L68
Is a shortname required even for the internal servers to talk to each other? I'm guessing yes, since we would only want the "kudu" (or "impala") users from a remote internal server to be able to talk to another internal node.
In which case, do we need to do both AuthenticateBySasl() and AuthenticateByCertificate() for host-based PKI?
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has abandoned this change.
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
Abandoned
Patch is incorrect. Will post another one shortly.
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
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/6555
to look at the new patch set (#3).
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
KUDU-1965: Allow user provided TLS certificates to work with KRPC
This patch adds a bool in the Cert class to keep track of whether
the certificate was user provided or not.
If it was user provided, the Kudu specific X509 extensions are not
checked for in the negotiation phase (since they most likely will
not be present).
Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
---
M src/kudu/rpc/server_negotiation.cc
M src/kudu/security/cert.h
M src/kudu/security/tls_context.cc
3 files changed, 19 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6555/3
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6555/1/src/kudu/security/cert.h
File src/kudu/security/cert.h:
PS1, Line 71: b
nit: const ?
PS1, Line 76: e:
It seems this is not initialized -- consider adding an item into the list of initializers in the constructor.
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6555/1/src/kudu/security/cert.h
File src/kudu/security/cert.h:
PS1, Line 71: b
> nit: const ?
Done
PS1, Line 76: e:
> It seems this is not initialized -- consider adding an item into the list o
Done
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
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-HasComments: Yes
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6555/3/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 629: if (!cert.is_user_provided()) {
> I don't see how this could work -- GetRemoteCert is coming off the TLS hand
You're right. I did something silly while testing which gave me false positive results. Looks like having this flag in TlsContext would be best.
However, why do the hostname verification check for host based PKI? Wouldn't that already happen in the SSL handshake layer?
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6555/3/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 629: if (!cert.is_user_provided()) {
> You're right. I did something silly while testing which gave me false posit
I guess it depends how we configure the handshake from the server side. Currently Kudu isn't doing hostname verification at all for our certs. I suppose Impala would need to configure it to do so. As of right now, the code's commented out though (I thought you mentioned you were working on making this configurable)
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6555/3/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 629: if (!cert.is_user_provided()) {
I don't see how this could work -- GetRemoteCert is coming off the TLS handshake, and the TLS library doesn't know the source of the cert on the other side. i.e this will always be false, so I don't think the change has any affect.
I think this has to be some explicit configuration on the messenger or acceptor which says whether to accept IPKI certs (in which case it should do this check) or to accept host-based PKI certs (in which case it probably should be doing some kind of hostname verification check)
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1965: Allow user provided TLS certificates to work with KRPC
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1965: Allow user provided TLS certificates to work with KRPC
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6555/3/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
Line 629: if (!cert.is_user_provided()) {
> I guess it depends how we configure the handshake from the server side. Cur
Ideally Kudu could support either IPKI certs or external certs in the same messenger, in the case where IPKI is used for tserver certs, and external certs are being used for clients. As such, it would be great to not have an either/or switch.
--
To view, visit http://gerrit.cloudera.org:8080/6555
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica6e2bacb378553723467f0dc54a166885db1e4d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes