-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix/fix #3570
Fix/fix #3570
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we unify ca-signed and self-signeed as one?
@@ -18,6 +18,7 @@ namespace nebula { | |||
std::shared_ptr<wangle::SSLContextConfig> sslContextConfig() { | |||
auto sslCfg = std::make_shared<wangle::SSLContextConfig>(); | |||
sslCfg->addCertificate(FLAGS_cert_path, FLAGS_key_path, FLAGS_password_path); | |||
sslCfg->clientVerification = folly::SSLContext::VerifyClientCertificate::DO_NOT_REQUEST; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the certificate from the client could be omitted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about using "always" and failed if verification fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not, it just check CA of server (which is new feature after library upgrading), so I disable it to keep same with before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still believe we should not distinguish ca-signed and self-signed cert, maybe address this in another PR.
Could you detail this proposal? |
What type of PR is this?
What does this PR do?
Fit the library ssl configuration upgrading.
Need #3471 , seems don't support plaintext in ssl port again.
Which issue(s)/PR(s) this PR relates to?
Special notes for your reviewer, ex. impact of this fix, etc:
Additional context/ Design document:
Checklist:
Release notes:
Please confirm whether to be reflected in release notes and how to describe: