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

apps/pkcs12: Not writing the privateKey.pem file until the import password is verified #23729

Conversation

naaysayer
Copy link
Contributor

@naaysayer naaysayer commented Mar 2, 2024

Fixes #904

CLA: trivial

Relocating the code responsible for opening the output file to the 'dump' label. All validation was performed before, and a file would not be written if any of the validation failed.

Copy link
Contributor

@shahsb shahsb left a comment

Choose a reason for hiding this comment

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

Ok with Trivial

@t8m t8m 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: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing branch: 3.2 Merge to openssl-3.2 labels Mar 12, 2024
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Mar 12, 2024
@t8m
Copy link
Member

t8m commented Mar 12, 2024

OK with CLA:trivial as this is just moving the code around.

@t8m t8m added the cla: trivial One of the commits is marked as 'CLA: trivial' label Mar 12, 2024
@github-actions github-actions bot added severity: fips change The pull request changes FIPS provider sources severity: ABI change This pull request contains ABI changes labels Mar 12, 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

@openssl-machine
Copy link
Collaborator

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

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, agree with trivial

@beldmit beldmit removed the approval: review pending This pull request needs review by a committer label May 13, 2024
@beldmit
Copy link
Member

beldmit commented May 13, 2024

close/open to rerun tests

@beldmit beldmit closed this May 13, 2024
@beldmit beldmit reopened this May 13, 2024
@github-actions github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label May 13, 2024
@beldmit
Copy link
Member

beldmit commented May 13, 2024

BTW, why do we have ABI change label on this PR?

@bernd-edlinger
Copy link
Member

Hmm, the main reason why #904 did not make any progress so far, is probably
that there are many other commands that have the same behavior that the output file
is empty when the output password was not typed in correctly.
e.g.
apps/openssl genpkey -paramfile test/testec-p112r1.pem -aes-128-cbc -out xxx.pem
the out fille is also created before the password is entered, and will be empty if password
is too short, or something goes wrong.
Are there other commands where the output file is not created when the password is empty,
or will this be the first one?

@naaysayer
Copy link
Contributor Author

Hmm, the main reason why #904 did not make any progress so far, is probably that there are many other commands that have the same behavior that the output file is empty when the output password was not typed in correctly. e.g. apps/openssl genpkey -paramfile test/testec-p112r1.pem -aes-128-cbc -out xxx.pem the out fille is also created before the password is entered, and will be empty if password is too short, or something goes wrong. Are there other commands where the output file is not created when the password is empty, or will this be the first one?

ok, I will look into other commands. Should I fix all of them in the same manner as the current one?

@bernd-edlinger
Copy link
Member

It would be good to have consistent behavior between all the different commands.
Even if that means setting the output file to empty, at ideally there should be
a general rule that is used everywhere and not exceptions.

@t8m t8m added the approval: done This pull request has the required number of approvals label May 13, 2024
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label May 14, 2024
@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label May 14, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented May 14, 2024

Merged to all the active branches. Thank you for your contribution.

@t8m t8m closed this May 14, 2024
openssl-machine pushed a commit that referenced this pull request May 14, 2024
…rd is verified

Fixes #904

CLA: trivial

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23729)

(cherry picked from commit f546257)
openssl-machine pushed a commit that referenced this pull request May 14, 2024
…rd is verified

Fixes #904

CLA: trivial

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23729)
openssl-machine pushed a commit that referenced this pull request May 14, 2024
…rd is verified

Fixes #904

CLA: trivial

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23729)

(cherry picked from commit f546257)
openssl-machine pushed a commit that referenced this pull request May 14, 2024
…rd is verified

Fixes #904

CLA: trivial

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23729)

(cherry picked from commit f546257)
openssl-machine pushed a commit that referenced this pull request May 14, 2024
…rd is verified

Fixes #904

CLA: trivial

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23729)

(cherry picked from commit f546257)
jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
…rd is verified

Fixes openssl#904

CLA: trivial

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#23729)
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 branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 cla: trivial One of the commits is marked as 'CLA: trivial' severity: ABI change This pull request contains ABI changes tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

openssl creates empty private key file, overwriting existing file, when no file should be created
6 participants