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 RSA-OAEP via #95 #142

Merged
12 commits merged into from
Feb 19, 2019
Merged

Add RSA-OAEP via #95 #142

12 commits merged into from
Feb 19, 2019

Conversation

ghost
Copy link

@ghost ghost commented Feb 12, 2019

The original #95 branch was pretty far behind master so I figured it was easier to merge the implementation on my machine and redo the tests.

All props to @garrefa for the RSA-OAEP implementation effort and @stulevine for making the RSA tests easily expandable.

garrefa and others added 5 commits February 10, 2019 19:25
* Add RSAES_OAEP type to AsymmetricKeyAlgorithm

* Adds AsymmetricKeyAlgorithm.RSAES_OAEP and implements plain text and cipher length check

* Add encrypter for (RSAES_OAEP, A256CBCHS512)

* Add decrypter for (RSAES_OAEP, A256CBCHS512)

* Rename new AsymmetricKeyAlgorithm case to RSAOAEP

* Fix length checks

* Add Encrypter Tests

* Add Decrypter Tests
…wift into resolve-conflicts-95

# Conflicts:
#	JOSESwift/Sources/Algorithms.swift
#	JOSESwift/Sources/CryptoImplementation/RSA.swift
#	JOSESwift/Sources/Decrypter.swift
#	JOSESwift/Sources/Encrypter.swift
#	Tests/RSADecrypterTests.swift
#	Tests/RSAEncrypterTests.swift
@ghost ghost self-assigned this Feb 12, 2019
@ghost ghost requested review from carol-mohemian and a team February 12, 2019 14:37
@ghost ghost mentioned this pull request Feb 12, 2019
CHANGELOG.md Show resolved Hide resolved
JOSESwift/Sources/Decrypter.swift Outdated Show resolved Hide resolved
Tests/JWERSATests.swift Outdated Show resolved Hide resolved
Tests/JWERSATests.swift Show resolved Hide resolved
Tests/JWERSATests.swift Outdated Show resolved Hide resolved
Tests/JWERSATests.swift Outdated Show resolved Hide resolved
nathan-mohemian and others added 5 commits February 13, 2019 14:44
Co-Authored-By: daniel-mohemian <[email protected]>
Co-Authored-By: daniel-mohemian <[email protected]>
Co-Authored-By: daniel-mohemian <[email protected]>
# Conflicts:
#	CHANGELOG.md
#	README.md
ghost
ghost previously approved these changes Feb 14, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@carol-mohemian carol-mohemian left a comment

Choose a reason for hiding this comment

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

Didn't look too closely on the logic and if the implementation is right. Maybe I find some time in the future. Nevertheless, with the typos fixed, good to go from my side 🙌

