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 2016/12/13 00:17:01 UTC
[kudu-CR] WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
Hello Todd Lipcon, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5484
to review the following change.
Change subject: WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
......................................................................
WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
This commit changes the RPC system to automatically upgrade a plaintext
connection to use TLS following the SASL negotiation step, if both sides
support TLS. Support for TLS is determined using a new 'TLS' RPC feature
flag. This flag is set by both the server and client if each has been
configured with TLS (currently that means having the
rpc_ssl_server_certificate, rpc_ssl_private_key, and
rpc_ssl_certificate_authority flags set, but will likely change in the
future, especially on the client-side).
Using a feature flag for determining TLS support is (in my opinion) a
good fit, but unfortunately the implementation is a little bit messy
because these flags are communicated during SASL negotiation, so it is
necessary to add some TLS-specific code to the SASL negotiation handling
classes.
Further work remaining to be done before TLS encrypted connections can
be considered secure:
* The server needs a flag which forces connections to use TLS. Without
such a configuration Kudu is vulnerable to downgrade attacks.
* Channel binding between the TLS channel and the SASL channel needs to
be established. Currently we aren't using any kind of auth-conf or
auth-int SASL channel, so that will necessarily need to come first.
WIP: I think the code is fine here, but I want to solicit feedback on
the general direction of establishing TLS following SASL negotiation,
and the RPC feature flag mechanism. Also, I'm considering adding a
commit to go before this one the renames SSL classes to use TLS instead,
since we are not supporting SSL.
Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_helper.h
M src/kudu/rpc/sasl_server.cc
M src/kudu/rpc/sasl_server.h
10 files changed, 66 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/5484/1
--
To view, visit http://gerrit.cloudera.org:8080/5484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
......................................................................
Patch Set 1:
(2 comments)
Just a couple of nits, but so far it looks great. Will take a closer look tonight.
http://gerrit.cloudera.org:8080/#/c/5484/1/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
PS1, Line 640: socket_.reset
nit: consider using swap() instead
http://gerrit.cloudera.org:8080/#/c/5484/1/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:
PS1, Line 153: bool
nit: it seems we do favor enums instead for such cases
--
To view, visit http://gerrit.cloudera.org:8080/5484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
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-HasComments: Yes
[kudu-CR] WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has abandoned this change.
Change subject: WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
......................................................................
Abandoned
Abandoned in favor of 'TLS-negotiation' patch series.
--
To view, visit http://gerrit.cloudera.org:8080/5484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
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: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
......................................................................
Patch Set 1:
I'm in the process of writing up the design in the auth design guide: https://docs.google.com/document/d/1Yu4iuIhaERwug1vS95yWDd_WzrNRIKvvVGUb31y-_mY/edit#.
The fallout is that, with some refactoring, I think we can do essentially what this patch is doing, but initiate TLS right after the NEGOTIATE step and before SASL INITIATE. This gives us the best of both worlds, in a sense. No need to tunnel the TLS handshake through PB messages, but the SASL negotiation is wrapped in TLS I'm going to start prototyping this now. Comments welcome on the doc. I'll most likely be closing this gerrit or completely rewriting it.
--
To view, visit http://gerrit.cloudera.org:8080/5484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
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: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
......................................................................
Patch Set 1:
I agree with everything you wrote, I'll write this up for the doc tomorrow.
--
To view, visit http://gerrit.cloudera.org:8080/5484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
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: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
......................................................................
Patch Set 1:
> I was under the impression that previously, it was decided that the SASL negotiation would need to be encrypted as well. Would that be the case when we have the server flag that forces TLS?
Yes, the original design called for initiating TLS *then* SASL inside TLS. After having though more about this, I've come to a couple conclusions:
1) It's much easier to do SASL then TLS if we want to retain backwards compatibility with old servers/clients - this commit is the proof. Negotiating TLS before SASL will require much bigger changes to the RPC layer.
2) I don't think it makes a difference, security wise, whether TLS or SASL comes first. We should check with an expert on this. My reasoning is pretty simple - if wrapping TLS around the SASL negotiation makes it more secure, then the security guarantees of SASL are broken. A passive listener learning about an individual connections' SASL credentials are the least of our worries if this is the case.
I'd like to get everyones thoughts on this - I've thought a lot about it, but I may have missed some salient facts.
> the downgrade attack seems very easy to do. Is there any way to deal with this when someone sets the future server side "force TLS" flag to off
It would be easy for a MITM, yes. The "force TLS" flag is the necessary and sufficient protection against this - with this flag turned on non-TLS connections will be rejected by the server. There's no way to make the upgrade protocol smarter to protect against this attack - necessarily this protocol is done outside of TLS, and is therefore MITM-able.
--
To view, visit http://gerrit.cloudera.org:8080/5484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
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: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
......................................................................
Patch Set 1:
> > I was under the impression that previously, it was decided that
> the SASL negotiation would need to be encrypted as well. Would that
> be the case when we have the server flag that forces TLS?
>
> Yes, the original design called for initiating TLS *then* SASL
> inside TLS. After having though more about this, I've come to a
> couple conclusions:
>
> 1) It's much easier to do SASL then TLS if we want to retain
> backwards compatibility with old servers/clients - this commit is
> the proof. Negotiating TLS before SASL will require much bigger
> changes to the RPC layer.
> 2) I don't think it makes a difference, security wise, whether TLS
> or SASL comes first. We should check with an expert on this. My
> reasoning is pretty simple - if wrapping TLS around the SASL
> negotiation makes it more secure, then the security guarantees of
> SASL are broken. A passive listener learning about an individual
> connections' SASL credentials are the least of our worries if this
> is the case.
>
That's true. My point was not that the security guarantees of SASL are broken, but rather a denial-of-service is always possible. And I don't mean it in a way where communication between peers is blocked, but rather they deny the choice of a peer to successfully carry out the upgrade step (or TLS hello), so basically just confirming what you mention below.
> I'd like to get everyones thoughts on this - I've thought a lot
> about it, but I may have missed some salient facts.
>
> > the downgrade attack seems very easy to do. Is there any way to
> deal with this when someone sets the future server side "force TLS"
> flag to off
>
> It would be easy for a MITM, yes. The "force TLS" flag is the
> necessary and sufficient protection against this - with this flag
> turned on non-TLS connections will be rejected by the server.
> There's no way to make the upgrade protocol smarter to protect
> against this attack - necessarily this protocol is done outside of
> TLS, and is therefore MITM-able.
I'm not too clear on the big picture (with the CA master service, etc.), but is the following a possibility?
Before the SASL negotiation, have the connection be initiated with TLS by a peer 'assuming' that the other peer also speaks TLS, and if that fails, then we know that only plain is possible, and based on the 'force TLS' flag, allow or deny further communication. Something similar to a "pre-flight" check.
Not sure if in the big picture, the certificates are already in place before the SASL step to allow for this.
I can comment more on that doc later.
--
To view, visit http://gerrit.cloudera.org:8080/5484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
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: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
......................................................................
Patch Set 1:
Specifically the 'TLS Negotiation' section is new
--
To view, visit http://gerrit.cloudera.org:8080/5484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
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: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
......................................................................
Patch Set 1:
(3 comments)
This looks good to me. I just have a few comments.
I was under the impression that previously, it was decided that the SASL negotiation would need to be encrypted as well. Would that be the case when we have the server flag that forces TLS?
Also, the downgrade attack seems very easy to do. Is there any way to deal with this when someone sets the future server side "force TLS" flag to off?
http://gerrit.cloudera.org:8080/#/c/5484/1/src/kudu/rpc/negotiation.cc
File src/kudu/rpc/negotiation.cc:
PS1, Line 189: if (ContainsKey(conn->sasl_client().server_features(), TLS)) {
Don't we also need to check if the client itself supports those features?
If the remote server supports it, but the client doesn't, it will DCHECK in conn->InitTls().
PS1, Line 210: if (ContainsKey(conn->sasl_server().client_features(), TLS)) {
Same here for the server.
http://gerrit.cloudera.org:8080/#/c/5484/1/src/kudu/rpc/sasl_helper.h
File src/kudu/rpc/sasl_helper.h:
PS1, Line 102: // Check if TLS is supported.
: Status IsTlsEnabled();
This doesn't seem to be implemented.
--
To view, visit http://gerrit.cloudera.org:8080/5484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: WIP: rpc: Initiate TLS connection upgrade following SASL negotiation
......................................................................
Patch Set 1:
Regarding TLS-then-SASL vs SASL-then-TLS, I think there is one potentially important difference here:
Previously we'd decided to use asymmetric authentication tokens -- i.e the client passes a master-signed token to tablet servers to authenticate itself. This token must be passed directly over the wire to the server as part of the connection authentication (unlike a symmetric scheme where the two peers could authenticate by passing only hashes of the token). Given that, the token-passing must happen _after_ an encrypted/server-authenticated channel is established. We were planning on using TLS+certs to ensure that the client doesn't pass a token to an impostor server.
So, if we did SASL-then-TLS, the negotiation with tokens would have to look somewhat like:
--> SASL NEGOTIATE
<-- [allowed mechanisms]
--> SASL "anonymous"
<-- server "OK"
--> TLS Upgrade (with the TLS hello)
<-> [TLS handshake], verify certificate
--> authentication token
--> first call (could be pipelined with the token?)
<-- authentication response
and the negotiation with KRB5 would have to look like:
--> SASL NEGOTIATE
<-- [allowed mechs]
--> SASL GSSAPI
<--> [SASL negotiation]
<-- SASL COMPLETE
--> TLS Upgrade (with TLS hello)
<-> [TLS handshake]
<-- TLS server endpoint fingerprint, integrity-protected by SASL (channel binding)
--> first call (has to wait for the channel binding)
right?
Perhaps we should move this to the google doc and try to flesh out the exact negotiation sequence. I'm thinking it might be possible to use fewer round trips but still be compatible, but a little hard to discuss via gerrit comments.
--
To view, visit http://gerrit.cloudera.org:8080/5484
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
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: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No