Remove support for verify_subject_alt_name in CertificateValidationContext#16978
Remove support for verify_subject_alt_name in CertificateValidationContext#16978alyssawilk merged 3 commits intoenvoyproxy:mainfrom
CertificateValidationContext#16978Conversation
Remove support for v2 verify_subject_alt_name field
Signed-off-by: Tianyu Xia <tyxia@google.com>
|
Hi @tyxia, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
tyxia
requested review from
alyssawilk,
asraa,
ggreenway,
lizan and
mattklein123
as code owners
|
/retest |
|
Retrying Azure Pipelines: |
|
Adi, can you take a first pass? |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
LGTM
Thanks for cleaning this up!
|
@htuch @mattklein123 Are there any docs that need an update when removing the implementation of an already deprecated (v2) feature? |
| transport_socket_options != nullptr | ||
| ? transport_socket_options->verifySubjectAltNameListOverride() | ||
| : std::vector<std::string>{}, |
There was a problem hiding this comment.
Just one thing to discuss with reviewers here:
This function arg becomes optional after my change.
Other than current code (creating an empty vector), there are also some other options 1) absl::optional 2) pointer and using nullptr to indicate "not exist".
I currently choose an empty vector approach because 1)it is a lightweight approach from implementation perspective 2) there is a !verify_san_list.empty() check inside verifyCertificate(), which basically provides the same functionality as absl::optional's has_value. And this is better because absl::optional doesn't support pass by const ref and will cause a copy.
But if this path is performance critical, using pointer/nullptr might be a slightly preferred approach. Please let me know what do you think. Thanks!
Thanks for review, Adi! I just posted a comment/questions at the same time you reviewed the CL. Please take another quick look. Thanks! |
Yeah, if the feature is v2 only no docs are needed. |
tyxia
requested review from
PiotrSikora,
Shikugawa,
antoniovicente,
cpakulski,
htuch,
jmarantz,
junr03,
lambdai,
snowp,
yanavlasov and
zuercher
as code owners
…moval Signed-off-by: Tianyu Xia <tyxia@google.com>
…ontext` (envoyproxy#16978) This field has been deprecated in favor of of the match_subject_alt_names field which provides more flexible matching. SAN list can still be specified via transport_socket_options's SAN list override and verified via verifySubjectAltName Risk Level: Low Testing: local test on linux (bazel test //test/...) Signed-off-by: Tianyu Xia <tyxia@google.com>
6 participants
This field has been deprecated in favor of of the
match_subject_alt_namesfield which provides more flexible matching.SAN list can still be specified via transport_socket_options's SAN list override and verified via
verifySubjectAltNameSigned-off-by: Tianyu Xia tyxia@google.com
Risk Level: Low
Testing: local test on linux (bazel test //test/...)