TLS library-agnostic X509 certificate interrogation functions#1057
Closed
rousskov wants to merge 25 commits into
Closed
TLS library-agnostic X509 certificate interrogation functions#1057rousskov wants to merge 25 commits into
rousskov wants to merge 25 commits into
Conversation
---- This code is based on Amos and my work on official PR 831. I have extracted certificate manipulation functions from that PR branch (commit b96c3c1 dated 2022-01-18) so that they can be polished and reused in other projects sooner.
The name "gadgets" indicates a collection naming failure, but we actually know what unites all these things! They all extract information from an X509 certificate.
TODO: Try to move more legacy certificate-related declarations (that are not forward declarations) away from src/security/forward.h.
The function argument already firmly establishes certificate manipulation context. We do not need to say "Cert" explicitly and many callers would be saying "cert" or equivalent twice without this change. The only doubt is the IsIssuedBy() naming because that function deals with two certs and their order is important. I think the current order is natural enough to avoid most reasonable doubts and, perhaps more importantly, adding the "Cert" prefix does not remove all reasonable doubts anyway.
... because known callers already report them anyway OR should report them (and probably will do so eventually). Alternatively, we could create pairs or parameterized extractors so that the caller can decide whether they want the extractor to dump the extracted information to cache.log as a side effect. However, that adds work for us now and for the callers in the future so lets KISS for now.
... instead of OpenSSL X509_check_issued(). The only case I could not convert is in ssl/gadgets.cc which is used by certificate helpers which cannot be linked with libsecurity yet.
Also fixes memory leak when debugging section 83 at level 3+ of an OpenSSL-using Squid (missing cleanup in clientNegotiateSSL()). This change speeds up ssl_verify_cb() and other functions that used to extract and copy certificate name into a buffer even when that name was unused because elevated debugging levels were not enabled.
Contributor
Author
|
This PR is based on @yadij work on #831, but all bugs are my responsibility. I have extracted certificate manipulation functions from that PR branch (as of commit b96c3c1 dated 2022-01-18) so that they can be polished (here) and used (here and in other projects) sooner. Technically, adding those functions was outside #831 primary scope, so this PR also provides a more focused/smaller change. |
rousskov
commented
May 18, 2022
yadij
requested changes
May 18, 2022
The official/natural order was giving me linking problems at some point, but I can no longer reproduce them so there is, hopefully, no need to fiddle with this order (in this PR).
TODO: Avoid code duplication
This also fixes a (usually symptom-free) sslcrtd bug: C strings allocated by OpenSSL were freed by xfree() instead of OPENSSL_free().
... text by removing an extra space character.
It would be nice to avoid this memory allocation, and OpenSSL may have enough knobs for doing so (Christos wrote a sketch that may be sufficient), but the amount of special code this optimization will require will be significant. And once we add GnuTLS support (if the same optimization is possible there at all), we would be looking at a rather complex optimization with little performance gain (at least based on the current Certificate printing use cases). A much better strategy may be to parameterize both TLS libraries to proxy their memory allocations through Squid's pooling-capable libmem!
rousskov
commented
May 18, 2022
rousskov
left a comment
rousskov
left a comment
Contributor
Author
There was a problem hiding this comment.
I believe I have addressed all of your concerns. One of the change request was only partially addressed; if you insist on fixing those GnuTLS memory leaks in this PR, I will do that.
I also removed one branch-added comment in 9846104.
rousskov
added
the
S-waiting-for-reviewer
yadij
approved these changes
May 20, 2022
yadij
added
M-cleared-for-merge
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
pushed a commit
that referenced
this pull request
May 20, 2022
Use added X509_check_issued() replacements. The only case left is in src/ssl/gadgets.cc which is used by certificate helpers that cannot be linked with libsecurity yet. Use added X509_NAME_oneline() replacements, where feasible. This change speeds up ssl_verify_cb() and other functions that used to extract and copy certificate name into a buffer even when that name was unused because debugging levels were not elevated enough, including by default. Also fixes memory leak when debugging section 83 at level 3+ of an OpenSSL-using Squid (missing name cleanup in clientNegotiateSSL()). Also fixes a (usually symptom-free) sslcrtd bug: C strings allocated by OpenSSL were freed by xfree() instead of OPENSSL_free(). Co-authored-by: Amos Jeffries <squid3@treenet.co.nz>
squid-anubis
added
M-failed-staging-checks
rousskov
removed
the
M-failed-staging-checks
squid-anubis
pushed a commit
that referenced
this pull request
May 20, 2022
Use added X509_check_issued() replacements. The only case left is in src/ssl/gadgets.cc which is used by certificate helpers that cannot be linked with libsecurity yet. Use added X509_NAME_oneline() replacements, where feasible. This change speeds up ssl_verify_cb() and other functions that used to extract and copy certificate name into a buffer even when that name was unused because debugging levels were not elevated enough, including by default. Also fixes memory leak when debugging section 83 at level 3+ of an OpenSSL-using Squid (missing name cleanup in clientNegotiateSSL()). Also fixes a (usually symptom-free) sslcrtd bug: C strings allocated by OpenSSL were freed by xfree() instead of OPENSSL_free(). Co-authored-by: Amos Jeffries <squid3@treenet.co.nz>
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
added
M-merged
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.
Use added X509_check_issued() replacements. The only case left is in
src/ssl/gadgets.cc which is used by certificate helpers that cannot be
linked with libsecurity yet.
Use added X509_NAME_oneline() replacements, where feasible. This change
speeds up ssl_verify_cb() and other functions that used to extract and
copy certificate name into a buffer even when that name was unused
because debugging levels were not elevated enough, including by default.
Also fixes memory leak when debugging section 83 at level 3+ of an
OpenSSL-using Squid (missing name cleanup in clientNegotiateSSL()).
Also fixes a (usually symptom-free) sslcrtd bug: C strings allocated by
OpenSSL were freed by xfree() instead of OPENSSL_free().
Co-authored-by: Amos Jeffries squid3@treenet.co.nz