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.

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

advancedtls: add fine-grained verification levels in XXXOptions #3454

Merged
merged 7 commits into from
Apr 22, 2020

Conversation

ZhenLian
Copy link
Contributor

@ZhenLian ZhenLian commented Mar 13, 2020

The main purpose of this PR is to set a new enum field VerificationAuthType for users to choose their own verification levels. Users could have three levels of verification: certificate and hostname check, certificate check only, and no check. The reasons for this change is:

  1. We used to determine which level users are intended to set by checking some particular fields, such as if user set the root credential reloading field, etc. This not only needs users to read comments carefully, but also add a lot of if-else statements in our code that is hard to debug. By asking users to explicitly choosing one level, in accompany with the custom verification check, we offer more flexible APIs for users.
  2. This is also to keep implementation align with other languages, like the constants in C core.

Note that with this change, we are able to provide better custom verification checks, both on client side and server side. Before, this feature is called "custom server authorization check", and only available on client side. Now since we also give server side the ability to choose "skip all verifications", server side also needs some "custom verification mechanism", so I changed the verification mechanism to apply to both sides, and updated the comments accordingly.
In sum, there should be no "custom server authorization check" any more, but instead "custom verification check".

I also added some unit tests and a new integration test for the newly added/changed behaviors.

@ZhenLian ZhenLian changed the title [Not Ready For Review]advancedtls: add fine-grained verification levels in XXXOptions advancedtls: add fine-grained verification levels in XXXOptions Mar 13, 2020
@ZhenLian
Copy link
Contributor Author

@cesarghali Could you please take a look at this when you have time? Thanks in advance!

@menghanl menghanl added the Type: Feature New features or improvements in behavior label Mar 13, 2020
@menghanl menghanl added this to the 1.29 Release milestone Mar 13, 2020
Copy link
Contributor

@cesarghali cesarghali left a comment

Choose a reason for hiding this comment

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

Thank you for the PR Zhen. Maybe I'm missing something but my main concern is that we're providing a security library with a way to completely disable it (using SkipVerification), is that intended?

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
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
security/advancedtls/advancedtls_test.go Show resolved Hide resolved
@ZhenLian
Copy link
Contributor Author

Thank you for the PR Zhen. Maybe I'm missing something but my main concern is that we're providing a security library with a way to completely disable it (using SkipVerification), is that intended?

Yes, that's intended. You can find an equivalent part in C core as well. The only difference is that in C core that GRPC_TLS_SKIP_ALL_SERVER_VERIFICATION only applies to the client side, but here we can apply this field both to server side and client side.
The underlying assumption is that, if users choose to disable all the checks, they would need to provide their own mechanism for doing their own custom checks(and that's the requirement from a lot of users). We could ask them to provide a custom check if they disable all the checks, but there is really not an effective way to see if their custom checks are good or not, from a security point of view.
@jiangtaoli2016 FYI.

@ZhenLian
Copy link
Contributor Author

@cesarghali all the comments are resolved/replied. Could you please take a second look?

@easwars easwars modified the milestones: 1.29 Release, 1.30 Release Apr 8, 2020
@ZhenLian
Copy link
Contributor Author

@cesarghali Hi, just want to let you know that this is ready for review :-) it would be good if you could take a look these days. Thanks😊

Copy link
Contributor

@cesarghali cesarghali left a comment

Choose a reason for hiding this comment

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

LGTM with one nit.

security/advancedtls/advancedtls.go Outdated Show resolved Hide resolved
security/advancedtls/advancedtls_test.go Show resolved Hide resolved
@ZhenLian
Copy link
Contributor Author

@easwars
Hi Easwar, our team has done the security part of the review. Can you please take a second look on the Go style & coding paradigm? Feel free to reassign it to your other team members if you find it better to do so. Thank you so much!

@ZhenLian ZhenLian requested a review from easwars April 20, 2020 23:36
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.

Mostly nits.

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

@easwars Thanks for the review! Now all comments fixed. It would be great if you could take a look again, :-)

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.

Really sorry for having you reformat all comments. I only wanted to make changes to the comments that you had touched in this PR. Anyways, thank you for that.

I think I have only a couple of minor nits. Other than that, PR looks good to me.

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

Really sorry for having you reformat all comments. I only wanted to make changes to the comments that you had touched in this PR. Anyways, thank you for that.

@easwars No worries at all! It's my pleasure to improve the overall code quality:-)
If everything looks good to you, can you help me merge the PR please? I have no write permission. Thanks!

@easwars easwars merged commit f313ade into grpc:master Apr 22, 2020
@ZhenLian ZhenLian deleted the zhen_spiffe_smallfix_1 branch April 25, 2020 18:59
@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.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants