Skip to content

Signed packages handling in RegistryClient #6184

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.

Sign up for GitHub

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

Closed
wants to merge 20 commits into from
Closed

Signed packages handling in RegistryClient #6184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
cf62890
Update registry config to match proposal
yim-lee Feb 24, 2023
81ee879
Setup CMake build for PackageSigning. Add dependency to PackageRegistry.
yim-lee Feb 24, 2023
96c1f49
API tweaks
yim-lee Feb 24, 2023
b23260b
Add signature model to package version metadata
yim-lee Feb 24, 2023
48d089f
update cmake build
yim-lee Feb 25, 2023
4551303
Wire up PackageSigningEntityStorage
yim-lee Feb 25, 2023
f90ede4
Rename tofu.check to tofu.validate
yim-lee Feb 27, 2023
fa06303
Use registry identity in configuration
yim-lee Feb 28, 2023
c41b899
Implement signature validation and signing entity TOFU flow. Need tests
yim-lee Feb 28, 2023
e1ae8e3
SigningEntity TOFU requires type
yim-lee Feb 28, 2023
2e37845
Add PackageSigningEntityTOFUTests
yim-lee Mar 1, 2023
6619576
Work around 'Task' not found error in CI
yim-lee Mar 1, 2023
992971c
Add SignatureValidationTests
yim-lee Mar 1, 2023
593f79b
Fix CI
yim-lee Mar 1, 2023
8426a0a
Add fatalError for TODOs so we don't forget
yim-lee Mar 1, 2023
d6248cf
Lowercase diagnostics
yim-lee Mar 1, 2023
6eca9db
Fix tests
yim-lee Mar 2, 2023
d67f64e
Add swift-crypto back to bootstrap
yim-lee Mar 2, 2023
3e76c40
Fix Windows build
yim-lee Mar 2, 2023
8009de9
Make 'import Crypto' implementation only
yim-lee Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This source file is part of the Swift open source project
#
# Copyright (c) 2014 - 2021 Apple Inc. and the Swift project authors
# Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors
# Licensed under Apache License v2.0 with Runtime Library Exception
#
# See http://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -54,6 +54,12 @@ if(FIND_PM_DEPS)
find_package(ArgumentParser CONFIG REQUIRED)
find_package(SwiftDriver CONFIG REQUIRED)
find_package(SwiftCollections CONFIG REQUIRED)
find_package(SwiftCrypto CONFIG)
if (SwiftCrypto_FOUND)
add_compile_options($<$<COMPILE_LANGUAGE:Swift>:-DCRYPTO_v2>)
else()
message(WARNING "SwiftCrypto not found, package collections and package signing will be unavailable")
endif()
endif()

find_package(dispatch QUIET)
Expand Down
11 changes: 7 additions & 4 deletions Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
//
// This source file is part of the Swift open source project
//
// Copyright (c) 2014-2022 Apple Inc. and the Swift project authors
// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -204,7 +204,8 @@ let package = Package(
"Basics",
"PackageFingerprint",
"PackageLoading",
"PackageModel"
"PackageModel",
"PackageSigning",
],
exclude: ["CMakeLists.txt"]
),
Expand Down Expand Up @@ -296,7 +297,8 @@ let package = Package(
.product(name: "Crypto", package: "swift-crypto"),
"Basics",
"PackageModel",
]
],
exclude: ["CMakeLists.txt"]
),

