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

Default the pkcs11 code to use sha256 for OAEP padding #61

Merged
merged 3 commits into from
Apr 28, 2022

Conversation

stefanberger
Copy link
Collaborator

This PR modifies the pkcs11 code to default to sha256

Fix a comment because the empty string of the hash defaults to sha1
and not sha256.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
@stefanberger stefanberger force-pushed the pkcs11_to_sha256_default branch 5 times, most recently from 323b585 to 6fde93a Compare April 27, 2022 21:07
@stefanberger stefanberger changed the title Default the pkcs11 code to sha256 Default the pkcs11 code to use sha256 for OAEP padding Apr 27, 2022
Copy link
Collaborator

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

Minor non-blocking nits, lgtm

// } ,
// [...]
// }
// Note: More recent versions of this code explicityly write 'sha1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit : explicitly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

crypto/pkcs11/pkcs11helpers_test.go Show resolved Hide resolved
crypto/pkcs11/pkcs11helpers.go Outdated Show resolved Hide resolved
@stefanberger stefanberger force-pushed the pkcs11_to_sha256_default branch 2 times, most recently from 3e20e9f to 0d7f671 Compare April 28, 2022 11:30
Remove the variable OAEPDefaultHash and explicitly store 'sha1'
in the JSON's Hash field.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
In case the user doesn't set the environment variable
OCICRYPT_OAEP_HASHALG sha256 will be used now. This breaks default
usage with SoftHSM because the only hash it currently (v2.6.1) supports
is sha1. So a user of SoftHSM now has to set the environment variable
to 'sha1' and we have to adjust the test case because of this.

SoftHSM link to OAEP only supporting sha1:
https://github.com/opendnssec/SoftHSMv2/blob/7f99bedae002f0dd04ceeb8d86d59fc4a68a69a0/src/lib/SoftHSM.cpp#L3123-L3127

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
@lumjjb lumjjb merged commit 566b808 into containers:main Apr 28, 2022
@lsm5
Copy link
Member

lsm5 commented Apr 28, 2022

Thanks a lot @stefanberger @lumjjb for this. Can we get new release with this please?

@lumjjb
Copy link
Collaborator

lumjjb commented Apr 28, 2022

Before we release, want to make sure that we capture this change in the rust version as well @arronwy , any concerns of this from the rust side?

@stefanberger stefanberger deleted the pkcs11_to_sha256_default branch April 28, 2022 14:35
@arronwy
Copy link
Collaborator

arronwy commented May 2, 2022

Before we release, want to make sure that we capture this change in the rust version as well @arronwy , any concerns of this from the rust side?

@lumjjb pkcs11 keywrapper is not implemented in rust version yet, this default setting will not impact current rust version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants