Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify the int param getter documentation #17445

Closed
wants to merge 2 commits into from

Conversation

mattcaswell
Copy link
Member

OSSL_PARAMs that are of type OSSL_PARAM_INTEGER or
OSSL_PARAM_UNSIGNED_INTEGER can be obtained using any of the functions
EVP_PKEY_get_int_param(), EVP_PKEY_get_size_t_param() or
EVP_PKEY_get_bn_param(). The former two will fail if the parameter is too
large to fit into the C variable. We clarify this in the documentation.

OSSL_PARAMs that are of type OSSL_PARAM_INTEGER or
OSSL_PARAM_UNSIGNED_INTEGER can be obtained using any of the functions
EVP_PKEY_get_int_param(), EVP_PKEY_get_size_t_param() or
EVP_PKEY_get_bn_param(). The former two will fail if the parameter is too
large to fit into the C variable. We clarify this in the documentation.
@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer branch: 3.0 Merge to openssl-3.0 branch labels Jan 7, 2022
@mouse07410
Copy link
Contributor

Nice, thanks!

Is there a way to guide the reader to which of the functions he should use to retrieve the param value?

I.e., is it always recommended to use EVP_PKEY_get_bn_param() if it's of "arbitrary length"? Or is there a function that would tell the size? Or should one try _get_int_param(), if it fails - proceed to _get_size_t_param(), and finally progress to _get_bn_param()???

@@ -37,6 +37,15 @@ EVP_PKEY_gettable_params() returns a constant list of I<params> indicating
the names and types of key parameters that can be retrieved.
See L<OSSL_PARAM(3)> for information about parameters.

An B<OSSL_PARAM> of type B<OSSL_PARAM_INTEGER> or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in the notes section? It seems slightly out of place here. It's also more likely to be read if here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I would have this in NOTES

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep it here. As @paulidale says it is much more likely to be read here, and I think this is critical information for understanding how these functions work.

@t8m
Copy link
Member

t8m commented Jan 10, 2022

Nice, thanks!

Is there a way to guide the reader to which of the functions he should use to retrieve the param value?

I.e., is it always recommended to use EVP_PKEY_get_bn_param() if it's of "arbitrary length"? Or is there a function that would tell the size? Or should one try _get_int_param(), if it fails - proceed to _get_size_t_param(), and finally progress to _get_bn_param()???

It's IMO always - apply your reason about what the meaning of the parameter actually is. If it is some kind of length parameter get_size_t should be used. If it is some small integer parameter, you can use get_int_param. For the rest, use get_bn_param. Of course there are border-line cases such as the RSA e value which usually fits into int but can be an arbitrary big number. For this you could use the fallback approach but going right away with get_bn_param() would of course work as well.

Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be OK with keeping it here or moving it to NOTES, no problem with either choice.

doc/man3/EVP_PKEY_gettable_params.pod Outdated Show resolved Hide resolved
@paulidale
Copy link
Contributor

If you have the param, you can inspect the size directly. It is a public structure.

@mattcaswell
Copy link
Member Author

Fixup pushed addressing the typo

@mattcaswell
Copy link
Member Author

It's IMO always - apply your reason about what the meaning of the parameter actually is. If it is some kind of length parameter get_size_t should be used. If it is some small integer parameter, you can use get_int_param. For the rest, use get_bn_param. Of course there are border-line cases such as the RSA e value which usually fits into int but can be an arbitrary big number. For this you could use the fallback approach but going right away with get_bn_param() would of course work as well.

Can we indicate this somehow in the documentation for each parameter (not in this PR)? The parameter documentation all has a standard form, e.g.

=item "p" (B<OSSL_PKEY_PARAM_EC_P>) <unsigned integer>

Could we change that to something like:

=item "p" (B<OSSL_PKEY_PARAM_EC_P>) <unsigned integer> (typically use a B<BIGNUM>)

or

=item "size" (B<OSSL_DIGEST_PARAM_SIZE>) <unsigned integer> (typically use a C B<size_t>)

@t8m t8m added approval: done This pull request has the required number of approvals triaged: documentation The issue/pr deals with documentation (errors) and removed approval: review pending This pull request needs review by a committer labels Jan 10, 2022
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Jan 11, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jan 11, 2022
@paulidale
Copy link
Contributor

Merged to 3.0 and master.

@paulidale paulidale closed this Jan 11, 2022
openssl-machine pushed a commit that referenced this pull request Jan 11, 2022
OSSL_PARAMs that are of type OSSL_PARAM_INTEGER or
OSSL_PARAM_UNSIGNED_INTEGER can be obtained using any of the functions
EVP_PKEY_get_int_param(), EVP_PKEY_get_size_t_param() or
EVP_PKEY_get_bn_param(). The former two will fail if the parameter is too
large to fit into the C variable. We clarify this in the documentation.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #17445)

(cherry picked from commit 254217a)
openssl-machine pushed a commit that referenced this pull request Jan 11, 2022
OSSL_PARAMs that are of type OSSL_PARAM_INTEGER or
OSSL_PARAM_UNSIGNED_INTEGER can be obtained using any of the functions
EVP_PKEY_get_int_param(), EVP_PKEY_get_size_t_param() or
EVP_PKEY_get_bn_param(). The former two will fail if the parameter is too
large to fit into the C variable. We clarify this in the documentation.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #17445)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants