Signed packages handling in RegistryClient #6184

yim-lee · 2023-02-24T05:37:25Z

No description provided.

tomerd · 2023-02-28T17:53:34Z

Sources/PackageRegistry/RegistryClient.swift

@@ -1606,6 +1667,10 @@ extension RegistryClient {
            public let version: String
            public let resources: [Resource]
            public let metadata: AdditionalMetadata?
+
+            var sourceArchive: Resource? {


Sources/PackageRegistry/SignatureValidation.swift

tomerd

looks great

yim-lee · 2023-03-01T05:39:38Z

Sources/CoreCommands/Options.swift

@@ -473,7 +476,7 @@ extension AbsolutePath: ExpressibleByArgument {
    }
 }

-extension FingerprintCheckingMode: ExpressibleByArgument {
+extension WorkspaceConfiguration.CheckingMode: ExpressibleByArgument {


Define a wrapper enum in WorkspaceConfiguration so we can avoid having PackageFingerprint and PackageSigning dependency in this module.

yim-lee · 2023-03-01T05:42:12Z

Sources/PackageRegistry/RegistryClient.swift


    private let availabilityCache = ThreadSafeKeyValueStore<
        URL,
        (status: Result<AvailabilityStatus, Error>, expires: DispatchTime)
    >()

+    private let metadataCache = ThreadSafeKeyValueStore<


Just an in-memory cache. I wonder if we should use sqlite-backed cache.

I think this is a good start, we can improve later if we see it is a bottle neck

yim-lee · 2023-03-01T05:42:46Z

Sources/PackageRegistry/RegistryClient.swift


    private let configuration: RegistryConfiguration
    private let archiverProvider: (FileSystem) -> Archiver
    private let httpClient: LegacyHTTPClient
    private let authorizationProvider: LegacyHTTPClientConfiguration.AuthorizationProvider?
    private let jsonDecoder: JSONDecoder

-    private var checksumTOFU: PackageVersionChecksumTOFU!
+    private(set) var checksumTOFU: PackageVersionChecksumTOFU!
+    private(set) var signatureValidation: SignatureValidation!


signatureValidation is initialized but not used (yet)

yim-lee · 2023-03-01T05:45:33Z

Sources/PackageRegistry/SigningEntityTOFU.swift

+        // Or is the package going from having a signer to .none?
+        case .none:
+            let versionSigners = signerVersions.versionSigners
+            // If the given version is semantically newer than any signed version,


👀

On one hand I think we are trying to be too smart here, but I also think that it will be pretty common for a package to sign new versions only and leave older versions unsigned.

this seems correct and reasonable to assume

Yah, I think the switch signed => unsigned is significant enough that it should require manual intervention by the user. Otherwise, any attacker can just publish a new unsigned version to circumvent the entire process.

yim-lee · 2023-03-01T05:46:30Z

Sources/PackageRegistry/SignatureValidation.swift

@@ -0,0 +1,257 @@
+//===----------------------------------------------------------------------===//


This file is the main change

yim-lee · 2023-03-01T05:46:38Z

Sources/PackageRegistry/SigningEntityTOFU.swift

@@ -0,0 +1,264 @@
+//===----------------------------------------------------------------------===//


This file is the main change

yim-lee · 2023-03-01T05:49:13Z

Sources/PackageRegistry/SignatureValidation.swift

+        observabilityScope: ObservabilityScope,
+        completion: @escaping (Result<SigningEntity?, Error>) -> Void
+    ) {
+        #if swift(>=5.5.2)


Need this to workaround CI error

yim-lee · 2023-03-01T05:53:09Z

@tomerd @neonichu This is ready for review.

Signature validation logic is mostly in place (there are TODOs because PackageSigning module is not finished yet). I haven't wired up RegistryClient to use it since it's not complete.

Sources/PackageRegistry/SignatureValidation.swift

tomerd · 2023-03-01T06:11:39Z

Sources/PackageRegistry/SignatureValidation.swift

+                case .warn:
+                    observabilityScope.emit(warning: "\(error)")
+                    completion(.success(.none))
+                case .silentAllow:


maybe debug or info logging here?

but then it's not "silent"?

Sources/PackageRegistry/SignatureValidation.swift

tomerd · 2023-03-01T06:24:02Z

Sources/PackageRegistry/SigningEntityTOFU.swift

+            observabilityScope: observabilityScope,
+            callbackQueue: callbackQueue
+        ) { result in
+            switch result {


nit: result.mapError could make this more concise

neonichu · 2023-03-01T16:38:19Z

Sources/CoreCommands/Options.swift

+    public var fingerprintCheckingMode: WorkspaceConfiguration.CheckingMode = .strict
+
+    @Option(name: .customLong("resolver-signing-entity-checking"))
+    public var signingEntityCheckingMode: WorkspaceConfiguration.CheckingMode = .warn


Not entirely sure about the naming here. I think it could be reasonable to think of the signing identity check as a form of "fingerprint" and therefore assuming it applies to both. I'd rather differentiate the options on the type of dependency and deprecate the old flag.

These are two different types of checks though--we do fingerprint check for SCM and registry dependency, while signing entity check happens for registry dependency only--and I think we should keep the two options separate, at least initially, because the fingerprint check has been around for some time and is mature enough, while with signing entity check I am more comfortable setting the default level to warn because it's new and there might be bugs.

Didn't realize there was a fingerprint check for registry dependencies as well, is that a checksum of the archive?

Didn't realize there was a fingerprint check for registry dependencies as well, is that a checksum of the archive?

Yep. We implemented fingerprint for both SCM and registry packages.

Does that mean we are grouping two fundamentally different types of fingerprints under one option and is that a potential issue? Registry ones are truly immutable (assuming you can't publish a different archive for an existing version) while SCM ones shouldn't typically change but are technically mutable.

Let's presume someone is regularly changing their SCM tags after the fact for some reason and decides to change the fingerprint checking policy because of that, as it stands that would affect registry dependencies as well even though it may not be intended?

Does that mean we are grouping two fundamentally different types of fingerprints under one option

Yes, though really, neither of the types should be changing regularly.

and is that a potential issue?

I would argue that if a user thinks it's ok to warn instead of error for the uncommon case of regularly changing git hash, then changing checksum is not worse? Checksums are supposed to be immutable, but it depends on registry implementation and thus not guaranteed.

I think it's worse in the sense that they accepted an uncommon case and that implicitly opted them into also accepting a somewhat unrelated case that is likely an actual problem in most scenarios. It's probably fine, though.

Sources/PackageRegistry/RegistryClient.swift

neonichu · 2023-03-01T17:47:23Z

Sources/PackageRegistry/SigningEntityTOFU.swift

+        // Or is the package going from having a signer to .none?
+        case .none:
+            let versionSigners = signerVersions.versionSigners
+            // If the given version is semantically newer than any signed version,


Yah, I think the switch signed => unsigned is significant enough that it should require manual intervention by the user. Otherwise, any attacker can just publish a new unsigned version to circumvent the entire process.

yim-lee · 2023-03-01T23:04:47Z

Sources/PackageSigning/CMakeLists.txt

+# See http://swift.org/LICENSE.txt for license information
+# See http://swift.org/CONTRIBUTORS.txt for Swift project authors
+
+add_library(PackageSigning STATIC


cc @compnerd PackageSigning is a new module

@compnerd Judging from swiftlang/swift-installer-scripts#76 and #4182 I don't think we need to add PackageSigning to apple/swift-installer-scripts? But we need to add SwiftCrypto back (swiftlang/swift-installer-scripts#172)?

Right, if it is statically linked, it doesn't need to be distributed. SwiftCrypto is being statically linked currently, so that is already present.

neonichu · 2023-03-02T05:14:00Z

Utilities/bootstrap

@@ -351,6 +352,7 @@ def build(args):
        ]
        build_dependency(args, "swift-driver", swift_driver_cmake_flags)
        build_dependency(args, "swift-collections")
+        build_dependency(args, "swift-crypto")


Yeah sorry :(

yim-lee · 2023-03-02T05:16:03Z

Utilities/bootstrap

@@ -184,6 +184,7 @@ def parse_global_args(args):
    args.source_dirs["tsc"]                   = os.path.join(args.project_root, "..", "swift-tools-support-core")
    args.source_dirs["yams"]                  = os.path.join(args.project_root, "..", "yams")
    args.source_dirs["swift-argument-parser"] = os.path.join(args.project_root, "..", "swift-argument-parser")
+    args.source_dirs["swift-crypto"]          = os.path.join(args.project_root, "..", "swift-crypto")


@neonichu I think we have to add swift-crypto back since PackageSigning depends on it and PackageRegistry depends on PackageSigning.

Yep, I think that's true for now. Eventually, PackageRegistry shouldn't build with CMake at all since it isn't needed for bootstrapping.

Windows can't bootstrap until we fix the myriad of issues around linking in SPM. We should be adding it to CMake.

We should be adding it to CMake.

I thought I did with https://github.com/apple/swift-package-manager/pull/6184/files#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20a, or there is something else? The Windows CI is failing with this:

FAILED: lib/SKSwiftPMWorkspace.lib Sources/SKSwiftPMWorkspace/CMakeFiles/SKSwiftPMWorkspace.dir/SwiftPMWorkspace.swift.obj swift/SKSwiftPMWorkspace.swiftmodule cmd.exe /C "cd . && T:\1\bin\swiftc.exe -output-file-map Sources\SKSwiftPMWorkspace\CMakeFiles\SKSwiftPMWorkspace.dir\Release\output-file-map.json -incremental -j 36 -emit-library -static -o lib\SKSwiftPMWorkspace.lib -module-name SKSwiftPMWorkspace -module-link-name SKSwiftPMWorkspace -emit-module -emit-module-path swift\SKSwiftPMWorkspace.swiftmodule -emit-dependencies -O -libc MD -Xcc -D_CRT_SECURE_NO_WARNINGS -Xcc -FT:/4 -autolink-force-load -Xfrontend -validate-tbd-against-ir=none -I swift -I T:\7\swift -I T:\6\swift -I C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swift-system\Sources\CSystem\include -I T:\4\swift -I T:\3\src\swift\swift -I T:\3 -I C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swift-corelibs-libdispatch -I C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swift-corelibs-libdispatch\src -I T:\3\src -I C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swift-corelibs-libdispatch\src\BlocksRuntime -I T:\14\swift -I T:\13\swift -I T:\8\products\llbuildSwift -I C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\llbuild\products\libllbuild\include C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\sourcekit-lsp\Sources\SKSwiftPMWorkspace\SwiftPMWorkspace.swift && cd ." <unknown>:0: error: missing required module 'Crypto'

If we use a non-@_implementationOnly import and the module is transitively part of libSwiftPM, we'll probably need to install it.

Windows can't bootstrap until we fix the myriad of issues around linking in SPM. We should be adding it to CMake.

I don't disagree with this, but we should be careful about what's essential to be build with CMake and what isn't, because otherwise we may end up with ancillary stuff being required for bootstrapping. This goes beyond Windows, btw, e.g. right now we need to build PackagePlugin with CMake because we weren't conscious of this in the past and haven't fixed it, yet.

I don't disagree with this, but we should be careful about what's essential to be build with CMake and what isn't, because otherwise we may end up with ancillary stuff being required for bootstrapping. This goes beyond Windows, btw, e.g. right now we need to build PackagePlugin with CMake because we weren't conscious of this in the past and haven't fixed it, yet.

Except we need the full build to distribute SPM on Windows. Otherwise, we end up without support for SPM on Windows.

Except we need the full build to distribute SPM on Windows. Otherwise, we end up without support for SPM on Windows.

Yah, that's why we are keeping the full build for now and the foreseeable future. That doesn't mean we shouldn't be aware of the fact that the way it is supposed to work in the fullness of time is not having everything build with CMake.

compnerd · 2023-03-02T22:31:38Z

Pulling the failure logs:

[1/1][100%][0.934s] Linking Swift executable bin\sourcekit-lsp.exe
FAILED: bin/sourcekit-lsp.exe Sources/sourcekit-lsp/CMakeFiles/sourcekit-lsp.dir/SourceKitLSP.swift.obj swift/sourcekit-lsp.swiftmodule 
cmd.exe /C "cd . && T:\1\bin\swiftc.exe -output-file-map Sources\sourcekit-lsp\CMakeFiles\sourcekit-lsp.dir\Release\output-file-map.json -incremental -j 36 -emit-executable -o bin\sourcekit-lsp.exe -emit-dependencies  -O -libc MD -parse-as-library -Xcc -D_CRT_SECURE_NO_WARNINGS -Xcc -FT:/4 -autolink-force-load -Xfrontend -validate-tbd-against-ir=none -O -libc MD -I swift -I C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\sourcekit-lsp\Sources\Csourcekitd\include -I T:\9\swift -I T:\15\swift -I T:\7\swift -I T:\6\swift -I C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swift-system\Sources\CSystem\include -I T:\4\swift -I T:\3\src\swift\swift -I T:\3 -I C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swift-corelibs-libdispatch -I C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swift-corelibs-libdispatch\src -I T:\3\src -I C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swift-corelibs-libdispatch\src\BlocksRuntime -I T:\14\swift -I T:\13\swift -I T:\8\products\llbuildSwift -I C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\llbuild\products\libllbuild\include -I T:\16\lib\swift\host C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\sourcekit-lsp\Sources\sourcekit-lsp\SourceKitLSP.swift  -L T:\9\lib  -L T:\17\lib  -L T:\17\lib  -L T:\17\lib  -L T:\4\lib  -L T:\15\Sources\IndexStoreDB  -L T:\15\lib\CIndexStoreDB  -L T:\15\lib\Index  -L T:\15\lib\Database  -L T:\15\lib\Core  -L T:\15\lib\Support  -L T:\15\lib\LLVMSupport  -L T:\17\lib  -L T:\17\lib  -L T:\17\lib  -L T:\17\lib  -L T:\17\lib  -L T:\17\lib  -L T:\17\lib  -L T:\17\lib  -L T:\17\lib  -L T:\14\lib  -L T:\14\lib  -L T:\14\lib  -L T:\14\lib  -L T:\14\lib  -L T:\14\lib  -L T:\14\lib  -L T:\8\lib  -L T:\14\lib  -L T:\14\lib  -L T:\7\lib  -L T:\7\lib  -L T:\4\lib  -L T:\4\lib  -L T:\3\src\swift  -L T:\3\src  -L T:\3\src\BlocksRuntime  -L T:\6\lib  -L T:\13\lib  -L T:\13\lib  -L T:\16\lib\swift\host  -L T:\16\lib\swift\host  -L T:\16\lib\swift\host  -L T:\16\lib\swift\host  -L T:\16\lib\swift\host T:\9\lib\ArgumentParser.lib  lib\LanguageServerProtocolJSONRPC.lib  lib\SKCore.lib  lib\SourceKitLSP.lib  T:\4\lib\FoundationXML.lib  T:\15\Sources\IndexStoreDB\IndexStoreDB.lib  T:\15\lib\CIndexStoreDB\CIndexStoreDB.lib  T:\15\lib\Index\Index.lib  T:\15\lib\Database\Database.lib  T:\15\lib\Core\Core.lib  T:\15\lib\Support\Support.lib  T:\15\lib\LLVMSupport\LLVMSupport.lib  lib\SKSwiftPMWorkspace.lib  lib\SKCore.lib  lib\LanguageServerProtocolJSONRPC.lib  lib\SourceKitD.lib  lib\LSPLogging.lib  lib\SKSupport.lib  lib\Csourcekitd.lib  lib\BuildServerProtocol.lib  lib\LanguageServerProtocol.lib  T:\14\lib\Build.lib  T:\14\lib\LLBuildManifest.lib  T:\14\lib\SPMBuildCore.lib  T:\14\lib\PackageGraph.lib  T:\14\lib\PackageLoading.lib  T:\14\lib\SourceControl.lib  T:\14\lib\SPMLLBuild.lib  T:\8\lib\llbuildSwift.lib  T:\14\lib\PackageModel.lib  T:\14\lib\Basics.lib  T:\7\lib\TSCUtility.lib  T:\7\lib\TSCBasic.lib  T:\4\lib\FoundationNetworking.lib  T:\4\lib\Foundation.lib  T:\3\src\swift\swiftDispatch.lib  T:\3\src\dispatch.lib  T:\3\src\BlocksRuntime\BlocksRuntime.lib  T:\6\lib\SystemPackage.lib  T:\13\lib\DequeModule.lib  T:\13\lib\OrderedCollections.lib  T:\16\lib\swift\host\SwiftBasicFormat.lib  T:\16\lib\swift\host\SwiftParser.lib  T:\16\lib\swift\host\SwiftDiagnostics.lib  T:\16\lib\swift\host\IDEUtils.lib  T:\16\lib\swift\host\SwiftSyntax.lib && cd ."
<unknown>:0: error: missing required module 'Crypto'

The problem is that swift-crypto is being linked into SPM and that is being pulled into SK-LSP. SK-LSP doesn't find the file and as a result fails. Are we linking in Crypto statically or dynamically? If it is statically, we should ideally just mark the imports as @_implementationOnly.

yim-lee · 2023-03-02T23:34:09Z

Windows build passes with @_implementationOnly but Linux fails/crashes.

I created #6217 based on this PR but minus import of swift-crypto to see if we can get that to pass and merge the bulk of the changes.

yim-lee · 2023-03-03T08:17:48Z

Split this into #6217 and #6218 basically.

yim-lee requested review from abertelrud, neonichu and tomerd as code owners February 24, 2023 05:37

yim-lee force-pushed the signed-package branch from e96f649 to b0fe175 Compare February 24, 2023 08:17

yim-lee marked this pull request as draft February 24, 2023 08:19

tomerd assigned yim-lee Feb 24, 2023

yim-lee force-pushed the signed-package branch 3 times, most recently from 09e900b to 594d7d3 Compare February 25, 2023 05:26

yim-lee mentioned this pull request Feb 27, 2023

Refactor registry checksum TOFU logic #6190

Merged

yim-lee force-pushed the signed-package branch 2 times, most recently from efade1c to 0554020 Compare February 28, 2023 08:49

tomerd reviewed Feb 28, 2023

View reviewed changes

Sources/PackageRegistry/SignatureValidation.swift Outdated Show resolved Hide resolved

tomerd reviewed Feb 28, 2023

View reviewed changes

yim-lee force-pushed the signed-package branch 2 times, most recently from 88b9701 to 3687bb1 Compare March 1, 2023 05:34

yim-lee commented Mar 1, 2023

View reviewed changes

yim-lee marked this pull request as ready for review March 1, 2023 05:49

yim-lee changed the title ~~[WIP] Signed packages handling in RegistryClient~~ Signed packages handling in RegistryClient Mar 1, 2023

tomerd reviewed Mar 1, 2023

View reviewed changes

Sources/PackageRegistry/SignatureValidation.swift Show resolved Hide resolved

tomerd reviewed Mar 1, 2023

View reviewed changes

Sources/PackageRegistry/SignatureValidation.swift Outdated Show resolved Hide resolved

tomerd reviewed Mar 1, 2023

View reviewed changes

Sources/PackageRegistry/SignatureValidation.swift Show resolved Hide resolved

tomerd reviewed Mar 1, 2023

View reviewed changes

neonichu approved these changes Mar 1, 2023

View reviewed changes

yim-lee force-pushed the signed-package branch from 95b2a07 to 8629fea Compare March 1, 2023 22:28

yim-lee commented Mar 1, 2023

View reviewed changes

yim-lee force-pushed the signed-package branch from b00dc8b to 2e5de32 Compare March 2, 2023 01:51

neonichu reviewed Mar 2, 2023

View reviewed changes

yim-lee commented Mar 2, 2023

View reviewed changes

yim-lee force-pushed the signed-package branch from 93a8538 to de2bfed Compare March 2, 2023 17:55

swiftlang deleted a comment from MaxDesiatov Mar 2, 2023

yim-lee added 20 commits March 2, 2023 14:27

Update registry config to match proposal

cf62890

Setup CMake build for PackageSigning. Add dependency to PackageRegistry.

81ee879

API tweaks

96c1f49

Add signature model to package version metadata

b23260b

update cmake build

48d089f

Wire up PackageSigningEntityStorage

4551303

Rename tofu.check to tofu.validate

f90ede4

Use registry identity in configuration

fa06303

Implement signature validation and signing entity TOFU flow. Need tests

c41b899

SigningEntity TOFU requires type

e1ae8e3

Add PackageSigningEntityTOFUTests

2e37845

Work around 'Task' not found error in CI

6619576

Add SignatureValidationTests

992971c

Fix CI

593f79b

Add fatalError for TODOs so we don't forget

8426a0a

Lowercase diagnostics

d6248cf

Fix tests

6eca9db

Add swift-crypto back to bootstrap

d67f64e

Fix Windows build

3e76c40

Make 'import Crypto' implementation only

8009de9

yim-lee force-pushed the signed-package branch from de2bfed to 8009de9 Compare March 2, 2023 22:30

yim-lee mentioned this pull request Mar 2, 2023

Signed packages handling in RegistryClient (without swift-crypto) #6217

Merged

yim-lee closed this Mar 3, 2023

		@@ -0,0 +1,257 @@
		//===----------------------------------------------------------------------===//

		@@ -0,0 +1,264 @@
		//===----------------------------------------------------------------------===//

Signed packages handling in RegistryClient #6184

Signed packages handling in RegistryClient #6184

Uh oh!

Conversation

yim-lee commented Feb 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomerd left a comment

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!

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!

yim-lee commented Mar 1, 2023

Uh oh!

Uh oh!

Uh oh!

tomerd Mar 1, 2023 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

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!

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!

tomerd Mar 1, 2023 •

edited

Loading

neonichu Mar 2, 2023 •

edited

Loading

yim-lee commented Mar 2, 2023 •

edited

Loading