[Collections] Signing (part 1): certificate and key types #3238

yim-lee · 2021-02-05T20:59:20Z

This is part 1 of a series of PRs to support package collection signing.

Modifications:

New PackageCollectionsSigning module
Add Certificate type
Add EC and RSA key types

Sources/PackageCollectionsSigning/CMakeLists.txt

Sources/PackageCollectionsSigning/Key/Key+RSA.swift

Sources/PackageCollectionsSigning/Certificate/Certificate.swift

Sources/PackageCollectionsSigning/CMakeLists.txt

tomerd · 2021-02-06T04:17:06Z

Sources/PackageCollectionsSigning/Certificate/Certificate.swift

+    case keyExtractionFailure
+    case indeterminateKeyType
+    case unsupportedKeyType
+}


This is part 2 of a series of PRs to support package collection signing. [Part 1](swiftlang#3238) Modifications: - Add support for signing with EC and RSA keys - Add `Signature`, which is similar to JWS - Introduce `CertificatePolicy` protocol (more on this in part 3) - `PackageCollectionsSigning` - given a package collection, certificate and its private key, generate a "signed" collection; and reverse that process to validate collection signature.

yim-lee · 2021-02-06T07:28:07Z

@Lukasa @FredericJacobs - would appreciate your review on this (and upcoming PRs) 🙇‍♀️

tomerd · 2021-02-06T22:01:39Z

seems good to me on the surface - would be good to wait for @Lukasa and/or @FredericJacobs feedback

Sources/PackageCollectionsSigning/Certificate/Certificate.swift

Sources/PackageCollectionsSigning/Key/BoringSSLKey.swift

Lukasa · 2021-02-08T08:51:47Z

Sources/PackageCollectionsSigning/Key/BoringSSLKey.swift

+                              _ closure: (UnsafeMutablePointer<BIO>) -> (T?)) throws -> T where Data: DataProtocol {
+        let bytes = data.copyBytes()
+
+        let bio = CCryptoBoringSSL_BIO_new(CCryptoBoringSSL_BIO_s_mem())


You can elide a copy in this code path by getting the backing pointer to bytes and using BIO_new_mem_buf. This doesn't take ownership of the pointer and won't free it, thus saving the call to BIO_write. The downside, however, is that the BIO is read-only. Not sure how much of a problem that'll be here.

The code was using BIO_new_mem_buf originally:

let bytes = data.copyBytes() let bio = CCryptoBoringSSL_BIO_new_mem_buf(bytes, numericCast(bytes.count)) defer { CCryptoBoringSSL_BIO_free(bio) } guard let bioPointer = bio, let result = closure(bioPointer) else { throw BoringSSLKeyError.bioConversionFailure } return result

but one of Linux CI builds failed consistently with bioConversionFailure. I couldn't tell if it was because bio was nil (unlikely I would think?) or there were issues loading the key (maybe it was the read-only issue that you noted). I wasn't able to reproduce the problem locally. Out of desperation I changed the code to this because it solved some other weird issue I had before when I was implementing OCSP support, and the error went away.

I will add a code comment on this.

Lukasa · 2021-02-08T08:52:33Z

Sources/PackageCollectionsSigning/Key/Key+EC.swift

+
+    init<Data>(pem data: Data) throws where Data: DataProtocol {
+        let pem = String(decoding: data, as: UTF8.self)
+        // TODO: init(pemRepresentation:) is available on macOS 11.0+


Does this support any macOS version older than that? Why?

We've only recently bumped SPM's minimum required macOS version from 10.10 to 10.15 when we added the swift-crypto dependency. I could add code to check for 11.0 availability and use init(pemRepresentation:) then fallback to the current logic on older versions.

Sources/PackageCollectionsSigning/Key/Key+RSA.swift

Sources/PackageCollectionsSigning/Key/Key.swift

Sources/PackageCollectionsSigning/Utilities/Utilities.swift

yim-lee · 2021-02-09T02:01:52Z

cc @catmurdoch

yim-lee · 2021-02-09T02:58:12Z

@swift-ci please smoke test

FredericJacobs · 2021-02-09T11:46:17Z

Sources/PackageCollectionsSigning/Key/Key+EC.swift

+            // From the output of `openssl pkey -in eckey.pem -text -noout`, the P256 private key is 32-byte long.
+            // PEM format is 7-byte pre_string || 32-byte private key || 14-byte mid_string || 65-byte public key
+            // See: https://stackoverflow.com/questions/48101258/how-to-convert-an-ecdsa-key-to-pem-format
+            self.underlying = try CryptoECPrivateKey(rawRepresentation: data.dropFirst(7).prefix(32))


This seems unsafe, it's missing a lot of validation about what kind of key this all is.

Do we have to support macOS Catalina?

Do we have to support macOS Catalina?

Yes we do

I rewrote this code. See #3238 (comment)

FredericJacobs · 2021-02-09T11:46:49Z

Sources/PackageCollectionsSigning/Key/Key+EC.swift

+        } else {
+            let data = try KeyUtilities.stripHeaderAndFooter(pem: pem)
+            // The P256 public key is 65-byte long and there's PEM prefix
+            self.underlying = try CryptoECPublicKey(x963Representation: data.suffix(65))


Same applies here. We should do some validation on the bytes we strip

I rewrote this code. See #3238 (comment)

FredericJacobs · 2021-02-09T11:50:04Z

Sources/PackageCollectionsSigning/Key/Key.swift

+}
+
+enum KeyType {
+    case RSA


Sent you an email to discuss further. From a security standpoint, we would prefer not supporting RSA for this use case as there doesn't seem to be a backwards compatibility requirement.

Per my email reply, we need to support RSA.

I added key size check to #3242 (ecaec8a) since I can't find a good spot to do that in this PR.

Sources/PackageCollectionsSigning/Certificate/Certificate.swift

This is part 1 of a series of PRs to support package collection signing. Modifications: - New `PackageCollectionsSigning` module - Add `Certificate` type - Add EC and RSA key types

This is part 1 of a series of PRs to support package collection signing on **Apple** platforms. Originally swiftlang#3238. Modifications: - New `PackageCollectionsSigning` module - Add `Certificate` type - Add EC and RSA key types

This is part 1 of a series of PRs to support package collection signing on **non-Apple** platforms. Originally swiftlang#3238. Depends on swiftlang#3259

yim-lee · 2021-02-12T06:48:50Z

We are going to split implementation into two paths: Apple platforms and non-Apple platforms. We will review PRs for supporting collection signing on Apple platforms first, then fine tune PRs for non-Apple platforms since they contain unsafe code.

Closing this PR, which now becomes #3259 and #3260

This is part 1 of a series of PRs to support package collection signing on **Apple** platforms. Originally swiftlang#3238. Modifications: - New `PackageCollectionsSigning` module - Add `Certificate` type - Add EC and RSA key types

This is part 1 of a series of PRs to support package collection signing on **non-Apple** platforms. Originally swiftlang#3238. Depends on swiftlang#3259

This is part 1 of a series of PRs to support package collection signing on **Apple** platforms. Originally swiftlang#3238. Modifications: - New `PackageCollectionsSigning` module - Add `Certificate` type - Add EC and RSA key types

This is part 1 of a series of PRs to support package collection signing on **Apple** platforms. Originally #3238. Modifications: - New `PackageCollectionsSigning` module - Add `Certificate` type - Add EC and RSA key types

This is part 1 of a series of PRs to support package collection signing on **all** platforms. Originally swiftlang#3238. Depends on swiftlang#3259

* [Collections] Signing (all, 1): certificate and key types This is part 1 of a series of PRs to support package collection signing on **all** platforms. Originally #3238. Depends on #3259 * [Collections] Signing (all, 2): signed collections This is part 2 of a series of PRs to support package collection signing on **all** platforms. Originally #3242. Depends on #3260, #3264 * [Collections] Signing (all, 3): certificate validations This is part 3 of a series of PRs to support package collection signing on **all** platforms. Originally #3245. Depends on #3265, #3269 * [Collections] Signing (all, 3.b): OCSP support for non-Apple platforms This is part 3.b of a series of PRs to support package collection signing on **all** platforms. Depends on #3270 Modifications: - Add `PackageCollectionsSigningLibc` module - Add OCSP support for non-Apple platforms through `PackageCollectionsSigningLibc` * Use shared callbackQueue and diagnosticsEngine in tests * Update platform check * Better OCSP error handling and cache OCSP results * Don't default to BoringSSL * OpenSSL license * fixup * Use ptr.count * Don't blindly drop X509_V_ERR_UNHANDLED_CRITICAL_EXTENSION error * Add withUnsafeMutablePointer for accessing underlying X509 pointer * Fix leaks * Use withExtendedLifetime * OSCP response handling * Delete remove from cache call * OCSP should fail open * Verify OCSP response * Create new X509_STORE per invocation

yim-lee requested review from abertelrud, friedbunny, neonichu and tomerd as code owners February 5, 2021 20:59

yim-lee commented Feb 5, 2021

View reviewed changes

Sources/PackageCollectionsSigning/CMakeLists.txt Show resolved Hide resolved

compnerd reviewed Feb 6, 2021

View reviewed changes

Sources/PackageCollectionsSigning/Key/Key+RSA.swift Outdated Show resolved Hide resolved

Sources/PackageCollectionsSigning/Certificate/Certificate.swift Outdated Show resolved Hide resolved

yim-lee mentioned this pull request Feb 6, 2021

build: distribute CCryptoBoringSSL apple/swift-crypto#71

Merged

5 tasks

yim-lee force-pushed the signing-cert branch from 4a13fd3 to 07c8a43 Compare February 6, 2021 03:01

tomerd reviewed Feb 6, 2021

View reviewed changes

Sources/PackageCollectionsSigning/CMakeLists.txt Show resolved Hide resolved

tomerd reviewed Feb 6, 2021

View reviewed changes

yim-lee force-pushed the signing-cert branch from 26876f5 to 2371ca0 Compare February 6, 2021 05:37

LucianoPAlmeida mentioned this pull request Feb 6, 2021

[SR-12033] [Sema] Do not allow inferring defaultable closure () -> $T for autoclosure arguments result swiftlang/swift#35503

Merged

tomerd assigned yim-lee Feb 6, 2021

yim-lee force-pushed the signing-cert branch 3 times, most recently from 0266358 to ce1432b Compare February 8, 2021 04:25

Lukasa reviewed Feb 8, 2021

View reviewed changes

yim-lee force-pushed the signing-cert branch from 532b967 to 6a0489b Compare February 9, 2021 02:57

FredericJacobs reviewed Feb 9, 2021

View reviewed changes

Lukasa reviewed Feb 9, 2021

View reviewed changes

Sources/PackageCollectionsSigning/Certificate/Certificate.swift Outdated Show resolved Hide resolved

yim-lee added 4 commits February 9, 2021 15:25

[Collections] Signing (part 1): certificate and key types

a1e5f8c

This is part 1 of a series of PRs to support package collection signing. Modifications: - New `PackageCollectionsSigning` module - Add `Certificate` type - Add EC and RSA key types

Fix Windows compilation errors

44bd1f7

Make deps private

915ebc9

minor cleanup

9cbeccf

yim-lee mentioned this pull request Feb 12, 2021

[Collections] Signing (apple, 1): certificate and key types #3259

Merged

yim-lee mentioned this pull request Feb 12, 2021

[DNM][Collections] Signing (all, 1): certificate and key types #3260

Closed

yim-lee closed this Feb 12, 2021

[Collections] Signing (part 1): certificate and key types #3238

[Collections] Signing (part 1): certificate and key types #3238

Uh oh!

Conversation

yim-lee commented Feb 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yim-lee commented Feb 6, 2021

Uh oh!

tomerd commented Feb 6, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yim-lee Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yim-lee commented Feb 9, 2021

Uh oh!

yim-lee commented Feb 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yim-lee commented Feb 12, 2021

Uh oh!

Uh oh!

yim-lee Feb 8, 2021 •

edited

Loading