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.

Sign up for GitHub

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

Jump to bottom

APPS/pkeyutl: improve -rawin usability and doc #22910

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

APPS/pkeyutl: improve -rawin usability and doc #22910

wants to merge 3 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Dec 2, 2023

This improves the usability of they pkeyutl app when signing or verifying with Ed25519 and Ed448 keys.

When the -rawin option was not given with them, so far an obscure low-level error was thrown such as

pkeyutl: Error initializing context
80205AEC01000000:error:03000096:digital envelope routines:evp_pkey_signature_init:operation not supported for this keytype:crypto/evp/signature.c:526:

This PR makes sure that the -rawin option is implied for such scenarios, also making the app more convenient to use.
Implementing this required a split of the init_ctx() helper function, which is now less convoluted.

Tests and doc are updated accordingly,
and on this occasion the help output and the doc of the -rawin option are made less confusing.

Checklist
  • documentation is added or updated
  • tests are added or updated

@DDvO DDvO added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: feature The issue/pr requests/adds a feature triaged: documentation The issue/pr deals with documentation (errors) triaged: refactor The issue/pr requests/implements refactoring tests: present The PR has suitable tests present labels Dec 2, 2023
@DDvO DDvO added this to the 3.3.0 milestone Dec 2, 2023
@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/otc but the last update was 92 days ago

@t-j-h t-j-h removed the approval: otc review pending This pull request needs review by an OTC member label Mar 9, 2024
hlandau
hlandau reviewed Mar 9, 2024
View reviewed changes
apps/pkeyutl.c Show resolved Hide resolved
@nhorman nhorman added 3.3.0 Planned eng view Include on 3.3.0 Engineering View Roadmap and removed 3.3.0 Planned eng view Include on 3.3.0 Engineering View Roadmap labels Mar 10, 2024
@nhorman nhorman removed this from the 3.3.0 milestone Mar 26, 2024
@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@beldmit
Copy link
Member

beldmit commented Apr 10, 2024 •
edited

@DDvO could you please answer @hlandau's comment?
Will it work with ed448/25519 when -rawin is specified (scripts backward compatibility)?

@DDvO
Copy link
Contributor Author

DDvO commented Apr 12, 2024

@DDvO could you please answer @hlandau's comment?

Thanks for pointing me to that, and
sorry that I missed that good review comment of March 10th.
Handled it now.

Will it work with ed448/25519 when -rawin is specified (scripts backward compatibility)?

Yes.

Rebased to fix trivial merge conflict in CHANGES.md.

@DDvO DDvO requested review from t-j-h and hlandau April 12, 2024 14:11
beldmit
beldmit reviewed Apr 12, 2024
View reviewed changes
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

Two minor questions

apps/pkeyutl.c Show resolved Hide resolved
test/recipes/20-test_pkeyutl.t Outdated Show resolved Hide resolved
@DDvO DDvO requested a review from beldmit April 12, 2024 18:10
@DDvO DDvO force-pushed the pkeyutl_improve_rawin branch 2 times, most recently from 3a14f5f to da8b344 Compare April 12, 2024 18:53
beldmit
beldmit approved these changes Apr 12, 2024
View reviewed changes
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM

@beldmit
Copy link
Member

beldmit commented Apr 12, 2024 •
edited

@t-j-h would you mind confirming your approval?

@DDvO
Copy link
Contributor Author

DDvO commented Apr 16, 2024

@hlandau or can you give the 2nd approval?

@DDvO
Copy link
Contributor Author

DDvO commented May 1, 2024

Rebased to fix trivial merge conflict

@github-actions github-actions bot added the severity: ABI change This pull request contains ABI changes label May 1, 2024
@DDvO
Copy link
Contributor Author

DDvO commented May 12, 2024

Rebased again to fix trivial merge conflict in CHANGES.md

@DDvO
Copy link
Contributor Author

DDvO commented May 12, 2024

PIng @openssl/committers for 2nd approval / reconfirmation.
Should not be a big deal.

vdukhovni
vdukhovni reviewed May 13, 2024
View reviewed changes
doc/man1/openssl-pkeyutl.pod.in
algorithms (but not EdDSA) needs to be hashed by some message digest algorithm.
The user can specify a digest algorithm by using the B<-digest> option.
This option can only be used with B<-sign> and B<-verify>
and is implied by the Ed25519 and Ed448 algorithms.
Copy link

Choose a reason for hiding this comment

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

Will this still be true if/when the Ed25519ph and Ed448ph algorithms are supported?
Presumably the key algorithm will be the same, and the user will need some way to indicate whether to prehash the data with SHA-512? (I'm not saying this will be a problem, just asking the question, we should have some idea of how the PH variants should be supported before making changes that could hypothetically get in the way).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdukhovni vdukhovni vdukhovni left review comments

@beldmit beldmit beldmit approved these changes

@t-j-h t-j-h Awaiting requested review from t-j-h

@hlandau hlandau Awaiting requested review from hlandau

Assignees
No one assigned
Labels
approval: review pending This pull request needs review by a committer branch: master Merge to master branch severity: ABI change This pull request contains ABI changes tests: present The PR has suitable tests present triaged: documentation The issue/pr deals with documentation (errors) triaged: feature The issue/pr requests/adds a feature triaged: refactor The issue/pr requests/implements refactoring
Projects
OpenSSL Release Schedule
Status: To do
Project Board
Status: New
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@DDvO @openssl-machine @beldmit @vdukhovni @hlandau @t-j-h @nhorman