Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

advancedtls: Add SNI logic to ServerOptions.GetCertificate #3697

Merged
merged 30 commits into from
Jul 28, 2020
Merged

advancedtls: Add SNI logic to ServerOptions.GetCertificate #3697

merged 30 commits into from
Jul 28, 2020

Conversation

cindyxue
Copy link
Contributor

@cindyxue cindyxue commented Jun 18, 2020 •
edited
Loading

This PR does the following thing:

For go1.10 and above, config.GetCertificate is now able to perform SNI logic based on ServerOptions.GetCertificate and return the certificate that matches the common name or DNS field in the given ClientHelloInfo.

For go1.9, config.GetCertificate will always return the first certificate in the certificate list given by ServerOptions.GetCertificate because SupportsCertificate is not supported.

@ZhenLian

@ZhenLian ZhenLian self-requested a review June 18, 2020 18:06
ZhenLian
ZhenLian suggested changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@ZhenLian ZhenLian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the draft! I left some comments at the first glance. Let me know if you have any questions.

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
@cindyxue
Copy link
Contributor Author

@ZhenLian Thanks for the review! I made some changes. Could you please check it again?

@cindyxue cindyxue requested a review from ZhenLian June 18, 2020 19:07
@menghanl
Copy link
Contributor

This PR is still a draft, is it right?

@cindyxue
Copy link
Contributor Author

@menghanl Yes :)

ZhenLian
ZhenLian suggested changes Jun 18, 2020
View reviewed changes
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
@cindyxue cindyxue requested a review from ZhenLian June 18, 2020 22:26
@cindyxue cindyxue changed the title advancedtls: Add SNI logic to config.GetCertificate advancedtls: Add SNI logic to ServerOptions.GetCertificate Jun 30, 2020
@cindyxue
Copy link
Contributor Author

cindyxue commented Jun 30, 2020 •
edited
Loading

@ZhenLian I added a unit test to test the newly added GetCertificate with SNI logic. However, one of the Travis-CI test fails and the error message shows security/advancedtls/advancedtls.go:526:24: clientHello.SupportsCertificate undefined (type *tls.ClientHelloInfo has no field or method SupportsCertificate). According to the golang tls package, this function func (chi *ClientHelloInfo) SupportsCertificate(c *Certificate) error does exist. Would you happen to know what was wrong here?

@cindyxue cindyxue marked this pull request as ready for review June 30, 2020 17:14
@ZhenLian
Copy link
Contributor

@cindyxue It looks like Go 1.9 doesn't support this function .... To bypass this check, you may want to use the build Constraints to have different implementations on different Go versions. See #3626 for example.

ZhenLian
ZhenLian approved these changes Jul 22, 2020
View reviewed changes
security/advancedtls/advancedtls_test.go Outdated Show resolved Hide resolved
@ZhenLian
Copy link
Contributor

@easwars Hi Easwar, Can you please take a look at this PR please? Thanks in advance!

@ZhenLian ZhenLian requested a review from easwars July 22, 2020 07:00
easwars
easwars requested changes Jul 24, 2020
View reviewed changes
security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved
security/advancedtls/sni_110.go Outdated Show resolved Hide resolved
security/advancedtls/testdata/server_cert_3.pem Show resolved Hide resolved
security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved
security/advancedtls/advancedtls_test.go Outdated Show resolved Hide resolved
security/advancedtls/advancedtls_test.go Outdated Show resolved Hide resolved
security/advancedtls/advancedtls_test.go Outdated Show resolved Hide resolved
security/advancedtls/advancedtls_test.go Outdated Show resolved Hide resolved
security/advancedtls/advancedtls_test.go Outdated Show resolved Hide resolved
security/advancedtls/advancedtls_integration_test.go Outdated Show resolved Hide resolved
@cindyxue cindyxue requested a review from easwars July 24, 2020 19:19
@easwars easwars added the Type: Feature New features or improvements in behavior label Jul 24, 2020
@easwars easwars added this to the 1.31 Release milestone Jul 24, 2020
@cindyxue cindyxue force-pushed the Cindy/SNI branch 2 times, most recently from daf1375 to 8fd0b50 Compare July 25, 2020 03:31
@cindyxue
Copy link
Contributor Author

cindyxue commented Jul 25, 2020 •
edited
Loading

@easwars Thank you so much for the code review, Easwar! I have made changes addressing the review comments. Could you please take another look and see if there are more places to change? Thanks again!

easwars
easwars approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Some minor nits to take care of before merging.

security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved
security/advancedtls/sni_110.go Outdated Show resolved Hide resolved
security/advancedtls/sni_before_110.go Outdated Show resolved Hide resolved
security/advancedtls/sni_test_110.go Outdated Show resolved Hide resolved
security/advancedtls/sni_test_before_110.go Outdated Show resolved Hide resolved
cindyxue
cindyxue commented Jul 28, 2020
View reviewed changes
Copy link
Contributor Author

@cindyxue cindyxue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@easwars Thank you so much for the code review! Fix all of the minor issues mentioned above.

@ZhenLian Could you please take one last look at the PR and see if it's ready to be merged? Thank you so much!

security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved
security/advancedtls/sni_110.go Outdated Show resolved Hide resolved
security/advancedtls/sni_before_110.go Outdated Show resolved Hide resolved
security/advancedtls/sni_test_110.go Outdated Show resolved Hide resolved
security/advancedtls/sni_test_before_110.go Outdated Show resolved Hide resolved
@ZhenLian
Copy link
Contributor

Thanks @cindyxue for the implementation and Thanks @easwars for the review! Merging now ...

@ZhenLian ZhenLian merged commit dfc0c05 into grpc:master Jul 28, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ZhenLian ZhenLian ZhenLian approved these changes

@easwars easwars easwars approved these changes

Assignees

@ZhenLian ZhenLian

Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Milestone
1.31 Release
Development

Successfully merging this pull request may close these issues.
4 participants
@cindyxue @menghanl @ZhenLian @easwars