// MARK: Package Manager Functionality
Expand Down Expand Up @@ -348,6 +350,7 @@ let package = Package(
"PackageGraph",
"PackageModel",
"PackageRegistry",
"PackageSigning",
"SourceControl",
"SPMBuildCore",
],
Expand All @@ -373,7 +376,6 @@ let package = Package(
.product(name: "ArgumentParser", package: "swift-argument-parser"),
"Basics",
"Build",
"PackageFingerprint",
"PackageLoading",
"PackageModel",
"PackageGraph",
Expand Down Expand Up @@ -540,6 +542,7 @@ let package = Package(
"PackageGraph",
"PackageLoading",
"PackageRegistry",
"PackageSigning",
"SourceControl",
.product(name: "TSCTestSupport", package: "swift-tools-support-core"),
"Workspace",
Expand Down
1 change: 1 addition & 0 deletions Sources/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ add_subdirectory(PackageLoading)
add_subdirectory(PackageModel)
add_subdirectory(PackagePlugin)
add_subdirectory(PackageRegistry)
add_subdirectory(PackageSigning)
add_subdirectory(SPMBuildCore)
add_subdirectory(SPMLLBuild)
add_subdirectory(SourceControl)
Expand Down
13 changes: 8 additions & 5 deletions Sources/CoreCommands/Options.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// This source file is part of the Swift open source project
//
// Copyright (c) 2014-2022 Apple Inc. and the Swift project authors
// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
Expand All @@ -14,8 +14,6 @@ import ArgumentParser

import struct Foundation.URL

import enum PackageFingerprint.FingerprintCheckingMode

import enum PackageModel.BuildConfiguration
import struct PackageModel.BuildFlags
import struct PackageModel.EnabledSanitizers
Expand All @@ -31,6 +29,8 @@ import struct TSCBasic.StringError
import struct TSCUtility.Triple
import struct TSCUtility.Version

import struct Workspace.WorkspaceConfiguration

public struct GlobalOptions: ParsableArguments {
public init() {}

Expand Down Expand Up @@ -217,7 +217,10 @@ public struct SecurityOptions: ParsableArguments {
#endif

@Option(name: .customLong("resolver-fingerprint-checking"))
public var fingerprintCheckingMode: FingerprintCheckingMode = .strict
public var fingerprintCheckingMode: WorkspaceConfiguration.CheckingMode = .strict

@Option(name: .customLong("resolver-signing-entity-checking"))
public var signingEntityCheckingMode: WorkspaceConfiguration.CheckingMode = .warn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

public struct ResolverOptions: ParsableArguments {
Expand Down Expand Up @@ -473,7 +476,7 @@ extension AbsolutePath: ExpressibleByArgument {
}
}

extension FingerprintCheckingMode: ExpressibleByArgument {
extension WorkspaceConfiguration.CheckingMode: ExpressibleByArgument {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public init?(argument: String) {
self.init(rawValue: argument)
}
Expand Down
3 changes: 2 additions & 1 deletion Sources/CoreCommands/SwiftTool.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// This source file is part of the Swift open source project
//
// Copyright (c) 2014-2022 Apple Inc. and the Swift project authors
// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -463,6 +463,7 @@ public final class SwiftTool {
additionalFileRules: isXcodeBuildSystemEnabled ? FileRuleDescription.xcbuildFileTypes : FileRuleDescription.swiftpmFileTypes,
sharedDependenciesCacheEnabled: self.options.caching.useDependenciesCache,
fingerprintCheckingMode: self.options.security.fingerprintCheckingMode,
signingEntityCheckingMode: self.options.security.signingEntityCheckingMode,
sourceControlToRegistryDependencyTransformation: self.options.resolver.sourceControlToRegistryDependencyTransformation.workspaceConfiguration,
restrictImports: .none
),
Expand Down
4 changes: 2 additions & 2 deletions Sources/PackageCollectionsSigning/Key/Key+EC.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// This source file is part of the Swift open source project
//
// Copyright (c) 2021 Apple Inc. and the Swift project authors
// Copyright (c) 2021-2023 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
Expand All @@ -12,7 +12,7 @@

import Foundation

import Crypto
@_implementationOnly import Crypto

typealias CryptoECPrivateKey = P256.Signing.PrivateKey
typealias CryptoECPublicKey = P256.Signing.PublicKey
Expand Down
4 changes: 2 additions & 2 deletions Sources/PackageCollectionsSigning/Signing/Signing+ECKey.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// This source file is part of the Swift open source project
//
// Copyright (c) 2021 Apple Inc. and the Swift project authors
// Copyright (c) 2021-2023 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
Expand All @@ -12,7 +12,7 @@

import struct Foundation.Data

import Crypto
@_implementationOnly import Crypto

// MARK: - MessageSigner and MessageValidator conformance

Expand Down
2 changes: 1 addition & 1 deletion Sources/PackageModel/PackageIdentity.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public struct PackageIdentity: CustomStringConvertible, Sendable {
self.registry != nil
}

public struct RegistryIdentity: CustomStringConvertible {
public struct RegistryIdentity: Hashable, CustomStringConvertible {
public let scope: PackageIdentity.Scope
public let name: PackageIdentity.Name
public let underlying: PackageIdentity
Expand Down
5 changes: 4 additions & 1 deletion Sources/PackageRegistry/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,19 @@
# See http://swift.org/CONTRIBUTORS.txt for Swift project authors

add_library(PackageRegistry STATIC
ChecksumTOFU.swift
Registry.swift
RegistryConfiguration.swift
RegistryClient.swift
RegistryDownloadsManager.swift
ChecksumTOFU.swift)
SignatureValidation.swift
SigningEntityTOFU.swift)
target_link_libraries(PackageRegistry PUBLIC
Basics
PackageFingerprint
PackageLoading
PackageModel
PackageSigning
TSCBasic
TSCUtility)
# NOTE(compnerd) workaround for CMake not setting up include flags yet
Expand Down
6 changes: 2 additions & 4 deletions Sources/PackageRegistry/ChecksumTOFU.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ struct PackageVersionChecksumTOFU {
self.registryClient = registryClient
}

func check(
func validate(
registry: Registry,
package: PackageIdentity.RegistryIdentity,
version: Version,
Expand Down Expand Up @@ -104,9 +104,7 @@ struct PackageVersionChecksumTOFU {
) { result in
switch result {
case .success(let metadata):
guard let sourceArchive = metadata.resources
.first(where: { $0.name == "source-archive" })
else {
guard let sourceArchive = metadata.sourceArchive else {
return completion(.failure(RegistryError.missingSourceArchive))
}

Expand Down
Loading