@@ -67,15 +69,23 @@ internal extension AsymmetricKeyAlgorithm {
///
/// - RSA1_5: For detailed information about the allowed plain text length for RSAES-PKCS1-v1_5,
/// please refer to [RFC-3447, Section 7.2](https://tools.ietf.org/html/rfc3447#section-7.2).
/// - RSAOAEP256: For detailed information about the allowed plain text length for RSA-OAEP,
/// - RSAOAEP: For detailed information about the allowed plain text length for RSA-OAEP,
Copy link
Contributor

Choose a reason for hiding this comment

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

Before the comma: Should be RSAES-OAEP.

/// - RSAOAEP256: For detailed information about the allowed plain text length for RSA-OAEP,
/// - RSAOAEP: For detailed information about the allowed plain text length for RSA-OAEP,
/// please refer to [RFC-3447, Section 7.1](https://tools.ietf.org/html/rfc3447#section-7.1).
/// - RSAOAEP256: For detailed information about the allowed plain text length for RSA-OAEP-256,
Copy link
Contributor

Choose a reason for hiding this comment

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

Before the comma: Should be RSAES-OAEP-256.

// and https://www.rfc-editor.org/errata_search.php?rfc=3447)
// C: ciphertext to be decrypted, an octet string of length k, where k >= 2hLen + 2
case .RSAOAEP, .RSAOAEP256:
// For detailed information about the allowed cipher length for RSA-OAEP and RSA-OAEP-256,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same same: RSAES-OAEP and ``RSAES-OAEP-256`

// For detailed information about the allowed cipher length for RSA-OAEP and RSA-OAEP-256,
// please refer to RFC-3447 (https://tools.ietf.org/html/rfc3447#section-7.1.2,
// https://www.rfc-editor.org/errata_search.php?rfc=3447):
// The ciphertext to be decrypted is an an octet string of length k,
Copy link
Contributor

Choose a reason for hiding this comment

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

is an an octet -> is an octet

// For detailed information, please refer to this RFC(https://tools.ietf.org/html/rfc3218#section-2.3.2)
// and http://www.ietf.org/mail-archive/web/jose/current/msg01832.html
// Generate a random CEK to substitue in case we fail to decrypt the CEK.
// This is to prevent the to prevent MMA (Million Message Attack) against RSA.
Copy link
Contributor

Choose a reason for hiding this comment

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

This it to prevent the to prevent MMA -> This is to prevent the MMA

@@ -34,17 +57,28 @@ class JWETests: RSACryptoTestCase {
YP_e_rhz0PVg9QnJXiRl030ggI9GGs3E_0pEPBs9_WJ3E60qQVoXTIMbJXSQ.bQc-W1Ph_0_3kX570pT8gjDlGyiK3kF8PlHiT7GWfMo
""".data(using: .utf8)!

let compactSerializedJWERSAOAEPSHA = """
Copy link
Contributor

Choose a reason for hiding this comment

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

..SHA1?

Copy link
Author

@ghost ghost Feb 14, 2019

Choose a reason for hiding this comment

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

Yes!

"""

// printf "The true sign of intelligence is not knowledge but imagination." | openssl pkeyutl -encrypt -pubin -inkey alice.pub.pem -pkeyopt rsa_padding_mode:oaep -pkeyopt rsa_oaep_md:sha1 -pkeyopt rsa_mgf1_md:sha1 -out >(base64)
// *NOTE*: openssl v1.1.x is required to encrypt data using RSA-OAEP with SHA256 digest
Copy link
Contributor

Choose a reason for hiding this comment

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

RSAES-OAEP

mNYaDDsxfYHg3LzWsVyhqpFuZQ6hhklG9lJr6OBBuk/+pcJYdHuYEuLnJhPeKqF/9xgMOU0e0xLMtkQW+IfDMlm0oAVavHrxk7A4T5L9+yjuxNj\
N16k2Rqiw==
"""

// printf "The true sign of intelligence is not knowledge but imagination." | openssl pkeyutl -encrypt -pubin -inkey bob.pub.pem -pkeyopt rsa_padding_mode:oaep -pkeyopt rsa_oaep_md:sha256 -pkeyopt rsa_mgf1_md:sha256 -out >(base64)
// *NOTE*: openssl v1.1.x is required to encrypt data using RSA-OAEP with SHA256 digest
Copy link
Contributor

Choose a reason for hiding this comment

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

RSAES-OAEP

"""

// printf "The true sign of intelligence is not knowledge but imagination." | openssl pkeyutl -encrypt -pubin -inkey bob.pub.pem -pkeyopt rsa_padding_mode:oaep -pkeyopt rsa_oaep_md:sha1 -pkeyopt rsa_mgf1_md:sha1 -out >(base64)
// *NOTE*: openssl v1.1.x is required to encrypt data using RSA-OAEP with SHA256 digest
Copy link
Contributor

Choose a reason for hiding this comment

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

RSAES-OAEP

@ghost ghost dismissed their stale review via 109901f February 14, 2019 12:47
@ghost ghost merged commit b0a28b1 into master Feb 19, 2019
@ghost ghost deleted the resolve-conflicts-95 branch February 19, 2019 10:27
This pull request was closed.
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.

3 participants