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 encrypt and decrypt and adapt api #64

Merged
merged 9 commits into from May 31, 2018

Conversation

3 participants
@FranzBusch
Copy link
Contributor

FranzBusch commented May 26, 2018

This PR adds encrypt and decrypt capabilities to the RSA class. Furthermore, the old .init with a digestAlgorithm didn't make sense anymore; therefore, I deprecated the old apis and introduced new methods which take a DigestAlgorithm as a parameter.

FranzBusch added some commits May 26, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

Thanks for the contribution! A couple of things:

  • We should avoid deprecating methods if we can. I think we could add the encrypt/decrypt methods as static functions and avoid deprecating existing API. If that doesn't work, we can try adding a new type instead of re-using this existing type.
  • The unsafe pointer fixes are great and we should get those in ASAP. If you could submit those as a separate PR that would be wonderful.

FranzBusch added some commits May 29, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

Mostly gardening comments, then LGTM 👍


/// Wrapper to allow for safely working with a potentially-nil Data's byte buffer.
extension Optional where Wrapped == Data {
public func withByteBuffer<T>(_ closure: (BytesBufferPointer?) throws -> T) rethrows -> T {

This comment has been minimized.

@tanner0101

tanner0101 May 29, 2018

Member

We should keep this internal.

@@ -17,6 +17,9 @@ import Foundation
///
/// Read more about RSA on [Wikipedia](https://en.wikipedia.org/wiki/RSA_(cryptosystem)).
public final class RSA {

typealias RSAPkeySymmetricCoder = @convention(c) (Int32, UnsafePointer<UInt8>?, UnsafeMutablePointer<UInt8>?, UnsafeMutablePointer<CNIOOpenSSL.RSA>?, Int32) -> Int32

This comment has been minimized.

@tanner0101

tanner0101 May 29, 2018

Member

Can we make this private? Should also have a comment.

return result == 1
}


/// Dencrypts the supplied input.

This comment has been minimized.

@tanner0101

tanner0101 May 29, 2018

Member

typo here


/// Dencrypts the supplied input.
///
/// let decryptedData = try RSA().sign("vapor", padding: .pkcs1 ,key: .private(pem: ...))

This comment has been minimized.

@tanner0101

tanner0101 May 29, 2018

Member

this example seems to have a typo

return try rsaPkeyCrypt(input, padding: padding, key: key, coder: RSA_public_encrypt)
}

fileprivate static func rsaPkeyCrypt(_ input: LosslessDataConvertible, padding: RSAPadding, key: RSAKey, coder: RSAPkeySymmetricCoder) throws -> Data {

This comment has been minimized.

@tanner0101

tanner0101 May 29, 2018

Member

can this be just private, too?

/// X9.31 padding
case x931

public init?(rawValue: Int32) {

This comment has been minimized.

@tanner0101

tanner0101 May 29, 2018

Member

Missing comment. Also should this be a static func named something like openssl? Just to be clear it's a raw value from OpenSSL

edit: I guess that wouldn't work if you use raw representable properly, so scratch this.

}
}

public var rawValue: Int32 {

This comment has been minimized.

@tanner0101

tanner0101 May 29, 2018

Member

I don't think you need to implement this method here. You should be able to use the = syntax when declaring the enum

This comment has been minimized.

@FranzBusch

FranzBusch May 30, 2018

Author Contributor

This method is necessary otherwise the underlying rawValues would be from 0-4, but the OpenSSL values are different. Sadly one can't assign the OpenSSL values directly, because they aren't literals. Without this method this would be false:

RSAPadding(rawValue: RSA_PKCS1_PADDING).rawValue == RSA_PKCS1_PADDING

This comment has been minimized.

@tanner0101

tanner0101 May 31, 2018

Member

You should be able to achieve by doing something like this:

public enum RSAPadding: Int32 {
    case pkcs1 = RSA_PKCS1_PADDING
    ...
}

I will take a look at adding this on master once merged, thanks!

This comment has been minimized.

@tanner0101

tanner0101 May 31, 2018

Member

(Nevermind, you were right! Didn't understand the part about literals)

FranzBusch and others added some commits May 30, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

thanks!

@tanner0101 tanner0101 merged commit 9b22e8f into vapor:master May 31, 2018

4 of 5 checks passed

ci/circleci: linux-postgresql Your tests failed on CircleCI
Details
ci/circleci: linux Your tests passed on CircleCI!
Details
ci/circleci: linux-jwt Your tests passed on CircleCI!
Details
ci/circleci: linux-mysql Your tests passed on CircleCI!
Details
ci/circleci: linux-vapor Your tests passed on CircleCI!
Details
@penny-coin

This comment has been minimized.

Copy link

penny-coin commented May 31, 2018

Hey @FranzBusch, you just merged a pull request, have a coin!

You now have 4 coins.

@tanner0101 tanner0101 added this to the 3.2.0 milestone May 31, 2018

@tanner0101 tanner0101 self-assigned this May 31, 2018

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