Updates to PackageSigningEntityStorage and signing entity TOFU errors (#6268)

- Storage now includes source entity origin (i.e., registry)
- Enhance storage data model to support more sophisticated use cases
- Add more details to `RegistryError.signingEntityForReleaseChanged` and `.signingEntityForPackageChanged`

Author: yim-lee

Sources/PackageRegistry/RegistryClient.swift

@@ -809,7 +809,8 @@ public final class RegistryClient: Cancellable {
                                                     switch checksumResult {
                                                     case .success:
                                                         do {
-                                                            // validate that the destination does not already exist (again, as this
+                                                            // validate that the destination does not already exist
+                                                            // (again, as this
                                                             // is
                                                             // async)
                                                             guard !fileSystem.exists(destinationPath) else {
@@ -1344,12 +1345,20 @@ public enum RegistryError: Error, CustomStringConvertible {
     case signerNotTrusted(SigningEntity)
     case failedToValidateSignature(Error)
     case signingEntityForReleaseChanged(
+        registry: Registry,
         package: PackageIdentity,
         version: Version,
         latest: SigningEntity?,
         previous: SigningEntity
     )
-    case signingEntityForPackageChanged(package: PackageIdentity, latest: SigningEntity?, previous: SigningEntity)
+    case signingEntityForPackageChanged(
+        registry: Registry,
+        package: PackageIdentity,
+        version: Version,
+        latest: SigningEntity?,
+        previous: SigningEntity,
+        previousVersion: Version
+    )
 
     public var description: String {
         switch self {
@@ -1443,10 +1452,17 @@ public enum RegistryError: Error, CustomStringConvertible {
             return "the signer \(signingEntity) is not trusted"
         case .failedToValidateSignature(let error):
             return "failed to validate signature: \(error)"
-        case .signingEntityForReleaseChanged(let package, let version, let latest, let previous):
-            return "the signing entity '\(String(describing: latest))' for \(package) version \(version) is different from the previously recorded value '\(previous)'"
-        case .signingEntityForPackageChanged(let package, let latest, let previous):
-            return "the signing entity '\(String(describing: latest))' for \(package) is different from the previously recorded value '\(previous)'"
+        case .signingEntityForReleaseChanged(let registry, let package, let version, let latest, let previous):
+            return "the signing entity '\(String(describing: latest))' from \(registry) for \(package) version \(version) is different from the previously recorded value '\(previous)'"
+        case .signingEntityForPackageChanged(
+            let registry,
+            let package,
+            let version,
+            let latest,
+            let previous,
+            let previousVersion
+        ):
+            return "the signing entity '\(String(describing: latest))' from \(registry) for \(package) version \(version) is different from the previously recorded value '\(previous)' for version \(previousVersion)"
         }
     }
 }

Sources/PackageRegistry/SignatureValidation.swift

@@ -77,6 +77,7 @@ struct SignatureValidation {
                 // Always do signing entity TOFU check at the end,
                 // whether the package is signed or not.
                 self.signingEntityTOFU.validate(
+                    registry: registry,
                     package: package,
                     version: version,
                     signingEntity: signingEntity,

Sources/PackageRegistry/SigningEntityTOFU.swift

@@ -31,6 +31,7 @@ struct PackageSigningEntityTOFU {
     }
 
     func validate(
+        registry: Registry,
         package: PackageIdentity.RegistryIdentity,
         version: Version,
         signingEntity: SigningEntity?,
@@ -48,12 +49,13 @@ struct PackageSigningEntityTOFU {
             callbackQueue: callbackQueue
         ) { result in
             switch result {
-            case .success(let signerVersions):
+            case .success(let packageSigners):
                 self.validateSigningEntity(
+                    registry: registry,
                     package: package,
                     version: version,
                     signingEntity: signingEntity,
-                    signerVersions: signerVersions,
+                    packageSigners: packageSigners,
                     observabilityScope: observabilityScope
                 ) { validateResult in
                     switch validateResult {
@@ -63,6 +65,7 @@ struct PackageSigningEntityTOFU {
                             return completion(.success(()))
                         }
                         self.writeToStorage(
+                            registry: registry,
                             package: package,
                             version: version,
                             signingEntity: signingEntity,
@@ -82,36 +85,40 @@ struct PackageSigningEntityTOFU {
     }
 
     private func validateSigningEntity(
+        registry: Registry,
         package: PackageIdentity.RegistryIdentity,
         version: Version,
         signingEntity: SigningEntity?,
-        signerVersions: [SigningEntity: Set<Version>],
+        packageSigners: PackageSigners,
         observabilityScope: ObservabilityScope,
         completion: @escaping (Result<Bool, Error>) -> Void
     ) {
         // Package is never signed.
         // If signingEntity is nil, it means package remains unsigned, which is OK. (none -> none)
-        // Otherwise, package has gained a signing entity, which is also OK. (none -> some)
-        if signerVersions.isEmpty {
+        // Otherwise, package has gained a signer, which is also OK. (none -> some)
+        if packageSigners.isEmpty {
             return completion(.success(true))
         }
 
         // If we get to this point, it means we have seen a signed version of the package.
 
-        // TODO: It's possible that some of the signing entity changes are legitimate:
-        // e.g., change of package ownership, package author decides to stop signing releases, etc.
-        // Instead of failing, we should allow and prompt user to add/replace/remove signing entity.
-
-        // We recorded the version's signer previously
-        if let signerForVersion = signerVersions.signingEntity(of: version) {
-            // The given signer is different
-            // TODO: This could indicate a legitimate change in package ownership
-            guard signerForVersion == signingEntity else {
-                return self.handleSigningEntityChanged(
+        let signingEntitiesForVersion = packageSigners.signingEntities(of: version)
+        // We recorded the version's signer(s) previously
+        if !signingEntitiesForVersion.isEmpty {
+            guard let signingEntityToCheck = signingEntity,
+                  signingEntitiesForVersion.contains(signingEntityToCheck)
+            else {
+                // The given signer is nil or different
+                // TODO: This could indicate a legitimate change
+                //   - If signingEntity is nil, it could mean the package author has stopped signing the package.
+                //   - If signingEntity is non-nil, it could mean the package has changed ownership and the new owner
+                //     is re-signing all of the package versions.
+                return self.handleSigningEntityForPackageVersionChanged(
+                    registry: registry,
                     package: package,
                     version: version,
                     latest: signingEntity,
-                    existing: signerForVersion,
+                    existing: signingEntitiesForVersion.first!, // !-safe since signingEntitiesForVersion is non-empty
                     observabilityScope: observabilityScope
                 ) { result in
                     completion(result.tryMap { false })
@@ -121,28 +128,75 @@ struct PackageSigningEntityTOFU {
             return completion(.success(false))
         }
 
+        // Check signer(s) of other version(s)
         switch signingEntity {
         // Is the package changing from one signer to another?
         case .some(let signingEntity):
-            // Check signer(s) of other version(s)
-            if let otherSigner = signerVersions.keys.filter({ $0 != signingEntity }).first {
-                // There is a different signer
-                // TODO: This could indicate a legitimate change in package ownership
-                self.handleSigningEntityChanged(
+            // Does the package have an expected signer?
+            if let expectedSigner = packageSigners.expectedSigner,
+               version >= expectedSigner.fromVersion
+            {
+                // Signer is as expected
+                if signingEntity == expectedSigner.signingEntity {
+                    return completion(.success(true))
+                }
+                // If the signer is different from expected but has been seen before,
+                // we allow versions before its highest known version to be signed
+                // by this signer. This is to handle the case where a signer was recorded
+                // before expectedSigner is set, and it had signed a version newer than
+                // expectedSigner.fromVersion. For example, if signer A is recorded to have
+                // signed v2.0 and later expectedSigner is set to signer B with fromVersion
+                // set to v1.5, then it should not be a TOFU failure if we see signer A
+                // for v1.9.
+                if let knownSigner = packageSigners.signers[signingEntity],
+                   let highestKnownVersion = knownSigner.versions.sorted(by: >).first,
+                   version < highestKnownVersion
+                {
+                    return completion(.success(true))
+                }
+                // Different signer than expected
+                self.handleSigningEntityForPackageChanged(
+                    registry: registry,
                     package: package,
+                    version: version,
                     latest: signingEntity,
-                    existing: otherSigner,
+                    existing: expectedSigner.signingEntity,
+                    existingVersion: expectedSigner.fromVersion,
                     observabilityScope: observabilityScope
                 ) { result in
                     completion(result.tryMap { false })
                 }
             } else {
+                // There might be other signers, but if we have seen this signer before, allow it.
+                if packageSigners.signers[signingEntity] != nil {
+                    return completion(.success(true))
+                }
+
+                let otherSigningEntities = packageSigners.signers.keys.filter { $0 != signingEntity }
+                for otherSigningEntity in otherSigningEntities {
+                    // We have not seen this signer before, and there is at least one other signer already.
+                    // TODO: This could indicate a legitimate change in package ownership
+                    if let existingVersion = packageSigners.signers[otherSigningEntity]?.versions.sorted(by: >).first {
+                        return self.handleSigningEntityForPackageChanged(
+                            registry: registry,
+                            package: package,
+                            version: version,
+                            latest: signingEntity,
+                            existing: otherSigningEntity,
+                            existingVersion: existingVersion,
+                            observabilityScope: observabilityScope
+                        ) { result in
+                            completion(result.tryMap { false })
+                        }
+                    }
+                }
+
                 // Package doesn't have any other signer besides the given one, which is good.
                 completion(.success(true))
             }
         // Or is the package going from having a signer to .none?
         case .none:
-            let versionSigners = signerVersions.versionSigners
+            let versionSigningEntities = packageSigners.versionSigningEntities
             // If the given version is semantically newer than any signed version,
             // then it must be signed. (i.e., when a package starts being signed
             // at a version, then all future versions must be signed.)
@@ -159,14 +213,19 @@ struct PackageSigningEntityTOFU {
             //     a newer version (i.e., < 1.5.0) and we assume it to be signed.
             //   - When unsigned v2.0.0 is downloaded, we don't fail because we haven't
             //     seen a signed 2.x release yet, so we assume 2.x releases are not signed.
-            let olderSignedVersions = versionSigners.keys.filter { $0.major == version.major && $0 < version }
+            //     (this might be controversial)
+            let olderSignedVersions = versionSigningEntities.keys
+                .filter { $0.major == version.major && $0 < version }
                 .sorted(by: >)
-            for signedVersion in olderSignedVersions {
-                if let versionSigner = versionSigners[signedVersion] {
-                    return self.handleSigningEntityChanged(
+            for olderSignedVersion in olderSignedVersions {
+                if let olderVersionSigner = versionSigningEntities[olderSignedVersion]?.first {
+                    return self.handleSigningEntityForPackageChanged(
+                        registry: registry,
                         package: package,
+                        version: version,
                         latest: signingEntity,
-                        existing: versionSigner,
+                        existing: olderVersionSigner,
+                        existingVersion: olderSignedVersion,
                         observabilityScope: observabilityScope
                     ) { result in
                         completion(result.tryMap { false })
@@ -179,6 +238,7 @@ struct PackageSigningEntityTOFU {
     }
 
     private func writeToStorage(
+        registry: Registry,
         package: PackageIdentity.RegistryIdentity,
         version: Version,
         signingEntity: SigningEntity,
@@ -194,14 +254,16 @@ struct PackageSigningEntityTOFU {
             package: package.underlying,
             version: version,
             signingEntity: signingEntity,
+            origin: .registry(registry.url),
             observabilityScope: observabilityScope,
             callbackQueue: callbackQueue
         ) { result in
             switch result {
             case .success:
                 completion(.success(()))
             case .failure(PackageSigningEntityStorageError.conflict(_, _, _, let existing)):
-                self.handleSigningEntityChanged(
+                self.handleSigningEntityForPackageVersionChanged(
+                    registry: registry,
                     package: package,
                     version: version,
                     latest: signingEntity,
@@ -215,7 +277,8 @@ struct PackageSigningEntityTOFU {
         }
     }
 
-    private func handleSigningEntityChanged(
+    private func handleSigningEntityForPackageVersionChanged(
+        registry: Registry,
         package: PackageIdentity.RegistryIdentity,
         version: Version,
         latest: SigningEntity?,
@@ -226,6 +289,7 @@ struct PackageSigningEntityTOFU {
         switch self.signingEntityCheckingMode {
         case .strict:
             completion(.failure(RegistryError.signingEntityForReleaseChanged(
+                registry: registry,
                 package: package.underlying,
                 version: version,
                 latest: latest,
@@ -234,29 +298,36 @@ struct PackageSigningEntityTOFU {
         case .warn:
             observabilityScope
                 .emit(
-                    warning: "The signing entity '\(String(describing: latest))' for \(package) version \(version) does not match previously recorded value '\(existing)'"
+                    warning: "the signing entity '\(String(describing: latest))' from \(registry) for \(package) version \(version) is different from the previously recorded value '\(existing)'"
                 )
             completion(.success(()))
         }
     }
 
-    private func handleSigningEntityChanged(
+    private func handleSigningEntityForPackageChanged(
+        registry: Registry,
         package: PackageIdentity.RegistryIdentity,
+        version: Version,
         latest: SigningEntity?,
         existing: SigningEntity,
+        existingVersion: Version,
         observabilityScope: ObservabilityScope,
         completion: @escaping (Result<Void, Error>) -> Void
     ) {
         switch self.signingEntityCheckingMode {
         case .strict:
             completion(.failure(RegistryError.signingEntityForPackageChanged(
+                registry: registry,
                 package: package.underlying,
-                latest: latest, previous: existing
+                version: version,
+                latest: latest,
+                previous: existing,
+                previousVersion: existingVersion
             )))
         case .warn:
             observabilityScope
                 .emit(
-                    warning: "The signing entity '\(String(describing: latest))' for \(package) does not match previously recorded value '\(existing)'"
+                    warning: "the signing entity '\(String(describing: latest))' from \(registry) for \(package) version \(version) is different from the previously recorded value '\(existing)' for version \(existingVersion)"
                 )
             completion(.success(()))
         }