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

Add support for pkcs1 oaep sha256 #206

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

rnro
Copy link
Contributor

@rnro rnro commented Oct 19, 2023

Add support for pkcs1 oaep sha256

Motivation

Add RSA OAEP-sha256 support in addition to the increasingly aged SHA-1
implementation.

Modifications

  • Add a Digest enum to _RSA.Encryption.Padding
  • Add a digest associated type to the .pkcs1_oaep padding enum case
    to allow us to distinguish different digest hash functions.
  • Add a PKCS1_OAEP_SHA256 public static let to allow users to use the
    new hash function.
  • Enable SHA-256 RSA encryption tests

Result

  • Support for RSA OAEP-sha256

NOTE: This change sticks with the BoringSSL default behaviour and the case which uses the new SHA-256 digest hash function also uses SHA-256 as its mask generation function.

@rnro rnro force-pushed the add_support_for_PKCS1_OAEP_SHA256 branch 4 times, most recently from fb3b750 to 015421c Compare October 24, 2023 20:04
@rnro rnro marked this pull request as ready for review October 25, 2023 07:11
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

I've left some notes, please also update RSA_security.swift.

case sha384
case sha512
case sha512_256
case blake2b256
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is excessive, let's just have only the digests we actually support.

@@ -36,8 +36,13 @@ final class TestRSAEncryption: XCTestCase {

let derPubKey = derPrivKey.publicKey

guard group.sha == "SHA-1", group.mgfSha == "SHA-1" else {
// We currently only support SHA-1 OAEP, which is very legacy but oh well.
let padding: _RSA.Encryption.Padding
Copy link
Collaborator

Choose a reason for hiding this comment

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

Motivation

Add RSA OAEP-sha256 support in addition to the increasingly aged SHA-1
implementation.

Modifications

* Add a `Digest` enum to `_RSA.Encryption.Padding`
* Add a digest associated type to the `.pkcs1_oaep` padding enum case
  to allow us to distinguish different digest hash functions.
* Add a `PKCS1_OAEP_SHA256` public static let to allow users to use the
  new hash function.
* Enable SHA-256 RSA encryption tests

Result

* Support for RSA OAEP-sha256
@rnro rnro force-pushed the add_support_for_PKCS1_OAEP_SHA256 branch from 3ed779c to 0974a60 Compare October 30, 2023 13:47
@rnro rnro requested a review from Lukasa October 30, 2023 13:47
@Lukasa Lukasa added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Oct 31, 2023
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Sorry, hang-on: we need a Security.framework equivalent.

@rnro rnro force-pushed the add_support_for_PKCS1_OAEP_SHA256 branch from 4f7fd18 to 3966fa4 Compare November 1, 2023 10:39
@rnro rnro requested a review from Lukasa November 1, 2023 11:27
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice one @rnro, thanks!

@Lukasa Lukasa merged commit f4b21db into apple:main Nov 6, 2023
8 checks passed
@rnro rnro deleted the add_support_for_PKCS1_OAEP_SHA256 branch November 6, 2023 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants