From 0846bb6640e7b89b5b46ede2e7ec1dc2e60e8029 Mon Sep 17 00:00:00 2001 From: Aditya Ramani Date: Wed, 4 Jun 2025 14:23:29 -0700 Subject: [PATCH] Create RequestScheme type to handle registry interactions (#6) Its a bit weird to have to pass `--http` when pulling / pushing images if we are trying to do so from a local registry This PR adds a `RequestScheme` enum type which tries to detect if the connection to the registry should be over http or https. This auto detection is the default behavior and can be overridden by passing the `--scheme http/https` flag to the `run` ,`pull`, `push` `login` and `registry default` command The auto detection works by looking at the hostname / IP. HTTP is used in the following cases - If the hostname starts with `localhost` or is `127.x.x.x` - if the host looks like an IP from the private address space - If the host ends with the current default dns domain name Also in this PR: - Removing some used CLI flag groups and TODOs - Better error messages when registry credentials are not found for a host Signed-off-by: Aditya Ramani --- Sources/CLI/Builder/BuilderStart.swift | 16 +--- Sources/CLI/Container/ContainerCreate.swift | 3 +- Sources/CLI/Container/ContainerExec.swift | 4 - Sources/CLI/Container/ContainerStart.swift | 4 - Sources/CLI/Image/ImagePull.swift | 13 ++-- Sources/CLI/Image/ImagePush.swift | 8 +- Sources/CLI/Registry/Login.swift | 10 ++- Sources/CLI/Registry/RegistryDefault.swift | 12 ++- Sources/CLI/RunCommand.swift | 7 +- .../ContainerClient/Core/ClientImage.swift | 24 ++++-- Sources/ContainerClient/Flags.swift | 22 ++---- Sources/ContainerClient/RequestScheme.swift | 73 +++++++++++++++++++ Sources/ContainerClient/Utility.swift | 26 +++---- .../Server/ImageService.swift | 14 +++- .../RequestSchemeTests.swift | 63 ++++++++++++++++ 15 files changed, 214 insertions(+), 85 deletions(-) create mode 100644 Sources/ContainerClient/RequestScheme.swift create mode 100644 Tests/ContainerClientTests/RequestSchemeTests.swift diff --git a/Sources/CLI/Builder/BuilderStart.swift b/Sources/CLI/Builder/BuilderStart.swift index e7e95eef..b1c98d3f 100644 --- a/Sources/CLI/Builder/BuilderStart.swift +++ b/Sources/CLI/Builder/BuilderStart.swift @@ -203,21 +203,11 @@ extension Application { let kernel = try await { await progressUpdate([ - .setDescription("Fetching kernel image"), - .setItemsName("blobs"), + .setDescription("Fetching kernel"), + .setItemsName("binary"), ]) - let s: SystemPlatform - let architecture = builderPlatform.architecture - switch architecture { - case "arm64": - s = .linuxArm - case "amd64": - s = .linuxAmd - default: - throw ContainerizationError.init(.unsupported, message: "platform architecture \(architecture)") - } - let kernel = try await ClientKernel.getDefaultKernel(for: s) + let kernel = try await ClientKernel.getDefaultKernel(for: .current) return kernel }() diff --git a/Sources/CLI/Container/ContainerCreate.swift b/Sources/CLI/Container/ContainerCreate.swift index 0eff9846..de224765 100644 --- a/Sources/CLI/Container/ContainerCreate.swift +++ b/Sources/CLI/Container/ContainerCreate.swift @@ -42,7 +42,7 @@ extension Application { var managementFlags: Flags.Management @OptionGroup - var pullFlags: Flags.Pull + var registryFlags: Flags.Registry @OptionGroup var global: Flags.Global @@ -75,6 +75,7 @@ extension Application { process: processFlags, management: managementFlags, resource: resourceFlags, + registry: registryFlags, progressUpdate: progress.handler ) diff --git a/Sources/CLI/Container/ContainerExec.swift b/Sources/CLI/Container/ContainerExec.swift index 5d8a0f19..de396958 100644 --- a/Sources/CLI/Container/ContainerExec.swift +++ b/Sources/CLI/Container/ContainerExec.swift @@ -29,10 +29,6 @@ extension Application { @OptionGroup var processFlags: Flags.Process - // FIXME: Add in detach keys support. - @OptionGroup(visibility: .hidden) - var detachFlags: Flags.Detach - @OptionGroup var global: Flags.Global diff --git a/Sources/CLI/Container/ContainerStart.swift b/Sources/CLI/Container/ContainerStart.swift index d94cc718..a804b9c2 100644 --- a/Sources/CLI/Container/ContainerStart.swift +++ b/Sources/CLI/Container/ContainerStart.swift @@ -32,10 +32,6 @@ extension Application { @Flag(name: .shortAndLong, help: "Attach container's STDIN") var interactive = false - // FIXME: Add in detach keys support. - @OptionGroup(visibility: .hidden) - var detachFlags: Flags.Detach - @OptionGroup var global: Flags.Global diff --git a/Sources/CLI/Image/ImagePull.swift b/Sources/CLI/Image/ImagePull.swift index c8d7fcd4..ade652a8 100644 --- a/Sources/CLI/Image/ImagePull.swift +++ b/Sources/CLI/Image/ImagePull.swift @@ -31,18 +31,19 @@ extension Application { @OptionGroup var global: Flags.Global - @Option(help: "Platform string in the form 'os/arch/variant'. Example 'linux/arm64/v8', 'linux/amd64'") var platform: String? + @OptionGroup + var registry: Flags.Registry - @Flag(help: "Pull using plain-text http") var http: Bool = false + @Option(help: "Platform string in the form 'os/arch/variant'. Example 'linux/arm64/v8', 'linux/amd64'") var platform: String? @Argument var reference: String init() {} - init(platform: String? = nil, http: Bool = false, reference: String) { + init(platform: String? = nil, scheme: String = "auto", reference: String) { self.global = Flags.Global() + self.registry = Flags.Registry(scheme: scheme) self.platform = platform - self.http = http self.reference = reference } @@ -52,6 +53,8 @@ extension Application { p = try Platform(from: platform) } + let scheme = try RequestScheme(registry.scheme) + let processedReference = try ClientImage.normalizeReference(reference) let progressConfig = try ProgressConfig( showTasks: true, @@ -70,7 +73,7 @@ extension Application { let taskManager = ProgressTaskCoordinator() let fetchTask = await taskManager.startTask() let image = try await ClientImage.pull( - reference: processedReference, platform: p, progressUpdate: ProgressTaskCoordinator.handler(for: fetchTask, from: progress.handler) + reference: processedReference, platform: p, scheme: scheme, progressUpdate: ProgressTaskCoordinator.handler(for: fetchTask, from: progress.handler) ) progress.set(description: "Unpacking image") diff --git a/Sources/CLI/Image/ImagePush.swift b/Sources/CLI/Image/ImagePush.swift index 379ae196..cba664c2 100644 --- a/Sources/CLI/Image/ImagePush.swift +++ b/Sources/CLI/Image/ImagePush.swift @@ -30,9 +30,10 @@ extension Application { @OptionGroup var global: Flags.Global - @Option(help: "Platform string in the form 'os/arch/variant'. Example 'linux/arm64/v8', 'linux/amd64'") var platform: String? + @OptionGroup + var registry: Flags.Registry - @Flag(help: "Push using plain-text http") var http: Bool = false + @Option(help: "Platform string in the form 'os/arch/variant'. Example 'linux/arm64/v8', 'linux/amd64'") var platform: String? @Argument var reference: String @@ -42,6 +43,7 @@ extension Application { p = try Platform(from: platform) } + let scheme = try RequestScheme(registry.scheme) let image = try await ClientImage.get(reference: reference) let progressConfig = try ProgressConfig( @@ -56,7 +58,7 @@ extension Application { progress.finish() } progress.start() - _ = try await image.push(platform: p, insecure: http, progressUpdate: progress.handler) + _ = try await image.push(platform: p, scheme: scheme, progressUpdate: progress.handler) progress.finish() } } diff --git a/Sources/CLI/Registry/Login.swift b/Sources/CLI/Registry/Login.swift index a1b721b3..018a131b 100644 --- a/Sources/CLI/Registry/Login.swift +++ b/Sources/CLI/Registry/Login.swift @@ -33,14 +33,15 @@ extension Application { @Flag(help: "Take the password from stdin") var passwordStdin: Bool = false - @Flag(help: "Login using plain-text http") var http: Bool = false - @Argument(help: "Registry server name") var server: String @OptionGroup var global: Flags.Global + @OptionGroup + var registry: Flags.Registry + func run() async throws { var username = self.username var password = "" @@ -63,11 +64,12 @@ extension Application { print() } - let scheme = http ? "http" : "https" let server = Reference.resolveDomain(domain: server) + let scheme = try RequestScheme(registry.scheme).schemeFor(host: server) + let client = RegistryClient( host: server, - scheme: scheme, + scheme: scheme.rawValue, authentication: BasicAuthentication(username: username, password: password), retryOptions: .init( maxRetries: 10, diff --git a/Sources/CLI/Registry/RegistryDefault.swift b/Sources/CLI/Registry/RegistryDefault.swift index 4aea27e0..354c69a1 100644 --- a/Sources/CLI/Registry/RegistryDefault.swift +++ b/Sources/CLI/Registry/RegistryDefault.swift @@ -39,20 +39,24 @@ extension Application { abstract: "Set the default registry" ) - @Flag(help: "Try to connect to the registry using plain-text http") - var http: Bool = false + @OptionGroup + var global: Flags.Global + + @OptionGroup + var registry: Flags.Registry @Argument var host: String func run() async throws { - let scheme = http ? "http" : "https" + let scheme = try RequestScheme(registry.scheme).schemeFor(host: host) + let _url = "\(scheme)://\(host)" guard let url = URL(string: _url), let domain = url.host() else { throw ContainerizationError(.invalidArgument, message: "Cannot convert \(_url) to URL") } let resolvedDomain = Reference.resolveDomain(domain: domain) - let client = RegistryClient(host: resolvedDomain, scheme: scheme, port: url.port) + let client = RegistryClient(host: resolvedDomain, scheme: scheme.rawValue, port: url.port) do { try await client.ping() } catch let err as RegistryClient.Error { diff --git a/Sources/CLI/RunCommand.swift b/Sources/CLI/RunCommand.swift index 80333ee9..22231801 100644 --- a/Sources/CLI/RunCommand.swift +++ b/Sources/CLI/RunCommand.swift @@ -33,10 +33,6 @@ extension Application { @OptionGroup var processFlags: Flags.Process - // FIXME: Add in detach keys support. - @OptionGroup(visibility: .hidden) - var detachFlags: Flags.Detach - @OptionGroup var resourceFlags: Flags.Resource @@ -44,7 +40,7 @@ extension Application { var managementFlags: Flags.Management @OptionGroup - var pullFlags: Flags.Pull + var registryFlags: Flags.Registry @OptionGroup var global: Flags.Global @@ -95,6 +91,7 @@ extension Application { process: processFlags, management: managementFlags, resource: resourceFlags, + registry: registryFlags, progressUpdate: progress.handler ) diff --git a/Sources/ContainerClient/Core/ClientImage.swift b/Sources/ContainerClient/Core/ClientImage.swift index 29dbd8d7..684f302c 100644 --- a/Sources/ContainerClient/Core/ClientImage.swift +++ b/Sources/ContainerClient/Core/ClientImage.swift @@ -201,15 +201,21 @@ extension ClientImage { }) } - public static func pull(reference: String, platform: Platform? = nil, insecure: Bool = false, progressUpdate: ProgressUpdateHandler? = nil) async throws -> ClientImage { + public static func pull(reference: String, platform: Platform? = nil, scheme: RequestScheme = .auto, progressUpdate: ProgressUpdateHandler? = nil) async throws -> ClientImage { let client = newXPCClient() let request = newRequest(.imagePull) let reference = try self.normalizeReference(reference) + guard let host = try Reference.parse(reference).domain else { + throw ContainerizationError(.invalidArgument, message: "Could not extract host from reference \(reference)") + } request.set(key: .imageReference, value: reference) try request.set(platform: platform) + let insecure = try scheme.schemeFor(host: host) == .http + request.set(key: .insecureFlag, value: insecure) + var progressUpdateClient: ProgressUpdateClient? if let progressUpdate { progressUpdateClient = await ProgressUpdateClient(for: progressUpdate, request: request) @@ -252,7 +258,8 @@ extension ClientImage { return (digests, size) } - public static func fetch(reference: String, platform: Platform? = nil, progressUpdate: ProgressUpdateHandler? = nil) async throws -> ClientImage { + public static func fetch(reference: String, platform: Platform? = nil, scheme: RequestScheme = .auto, progressUpdate: ProgressUpdateHandler? = nil) async throws -> ClientImage + { do { let match = try await self.get(reference: reference) if let platform { @@ -265,7 +272,7 @@ extension ClientImage { guard err.isCode(.notFound) else { throw err } - return try await Self.pull(reference: reference, platform: platform, progressUpdate: progressUpdate) + return try await Self.pull(reference: reference, platform: platform, scheme: scheme, progressUpdate: progressUpdate) } } } @@ -273,11 +280,18 @@ extension ClientImage { // MARK: Instance methods extension ClientImage { - public func push(platform: Platform? = nil, insecure: Bool = false, progressUpdate: ProgressUpdateHandler?) async throws { + public func push(platform: Platform? = nil, scheme: RequestScheme, progressUpdate: ProgressUpdateHandler?) async throws { let client = Self.newXPCClient() let request = Self.newRequest(.imagePush) - request.set(key: .imageReference, value: self.description.reference) + + guard let host = try Reference.parse(reference).domain else { + throw ContainerizationError(.invalidArgument, message: "Could not extract host from reference \(reference)") + } + request.set(key: .imageReference, value: reference) + + let insecure = try scheme.schemeFor(host: host) == .http request.set(key: .insecureFlag, value: insecure) + try request.set(platform: platform) var progressUpdateClient: ProgressUpdateClient? diff --git a/Sources/ContainerClient/Flags.swift b/Sources/ContainerClient/Flags.swift index 82587127..12d5bf1c 100644 --- a/Sources/ContainerClient/Flags.swift +++ b/Sources/ContainerClient/Flags.swift @@ -33,10 +33,6 @@ public struct Flags { help: "Current working directory for the container") public var cwd: String? - // FIXME: Implement. - // @Option(name: .customLong("shm-size"), help: "Size of /dev/shm") - // public var shmSize: String = "" - @Option(name: [.customLong("env"), .customShort("e")], help: "Set environment variables") public var env: [String] = [] @@ -73,17 +69,15 @@ public struct Flags { public var memory: String? } - public struct Pull: ParsableArguments { + public struct Registry: ParsableArguments { public init() {} - } + public init(scheme: String) { + self.scheme = scheme + } - public struct Detach: ParsableArguments { - public init() {} - - // FIXME: Implement. - // @Option(name: .customLong("detach-keys"), help: "Override the key sequence for detaching a container") - // public var detachKeys: String? + @Option(help: "Scheme to use when conntecting to the container registry. One of (http, https, auto)") + public var scheme: String = "auto" } public struct Management: ParsableArguments { @@ -114,10 +108,6 @@ public struct Flags { name: [.customLong("arch"), .customShort("a")], help: "Set arch if image can target multiple architectures") public var arch: String = Arch.hostArchitecture().rawValue - // FIXME: Implement - // @Option(name: [.customLong("publish"), .customShort("p")], help: "Publish a container's port(s) to the host") - // public var ports: [String] = [] - @Option(name: [.customLong("volume"), .customShort("v")], help: "Bind mount a volume into the container") public var volumes: [String] = [] diff --git a/Sources/ContainerClient/RequestScheme.swift b/Sources/ContainerClient/RequestScheme.swift new file mode 100644 index 00000000..f25eb7f5 --- /dev/null +++ b/Sources/ContainerClient/RequestScheme.swift @@ -0,0 +1,73 @@ +//===----------------------------------------------------------------------===// +// Copyright © 2025 Apple Inc. and the container project authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +//===----------------------------------------------------------------------===// + +import ContainerizationError + +/// The URL scheme to be used for a HTTP request. +public enum RequestScheme: String, Sendable { + case http = "http" + case https = "https" + + case auto = "auto" + + public init(_ rawValue: String) throws { + switch rawValue { + case RequestScheme.http.rawValue: + self = .http + case RequestScheme.https.rawValue: + self = .https + case RequestScheme.auto.rawValue: + self = .auto + default: + throw ContainerizationError(.invalidArgument, message: "Unsupported scheme \(rawValue)") + } + } + + /// Returns the prescribed protocol to use while making a HTTP request to a webserver + /// - Parameter host: The domain or IP address of the webserver + /// - Returns: RequestScheme + package func schemeFor(host: String) throws -> Self { + guard host.count > 0 else { + throw ContainerizationError(.invalidArgument, message: "Host cannot be empty") + } + switch self { + case .http, .https: + return self + case .auto: + return Self.isInternalHost(host: host) ? .http : .https + } + } + + /// Checks if the given `host` string is a private IP address + /// or a domain typically reachable only on the local system. + private static func isInternalHost(host: String) -> Bool { + if host.hasPrefix("localhost") || host.hasPrefix("127.") { + return true + } + if host.hasPrefix("192.168.") || host.hasPrefix("10.") { + return true + } + let regex = "(^172\\.1[6-9]\\.)|(^172\\.2[0-9]\\.)|(^172\\.3[0-1]\\.)" + if host.range(of: regex, options: .regularExpression) != nil { + return true + } + let dnsDomain = ClientDefaults.get(key: .defaultDNSDomain) + if host.hasSuffix(".\(dnsDomain)") { + return true + } + return false + } +} diff --git a/Sources/ContainerClient/Utility.swift b/Sources/ContainerClient/Utility.swift index c050efa5..1b8bfc52 100644 --- a/Sources/ContainerClient/Utility.swift +++ b/Sources/ContainerClient/Utility.swift @@ -68,9 +68,11 @@ public struct Utility { process: Flags.Process, management: Flags.Management, resource: Flags.Resource, + registry: Flags.Registry, progressUpdate: @escaping ProgressUpdateHandler ) async throws -> (ContainerConfiguration, Kernel) { let requestedPlatform = Parser.platform(os: management.os, arch: management.arch) + let scheme = try RequestScheme(registry.scheme) await progressUpdate([ .setDescription("Fetching image"), @@ -81,6 +83,7 @@ public struct Utility { let img = try await ClientImage.fetch( reference: image, platform: requestedPlatform, + scheme: scheme, progressUpdate: ProgressTaskCoordinator.handler(for: fetchTask, from: progressUpdate) ) @@ -95,14 +98,11 @@ public struct Utility { progressUpdate: ProgressTaskCoordinator.handler(for: unpackTask, from: progressUpdate)) await progressUpdate([ - .setDescription("Fetching kernel image"), - .setItemsName("blobs"), + .setDescription("Fetching kernel"), + .setItemsName("binary"), ]) - let fetchKernelTask = await taskManager.startTask() - let kernel = try await self.getKernel( - management: management, - progressUpdate: ProgressTaskCoordinator.handler(for: fetchKernelTask, from: progressUpdate)) + let kernel = try await self.getKernel(management: management) // Pull and unpack the initial filesystem await progressUpdate([ @@ -111,7 +111,7 @@ public struct Utility { ]) let fetchInitTask = await taskManager.startTask() let initImage = try await ClientImage.fetch( - reference: ClientImage.initImageRef, platform: .current, + reference: ClientImage.initImageRef, platform: .current, scheme: scheme, progressUpdate: ProgressTaskCoordinator.handler(for: fetchInitTask, from: progressUpdate)) await progressUpdate([ @@ -185,18 +185,10 @@ public struct Utility { return (config, kernel) } - private static func getKernel(management: Flags.Management, progressUpdate: @escaping ProgressUpdateHandler) async throws -> Kernel { + private static func getKernel(management: Flags.Management) async throws -> Kernel { // For the image itself we'll take the user input and try with it as we can do userspace // emulation for x86, but for the kernel we need it to match the hosts architecture. - let s: SystemPlatform - switch Platform.current.architecture { - case "arm64": - s = .linuxArm - case "amd64": - s = .linuxAmd - default: - throw ContainerizationError.init(.unsupported, message: "platform architecture \(Platform.current.architecture)") - } + let s: SystemPlatform = .current if let userKernel = management.kernel { guard FileManager.default.fileExists(atPath: userKernel) else { throw ContainerizationError(.notFound, message: "Kernel file not found at path \(userKernel)") diff --git a/Sources/Services/ContainerImagesService/Server/ImageService.swift b/Sources/Services/ContainerImagesService/Server/ImageService.swift index 1ba2dfe9..d56ddcb1 100644 --- a/Sources/Services/ContainerImagesService/Server/ImageService.swift +++ b/Sources/Services/ContainerImagesService/Server/ImageService.swift @@ -169,11 +169,17 @@ extension ImagesService { authentication = try? keychain.lookup(domain: host) do { return try await body(authentication) - } catch { - guard authentication != nil else { - throw ContainerizationError(.internalError, message: "\(String(describing: error)). No credentials found for host \(host)") + } catch let err as RegistryClient.Error { + guard case .invalidStatus(_, let status) = err else { + throw err } - throw error + guard status == .unauthorized || status == .forbidden else { + throw err + } + guard authentication != nil else { + throw ContainerizationError(.internalError, message: "\(String(describing: err)). No credentials found for host \(host)") + } + throw err } } diff --git a/Tests/ContainerClientTests/RequestSchemeTests.swift b/Tests/ContainerClientTests/RequestSchemeTests.swift new file mode 100644 index 00000000..35144467 --- /dev/null +++ b/Tests/ContainerClientTests/RequestSchemeTests.swift @@ -0,0 +1,63 @@ +//===----------------------------------------------------------------------===// +// Copyright © 2025 Apple Inc. and the container project authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +//===----------------------------------------------------------------------===// + +// + +import ContainerizationError +import Foundation +import Testing + +@testable import ContainerClient + +struct RequestSchemeTests { + static let defaultDnsDomain = ClientDefaults.get(key: .defaultDNSDomain) + + internal struct TestArg { + let scheme: String + let host: String + let expected: RequestScheme + } + + @Test(arguments: [ + TestArg(scheme: "http", host: "myregistry.io", expected: .http), + TestArg(scheme: "https", host: "myregistry.io", expected: .https), + TestArg(scheme: "auto", host: "myregistry.io", expected: .https), + TestArg(scheme: "https", host: "localhost", expected: .https), + TestArg(scheme: "http", host: "localhost", expected: .http), + TestArg(scheme: "auto", host: "localhost", expected: .http), + TestArg(scheme: "http", host: "127.0.0.1", expected: .http), + TestArg(scheme: "https", host: "127.0.0.1", expected: .https), + TestArg(scheme: "auto", host: "127.0.0.1", expected: .http), + TestArg(scheme: "https", host: "10.3.4.1", expected: .https), + TestArg(scheme: "auto", host: "10.3.4.1", expected: .http), + TestArg(scheme: "auto", host: "some-dns-name.io.\(Self.defaultDnsDomain)", expected: .http), + TestArg(scheme: "auto", host: "some-dns-name.io", expected: .https), + TestArg(scheme: "auto", host: "172.32.0.1", expected: .https), + TestArg(scheme: "auto", host: "172.22.23.61", expected: .http), + ]) + + func testIsConnectionSecure(arg: TestArg) throws { + let requestScheme = RequestScheme(rawValue: arg.scheme)! + #expect(try requestScheme.schemeFor(host: arg.host) == arg.expected) + } + + func testEmptyHostThrowsError() throws { + #expect(throws: (any Error).self) { + let requestScheme = RequestScheme(rawValue: "https")! + _ = try requestScheme.schemeFor(host: "") + } + } +}