tls: use X509_V_FLAG_NO_CHECK_TIME to implement allow_expired_certificate#23320
Merged
ggreenway merged 2 commits intoenvoyproxy:mainfrom Oct 3, 2022
Merged
tls: use X509_V_FLAG_NO_CHECK_TIME to implement allow_expired_certificate#23320ggreenway merged 2 commits intoenvoyproxy:mainfrom
ggreenway merged 2 commits intoenvoyproxy:mainfrom
Conversation
ggreenway
requested changes
Oct 3, 2022
source/extensions/transport_sockets/tls/cert_validator/utility.cc
Outdated
Show resolved
Hide resolved
|
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
…cate Newer revisions of BoringSSL provide OpenSSL's X509_V_FLAG_NO_CHECK_TIME API. Envoy currently implements allow_expired_certificate by using the X509_STORE_CTX verify callback. This callback is difficult for BoringSSL to support as it exposes internal details of the verification process. (What order errors come in, that there isn't a real path-builder, etc.) It also leaves X509_STORE_CTX in a funny state (per the error-reporting bug fixed before this commit). This change replaces this callback with a more direct API. Without removing the callback, it will be very difficult to migrate Envoy to the future replacement certificate verifier in BoringSSL. As Envoy doesn't track BoringSSL HEAD, and needs to support a very old boringssl_fips config, I've retained the callback-based path for when X509_V_FLAG_NO_CHECK_TIME is unavailable. Once Envoy's minimum BoringSSL revision is new enough, this can all be removed in favor of just calling X509_STORE_CTX_set_flags in default_validator.cc and spiffe_validator.cc. utility_test.cc is removed because it wasn't actually testing anything, just that callback recognized a pair of return values, without even driving a certificate verification. The actual test coverage came from spiffe_validator_test and ssl_socket_test. Signed-off-by: David Benjamin <davidben@google.com>
…certificate Signed-off-by: David Benjamin <davidben@google.com>
ef2ff05 to
3524404
Compare
Contributor
Author
|
Forced-pushed to rebase atop the merge of #23319. |
Member
In the future, please merge instead of rebasing; it makes PRs to easier to review on github (due to github's poor support for rebasing). |
davidben
added a commit
to davidben/envoy
that referenced
this pull request
Nov 4, 2022
Both things the comment says are inaccurate: After envoyproxy#23320, this mechanism no longer uses the X509_STORE callback, but a flag. After envoyproxy#21417, Envoy is also not (always) using SSL_CTX_set_cert_verify_callback, but a different one. Just remove the comment altogether, as it doesn't seem to provide any value. All it's saying is that, because Envoy still uses `store` at the end of the day, configuring things on `store` does stuff. But the whole function configures things on `store`, so it's not specific to that block anyway. Signed-off-by: David Benjamin <davidben@google.com>
lizan
pushed a commit
that referenced
this pull request
Nov 7, 2022
Both things the comment says are inaccurate: After #23320, this mechanism no longer uses the X509_STORE callback, but a flag. After #21417, Envoy is also not (always) using SSL_CTX_set_cert_verify_callback, but a different one. Just remove the comment altogether, as it doesn't seem to provide any value. All it's saying is that, because Envoy still uses `store` at the end of the day, configuring things on `store` does stuff. But the whole function configures things on `store`, so it's not specific to that block anyway. Signed-off-by: David Benjamin <davidben@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR includes PR #23319 as its first commit. For review, I'd suggest just looking at the second commit, and leaving the first commit to be reviewed in the base PR.The intent was to upload a pair of dependent PRs. I've made them into separate PRs because it looked like you all prefer one commit per PR (since PR descriptions ask for a commit message), and it seemed worth keeping the changes ultimately separate. Let me know if this isn't the preferred workflow.Update: #23319 is now merged, so this has been rebased on top of it.
Commit Message:
Newer revisions of BoringSSL provide OpenSSL's X509_V_FLAG_NO_CHECK_TIME API. Envoy currently implements allow_expired_certificate by using the X509_STORE_CTX verify callback. This callback is difficult for BoringSSL to support as it exposes internal details of the verification process. (What order errors come in, that there isn't a real path-builder, etc.) It also leaves X509_STORE_CTX in a funny state (per the error-reporting bug fixed before this commit).
This change replaces this callback with a more direct API. Without removing the callback, it will be very difficult to migrate Envoy to the future replacement certificate verifier in BoringSSL.
As Envoy doesn't track BoringSSL HEAD, and needs to support a very old boringssl_fips config, I've retained the callback-based path for when X509_V_FLAG_NO_CHECK_TIME is unavailable. Once Envoy's minimum BoringSSL revision is new enough, this can all be removed in favor of just calling X509_STORE_CTX_set_flags in default_validator.cc and spiffe_validator.cc.
utility_test.cc is removed because it wasn't actually testing anything, just that callback recognized a pair of return values, without even driving a certificate verification. The actual test coverage came from spiffe_validator_test and ssl_socket_test.
Signed-off-by: David Benjamin davidben@google.com
Additional Description:
Risk Level: Low
Testing:
bazelist test //test/extensions/transport_sockets/tls/..., also with BoringSSL updated to HEAD to make sure both codepaths workedDocs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A