advancedtls: add new module for advanced TLS handshaker #3187

ZhenLian · 2019-11-16T00:14:25Z

This PR contains the implementation and tests of a utility library for some advanced TLS features, including:

Credential Reloading: users are able to reload their credentials without restarting the app. Note that the "credential" here can be a private key, its corresponding certificate bundle, or trusted CA certificate bundle.
Server Authorization: on client side, we let users specify their own functions to check peers' identities when handshakes are finished. This feature is sometimes also referred to as "secure name checking" or "hostname verification".

There are some previous discussions in this deprecated PR: #3044. The goal of this PR remains the same, but change the APIs to provide better functionalities.
This PR includes the implementation and unit tests and integration tests for the client and server without credential reloading. Next PR will cover the integration tests for credential reloading.

ZhenLian · 2019-11-16T20:57:47Z

@jiangtaoli2016 Could you please take a first round of review on the API design? Thank you so much for the help!

jiangtaoli2016

Thanks Zhen for implementation! Looks very good. A few questions.
I have not reviewed the test file yet.

security/advancedtls/advancedtls.go

jiangtaoli2016

Some minor comments on test.

security/advancedtls/advancedtls.go

security/advancedtls/advancedtls_test.go

jiangtaoli2016 · 2019-11-22T17:11:17Z

security/advancedtls/advancedtls_test.go

+		serverRoot                 *x509.CertPool
+		serverGetRoot              func(rawConn net.Conn, rawCerts [][]byte) (*x509.CertPool, error)
+		serverExpectError          bool
+	}{


We may want to test the cases where the root CA certs do not match the peer certificates. E.g., server provides a valid certificate, but server's CA cert is different from client Root cert.

Added the following test cases:

Client_reload_both_certs_verifyFunc_Server_reload_both_certs_mutualTLS

Client_wrong_peer_cert_Server_reload_both_certs_mutualTLS

Client_wrong_trust_cert_Server_reload_both_certs_mutualTLS

Client_reload_both_certs_verifyFunc_Server_wrong_peer_cert

Client_reload_both_certs_verifyFunc_Server_wrong_trust_cert

jiangtaoli2016 · 2019-11-22T17:26:40Z

In current implementation, client has to provide either RootCA or GetRootCA.

We see use case grpc/grpc#20530 that client does not know root CA certs in advance. In that case, we give the power to client to validate via VerifyPeer function (since application can access target name and server certain chain).

Thus, if client does not provide RootCA or GetRootCA, instead of failing, we just don't validate the server cert and let application decide via VerifyPeer. @ZhenLian Could update your implementation as well?

jiangtaoli2016 · 2019-11-23T03:05:24Z

Thanks @ZhenLian for the implementation.
My knowledge in Golang is limited.
@cesarghali Please check Go readability and security suggestions.
@dfawley Please review on API and make sure you are comfortable.

ZhenLian · 2019-11-23T21:31:32Z

The API is now updated as:
When the client side specifies neither RootCA nor GetRootCA but with VerifyPeer, we will only use VerifyPeer to see if this RPC call is valid.
Note that if client specifies none of the three, we will still fall back to default checks, which are certificate check and host name check.

cesarghali

Thanks for the PR. I left some comments in advancedtls.go. I will review advancedtls_test.go next.

security/advancedtls/advancedtls.go

ZhenLian · 2019-12-05T06:17:01Z

@cesarghali Thank you so much for the review! I left a few more comments. Please let me know if anything could be further improved :-)

ZhenLian · 2019-12-10T01:24:40Z

@cesarghali
Hi Cesar, I've updated the comments inside config() functions and refactored the Options struct as we discussed. Could you please take a look at it(as well as the tests) to see if there are any other things that could be improved? Thank you so much!

ZhenLian · 2019-12-11T17:12:06Z

Friendly ping :-)

security/advancedtls/advancedtls.go

ZhenLian · 2019-12-21T23:15:29Z

@dfawley
Hi Doug, sorry for the delay of this PR. Now our team has completed the review process. Could you please take a look at the overall structure, the go module files, etc? Thanks in advance for your time!

dfawley

Looks good overall, just some minor things.

security/advancedtls/advancedtls.go

ZhenLian · 2020-01-07T21:51:42Z

@dfawley
Hi Doug, thank you so much for the suggestions and comments. I have addressed them all. Could you please take a second look?
By the way, I made another PR, #3300, to fix a small issue found here but might also apply to that place. It would be good if you could take a look as well.
Thanks again for the help :-)

ZhenLian force-pushed the zhen_spiffe_1 branch from fc76a15 to 7c9f9a4 Compare November 16, 2019 00:17

dfawley added Type: Feature New features or improvements in behavior no release notes labels Nov 16, 2019

