Allow public init from P384 PublicKey

[frederikbosch]

This allows to initialize a public key directly from a P384 Public Key.

In my case, I have a PEM file as public key, which I read from a file and then I need to initialize Version3.AsymmetricPublicKey.

let publicKeyPem = try String(contentsOf: Bundle.main.resourceURL!.appendingPathComponent("ios.paseto.pub"), encoding: String.Encoding.utf8)
let publicKey = try P384.Signing.PublicKey.init(pemRepresentation: publicKeyPem)
let key = try Version3.AsymmetricPublicKey.init(key: publicKey)
let token = try self.pasetoParser.verify(encodedToken, with: key)

[aidantwoods]

Thanks for the PR! The reason I didn't export this method publicly was that I couldn't find (and am still having trouble finding) documentation about whether or not Apple's CryptoKit does point validation when constructing the PublicKey type.

Right now the library only accepts public keys which are represented as compressed points (which is the approach recommended by the paseto standard), which ensures that the public key imported ends up on the curve. Unless we can find docs stating that the points are being validated, I'll probably have to implement point validation for this constructor 🙂

[frederikbosch]

I cannot find any documentation that answers your question. Honestly, I lack the knowledge to know where to look for. But I did have a look at Apple's Swift Crypto source code. Apparently, when constructing a P384 public key using pemRepresentation

• it first extracts the derBytes from the PEM
• then it parses ASN1.SubjectPublicKeyInfo and uses the key to initialize the PublicKey using the x963Representation.
• and then the property impl is initialized with using the typealias NISTCurvePublicKeyImpl
• finally, the implementation of NISTCurvePublicKeyImpl is either a typealias of CoreCryptoNISTCurvePublicKeyImpl or a typealias of OpenSSLNISTCurvePublicKeyImpl

So it's not documentation, but code. Maybe you can tell if this includes point validation. Googling NISTCurvePublicKeyImpl and OpenSSLNISTCurvePublicKeyImpl did not give me more information. Maybe we can open an issue at the referenced repository? It is actively maintained. I would love to do this. But I honestly think you are way better equipped than I am.

Another option could be to create an initializer that warns the user: initWithManuallyVerifiedKey()

[aidantwoods]

Googling NISTCurvePublicKeyImpl and OpenSSLNISTCurvePublicKeyImpl did not give me more information.

This is a pretty familiar story trying to dig into Apple's CryptoKit unfortunately 😅

What I may do here despite a little bit of inefficiency, for the publicly exposed initialiser: I'll export the public key into compressed form and then re-import it. It's not pretty, but it'll avoid needing to do explicit point checks and funnel things through a common (safe) initialiser where invalid points are a non-issue.

[frederikbosch]

I think this is a great solution. The inefficiency is not a problem for me. But I decided to ask our question at swift-crypto anyhow.

[frederikbosch]

@aidantwoods Both swift-crypto and CryptoKit do point validation. That means we can make the init method public as suggested in the original commit to this PR.

[aidantwoods]

Both apple/swift-crypto#146 and CryptoKit do point validation.

This test being possible would seem to indicate it is possible to construct invalid public keys (the invalid point I used is only caught when using the new PASETO constructor, but CryptoKit allows the key to be constructed).

[frederikbosch]

I'll report at the Developer Forum.

[remko]

FYI, in the test I ran, constructing the identity point was allowed on CryptoKit (it wasn't in Swift Crypto), but operations using it failed, so it could be that invalid public keys can be constructed in CryptoKit, but that the validity check is delayed in the operations using them.

[frederikbosch]

@aidantwoods Maybe we can try if we can replicate this in your test?

[aidantwoods]

Replicating for signing is a little more difficult, because I'd need to construct a malicious signature that I'd expect to be validated by a carefully chosen invalid key 😅 It also makes me nervous to make decisions based on observed behaviour (even if validation is being done at point of use, who knows what OS versions that might apply to).

That aside, the API I want to strive for in this library is to detect attacks as soon as possible (and not pass around a bad key as if it were valid, hopefully refusing to use it at the last minute).

I'd only feel comfortable accepting these key objects directly with the validation step in place (already now implemented in this PR), so that bad keys are caught on import.

[frederikbosch]

I see @remko published your findings at the Developer Forum. Let's see what they come up with.

[frederikbosch]

@aidantwoods Shall we merge this in? It could take a while before we get our answer. I think your work is a perfect solution that can be changed in the future if needed.

[aidantwoods]

It seems there was a BC break in the initialiser P384.Signing.PublicKey(x963Representation:) in that it no longer accepts compressed keys (it used to!). I wanted to conditionally use the new P384.Signing.PublicKey(compressedRepresentation:) on newer OS versions where the BC break is present, but I think I might just have to drop support for old versions instead because I seemingly can't compile the code for both version simultaneously due to if #available being a runtime check.

I could probably bug report the BC break and retrospectively switch back to support older versions again at some point, but given that this seems to be broken on latest OS, that's really the priority to get working again.

[aidantwoods]

7df0c5f adds a library specific implementation for handling compressed points. It's not possible to use Apple's constructors anymore because of the BC break on x963Representation, and not being able to compile on lower versions if using the new compressedRepresentation constructor due to the lack of OS version compile time #if branches in Swift.

This is all pretty unfortunate, and hopefully temporary. I'll likely switch back to x963Representation if it gets fixed, or bump the min version for access to V3 public PASETO if/when GitHub adds pipeline support for macOS 13 and use the new compressedRepresentation stuff.

[frederikbosch]

@aidantwoods Wonderful, thanks for all your work here!

[aidantwoods]

Fixed a couple of bugs with another pass of the spec, and now confirming that my implementation matches the behaviour of the new compressedRepresentation constructor for some random inputs (tests only run on macOS 13 where that is defined).

I'm going to make another slow pass through the spec tomorrow to make sure I don't spot any nits, but then should be good to merge.

[aidantwoods]

Thanks for the MR @frederikbosch! 😄