access log / route headers: Expose more downstream TLS information #7019
Conversation
* New accessors added for querying information about the established TLS session: issuerPeerCertificate(), ciphersuiteId(), ciphersuiteString(), tlsVersion(). * access_log_formatter.cc and header_formatter.cc updated with new variables for exposing downstream TLS information. * grpc_access_log_impl.cc updated to fill in existing (but previously-hidden) ciphersuite and tls version fields. * Unit tests updated to exercise the above changes. Signed-off-by: Mike Grass <mgrass@salesforce.com>
Signed-off-by: Mike Grass <mgrass@salesforce.com>
|
/retest |
|
🔨 rebuilding |
|
🤷♀️ nothing to rebuild. |
Signed-off-by: Mike Grass <mgrass@salesforce.com>
|
@mattklein123 thanks for assigning a reviewer! @zuercher looking forward to your feedback! |
|
@zuercher -- anything I can do to help move this review along? |
Signed-off-by: Mike Grass <mgrass@salesforce.com>
| case CertName::Subject: | ||
| name = X509_get_subject_name(&cert); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Usually we'll add a default case with a NOT_REACHED_GCOVR_EXCL_LINE statement to help detect missing cases.
There was a problem hiding this comment.
Fixed, thank you!
There was a problem hiding this comment.
This default shouldn't be needed at all since you cover all cases. Can you remove?
There was a problem hiding this comment.
@mattklein123 Sure -- I've removed it in the latest commit.
Signed-off-by: Mike Grass <mgrass@salesforce.com>
…ing cases Signed-off-by: Mike Grass <mgrass@salesforce.com>
|
Thanks for the review @zuercher! Is there anyone else who needs to review before this can be merged? |
| case CertName::Subject: | ||
| name = X509_get_subject_name(&cert); | ||
| break; | ||
| } |
There was a problem hiding this comment.
This default shouldn't be needed at all since you cover all cases. Can you remove?
Signed-off-by: Mike Grass <mgrass@salesforce.com>
|
@mikegrass friendly request to not force push in the future, it makes reviews more difficult. Thank you! |
@mattklein123 apologies for that! I had moved to a new computer just yesterday and had neglected to run the Thank you for the review! |
Description:
Risk Level: Low
Testing: Unit tests and ad-hoc manual testing
Docs Changes: Updated docs/root/configuration/access_log.rst and docs/root/configuration/http_conn_man/headers.rst with information on new variables.
Release Notes: Updated docs/root/intro/version_history.rst to note the changes.
More incremental progress on #3296.