ZhenLian force-pushed the zhen_spiffe_1 branch from 7c9f9a4 to 906d1f3 Compare November 16, 2019 02:52

jiangtaoli2016 reviewed Nov 19, 2019

View reviewed changes

security/advancedtls/advancedtls.go Show resolved Hide resolved

security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved

security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved

security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved

dfawley assigned ZhenLian and jiangtaoli2016 Nov 21, 2019

ZhenLian requested a review from jiangtaoli2016 November 22, 2019 01:19

jiangtaoli2016 reviewed Nov 22, 2019

View reviewed changes

ZhenLian force-pushed the zhen_spiffe_1 branch 2 times, most recently from 128795a to 781f1c0 Compare November 22, 2019 22:25

jiangtaoli2016 approved these changes Nov 23, 2019

View reviewed changes

cesarghali reviewed Dec 5, 2019

View reviewed changes

ZhenLian force-pushed the zhen_spiffe_1 branch 5 times, most recently from 74939ed to 43d11d8 Compare December 5, 2019 06:01

ZhenLian requested a review from cesarghali December 5, 2019 06:17

menghanl assigned dfawley Dec 5, 2019

menghanl requested a review from dfawley December 5, 2019 22:53

cesarghali reviewed Dec 18, 2019

View reviewed changes

security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved

ZhenLian force-pushed the zhen_spiffe_1 branch from d4ad63c to 2e3e312 Compare December 20, 2019 22:14

ZhenLian force-pushed the zhen_spiffe_1 branch from 2e3e312 to 850dad9 Compare December 20, 2019 23:00

cesarghali approved these changes Dec 20, 2019

View reviewed changes

ZhenLian force-pushed the zhen_spiffe_1 branch 3 times, most recently from 35fa648 to 67ad997 Compare December 21, 2019 22:55

dfawley reviewed Jan 7, 2020

View reviewed changes

dfawley removed their assignment Jan 7, 2020

ZhenLian force-pushed the zhen_spiffe_1 branch from f3c727d to aa767af Compare January 7, 2020 21:14

ZhenLian requested a review from dfawley January 7, 2020 21:51

dfawley approved these changes Jan 7, 2020

View reviewed changes

[1/2] SPIFFE: Util Lib for Advanced TLS Features

8d72f23

ZhenLian force-pushed the zhen_spiffe_1 branch from aa767af to 8d72f23 Compare January 7, 2020 22:52

dfawley removed the no release notes label Jan 7, 2020

dfawley added this to the 1.27 Release milestone Jan 7, 2020

dfawley changed the title ~~[1/2] SPIFFE: Util Lib for Advanced TLS Features~~ advancedtls: add new module for advanced TLS handshaker Jan 7, 2020

dfawley merged commit 4a4d179 into grpc:master Jan 7, 2020

github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

advancedtls: add new module for advanced TLS handshaker #3187

advancedtls: add new module for advanced TLS handshaker #3187

ZhenLian commented Nov 16, 2019

ZhenLian commented Nov 16, 2019

jiangtaoli2016 left a comment

jiangtaoli2016 left a comment

jiangtaoli2016 Nov 22, 2019

ZhenLian Nov 22, 2019 •

edited

Loading

jiangtaoli2016 commented Nov 22, 2019

jiangtaoli2016 commented Nov 23, 2019

ZhenLian commented Nov 23, 2019 •

edited

Loading

cesarghali left a comment

ZhenLian commented Dec 5, 2019

ZhenLian commented Dec 10, 2019

ZhenLian commented Dec 11, 2019

ZhenLian commented Dec 21, 2019

dfawley left a comment

ZhenLian commented Jan 7, 2020

advancedtls: add new module for advanced TLS handshaker #3187

advancedtls: add new module for advanced TLS handshaker #3187

Conversation

ZhenLian commented Nov 16, 2019

ZhenLian commented Nov 16, 2019

jiangtaoli2016 left a comment

Choose a reason for hiding this comment

jiangtaoli2016 left a comment

Choose a reason for hiding this comment

jiangtaoli2016 Nov 22, 2019

Choose a reason for hiding this comment

ZhenLian Nov 22, 2019 • edited Loading

Choose a reason for hiding this comment

jiangtaoli2016 commented Nov 22, 2019

jiangtaoli2016 commented Nov 23, 2019

ZhenLian commented Nov 23, 2019 • edited Loading

cesarghali left a comment

Choose a reason for hiding this comment

ZhenLian commented Dec 5, 2019

ZhenLian commented Dec 10, 2019

ZhenLian commented Dec 11, 2019

ZhenLian commented Dec 21, 2019

dfawley left a comment

Choose a reason for hiding this comment

ZhenLian commented Jan 7, 2020

ZhenLian Nov 22, 2019 •

edited

Loading

ZhenLian commented Nov 23, 2019 •

edited

Loading