Replace and Deprecate TS_VERIFY_CTX Functions and Add Documentation #24701
Conversation
This file contains 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
Fixes openssl#18854: Replace and deprecate the functions `TS_VERIFY_CTX_set_data`, `TS_VERIFY_CTX_set_store`, `TS_VERIFY_CTX_set_certs`, `TS_VERIFY_CTX_set_imprint` with new versions: `TS_VERIFY_CTX_set0_data`, `TS_VERIFY_CTX_set0_store`, `TS_VERIFY_CTX_set0_certs` and `TS_VERIFY_CTX_set0_imprint`. The previous functions had poorly documented memory handling, potentially leading to memory leaks. The new functions improve memory management and provide clearer usage. Also, update existing code to use the new function calls instead of the deprecated ones.
|
@erbsland-dev looks like there are a few minor doc nits to correct |
github-actions
bot
added
the
severity: ABI change
erbsland-dev
force-pushed
the
issue-18854
branch
2 times, most recently
from
f8bbb2e
to
d2f1b19
Compare
Mark the existing `TS_VERIFY_CTX_set_certs` function as deprecated in the documentation. Add missing documentation for the deprecated functions `TS_VERIFY_CTX_set_data`, `TS_VERIFY_CTX_set_imprint`, and `TS_VERIFY_CTX_set_store`. Write missing documentation for the following functions: - `TS_VERIFY_CTX_new` - `TS_VERIFY_CTX_init` - `TS_VERIFY_CTX_free` - `TS_VERIFY_CTX_cleanup` - `TS_VERIFY_CTX_set_flags` - `TS_VERIFY_CTX_add_flags` - `TS_VERIFY_CTX_set0_data` - `TS_VERIFY_CTX_set0_imprint` - `TS_VERIFY_CTX_set0_store` - `TS_VERIFY_CTX_set0_certs`
erbsland-dev
force-pushed
the
issue-18854
branch
from
d2f1b19
to
f3562d7
Compare
|
@nhorman I fixed the documentation issues. |
nhorman
approved these changes
Jun 21, 2024
shahsb
approved these changes
Jun 23, 2024
t8m
added
branch: master
t8m
requested changes
Jun 24, 2024
|
|
||
| TS_VERIFY_CTS_set_certs() is a misspelled version of TS_VERIFY_CTX_set_certs() | ||
| which takes the same parameters and returns the same result. | ||
|
|
There was a problem hiding this comment.
Nit: please drop this extra empty line
|
|
||
| STACK_OF(X509) *TS_VERIFY_CTS_set_certs(TS_VERIFY_CTX *ctx, | ||
| STACK_OF(X509) *certs); | ||
|
|
| # endif | ||
| int TS_VERIFY_CTX_set0_data(TS_VERIFY_CTX *ctx, BIO *b); | ||
| # ifndef OPENSSL_NO_DEPRECATED_3_4 | ||
| OSSL_DEPRECATEDIN_3_4_FOR("Memory usage was not defined, replace with TS_VERIFY_CTX_set0_imprint().") |
There was a problem hiding this comment.
I'd suggest not putting the deprecation reason into the message. Or at least make it shorter Unclear semantics, replace with....
| int TS_VERIFY_CTX_set0_imprint(TS_VERIFY_CTX *ctx, | ||
| unsigned char *hexstr, long len); | ||
| # ifndef OPENSSL_NO_DEPRECATED_3_4 | ||
| OSSL_DEPRECATEDIN_3_4_FOR("Previous ctx data was not released, replace with TS_VERIFY_CTX_set0_store().") |
| # ifndef OPENSSL_NO_DEPRECATED_3_0 | ||
| # define TS_VERIFY_CTS_set_certs(ctx, cert) TS_VERIFY_CTX_set_certs(ctx,cert) | ||
| # endif | ||
| # ifndef OPENSSL_NO_DEPRECATED_3_4 | ||
| OSSL_DEPRECATEDIN_3_4_FOR("Previous ctx data was not released, replace with TS_VERIFY_CTX_set0_certs().") |
| key. Then the client verifies the received TST using the server's certificate | ||
| chain. | ||
|
|
||
| For all the following methods, unless noted otherwise, B<ctx> is the |
There was a problem hiding this comment.
I know you're just copying the page however as you're already touching this, could you please use I<ctx> and similarly elsewhere throughout the manpage where names of arguments are referenced?
|
(Tests exempted because we do not have any TS subsystem testcases at all yet.) |
|
@erbsland-dev Could you please also rebase this to resolve the conflict? |
|
@t8m Sure, I'll do it, but it probably won't be until the weekend. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 pull request addresses two main areas related to the
TS_VERIFY_CTXfunctions:1. Replace and Deprecate Functions
Fixes #18854 by replacing and deprecating the following functions due to poorly documented memory handling, which could lead to memory leaks:
TS_VERIFY_CTX_set_dataTS_VERIFY_CTX_set_storeTS_VERIFY_CTX_set_certsTS_VERIFY_CTX_set_imprintThese functions are replaced with:
TS_VERIFY_CTX_set0_dataTS_VERIFY_CTX_set0_storeTS_VERIFY_CTX_set0_certsTS_VERIFY_CTX_set0_imprintUpdates have been made to existing code to use the new function calls instead of the deprecated ones.
2. Add and Update Documentation
TS_VERIFY_CTX_set_certsfunction as deprecated in the documentation.TS_VERIFY_CTX_set_dataTS_VERIFY_CTX_set_imprintTS_VERIFY_CTX_set_storeTS_VERIFY_CTX_newTS_VERIFY_CTX_initTS_VERIFY_CTX_freeTS_VERIFY_CTX_cleanupTS_VERIFY_CTX_set_flagsTS_VERIFY_CTX_add_flagsTS_VERIFY_CTX_set0_dataTS_VERIFY_CTX_set0_imprintTS_VERIFY_CTX_set0_storeTS_VERIFY_CTX_set0_certsThese changes improve memory management, enhance future extensibility, and provide clearer usage guidelines through updated and additional documentation.