Skip to content

Expose NoOpCertificateVerifier to C++#29322

Merged
ZhenLian merged 3 commits intogrpc:masterfrom
ZhenLian:zhen_noop_verifier
Apr 27, 2022
Merged

Expose NoOpCertificateVerifier to C++#29322
ZhenLian merged 3 commits intogrpc:masterfrom
ZhenLian:zhen_noop_verifier

Conversation

@ZhenLian
Copy link
Copy Markdown
Contributor

@ZhenLian ZhenLian commented Apr 6, 2022

This is originally raised in an internal bug.
The original idea for not having NoOpCertificateVerifier is to raise security bars. NoOpCertificateVerifier can expose security issues if not properly used. So we tried to "hide" it and hope that will help us limit its usage.
As Advanced TLS becomes popular, there are more usages of NoOpCertificateVerifier internally, and their reasons for using it are all valid. In that case, we can expose this in C++, with better documentations, so that users would get a sense of what are implied when using this class.
I will clean-up all the internal sites as long as this goes in.

// Note: using this solely without any other authentication mechanisms on the
// peer identity will leave your applications to the MITM(Man-In-The-Middle)
// attacks. Users should avoid doing so in production environments.
class NoOpCertificateVerifier : public ExternalCertificateVerifier {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why make this an external verifier? If we're providing this out of the box, let's just implement it inside core, the same way that we do for the hostname verifier, so that we don't have an extra and unnecessary level of indirection.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Now the NoOpCertificateVerifier is exposed through core. Could you take a look again? Thank you!

@ZhenLian ZhenLian added the release notes: yes Indicates if PR needs to be in release notes label Apr 26, 2022
Copy link
Copy Markdown
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Looks great!

@ZhenLian
Copy link
Copy Markdown
Contributor Author

All failures seem irrelevant(seems something wrong with the MacOS test infra):
Artifact Build MacOS - #27932
Bazel C/C++ Dbg MacOS - an irrelevant h2_sockpair_test failed
Distribution Tests Ruby MacOS - #29487

I am going to merge now.


A note for the importer:
This PR is unlikely to get rolled back. But just in case it breaks the import: feel free to roll it back, and I will work on it once I am back(I am out until next Thursday).

@ZhenLian ZhenLian merged commit 2badafb into grpc:master Apr 27, 2022
@copybara-service copybara-service Bot added the imported Specifies if the PR has been imported to the internal repository label Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bloat/none imported Specifies if the PR has been imported to the internal repository lang/c++ per-call-memory/neutral release notes: yes Indicates if PR needs to be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants