From a0216bd758a8b8b8e83ee7c6b9ef2f6bb7abc0cb Mon Sep 17 00:00:00 2001 From: Hiroshi SHIBATA Date: Tue, 30 Jun 2026 13:02:39 +0900 Subject: [PATCH 1/2] Read registry credentials in the CLI, not the images helper registry login stores the keychain item with an ACL that trusts only the writing process (the CLI, com.apple.container.cli), so reading it from the container-core-images helper, which has a different code identity, fails with errSecInteractionNotAllowed (-25308) in a non-interactive XPC service. Resolve the credential in the CLI where login wrote it and forward the Authorization header to the helper over XPC, fixing both pull and push. Co-Authored-By: Claude Opus 4.8 --- .../Client/ClientImage.swift | 37 ++++++++++++++++ .../Client/ImageServiceXPCKeys.swift | 3 ++ .../Server/ImagesService.swift | 44 +++++++++++-------- .../Server/ImagesServiceHarness.swift | 6 ++- 4 files changed, 70 insertions(+), 20 deletions(-) diff --git a/Sources/Services/ContainerAPIService/Client/ClientImage.swift b/Sources/Services/ContainerAPIService/Client/ClientImage.swift index 3516d0ef9..1f095fcb5 100644 --- a/Sources/Services/ContainerAPIService/Client/ClientImage.swift +++ b/Sources/Services/ContainerAPIService/Client/ClientImage.swift @@ -271,6 +271,10 @@ extension ClientImage { request.set(key: .insecureFlag, value: insecure) request.set(key: .maxConcurrentDownloads, value: Int64(maxConcurrentDownloads)) + if let authorization = try await Self.registryAuthorization(for: reference) { + request.set(key: .registryAuthorization, value: authorization) + } + var progressUpdateClient: ProgressUpdateClient? if let progressUpdate { progressUpdateClient = await ProgressUpdateClient(for: progressUpdate, request: request) @@ -284,6 +288,35 @@ extension ClientImage { return image } + /// Resolve registry credentials from the keychain and return the + /// `Authorization` header value to forward to the images helper. + /// + /// The lookup runs in this client process, which for `container image + /// pull`/`push` and `container build` is always the `container` CLI binary + /// (`com.apple.container.cli`). That is the same code identity that + /// `container registry login` uses when it writes the keychain item, so the + /// item's access control accepts this read. The images helper runs under a + /// different identity (`com.apple.container.container-core-images`) and is + /// rejected by that access control, so it must not read the keychain itself. + /// + /// Returns `nil` when no stored credentials exist for the reference's host. + private static func registryAuthorization(for reference: String) async throws -> String? { + guard let host = try Reference.parse(reference).resolvedDomain else { + return nil + } + let keychain = KeychainHelper(securityDomain: Constants.keychainID) + let auth: Authentication + do { + auth = try keychain.lookup(hostname: host) + } catch let err as KeychainHelper.Error { + guard case .keyNotFound = err else { + throw ContainerizationError(.internalError, message: "error querying keychain for \(host)", cause: err) + } + return nil + } + return try await auth.token() + } + public static func delete(reference: String, garbageCollect: Bool = false) async throws { let client = newXPCClient() let request = newRequest(.imageDelete) @@ -395,6 +428,10 @@ extension ClientImage { try request.set(platform: platform) + if let authorization = try await Self.registryAuthorization(for: reference) { + request.set(key: .registryAuthorization, value: authorization) + } + var progressUpdateClient: ProgressUpdateClient? if let progressUpdate { progressUpdateClient = await ProgressUpdateClient(for: progressUpdate, request: request) diff --git a/Sources/Services/ContainerImagesService/Client/ImageServiceXPCKeys.swift b/Sources/Services/ContainerImagesService/Client/ImageServiceXPCKeys.swift index c087f99f1..566d2c0e8 100644 --- a/Sources/Services/ContainerImagesService/Client/ImageServiceXPCKeys.swift +++ b/Sources/Services/ContainerImagesService/Client/ImageServiceXPCKeys.swift @@ -38,6 +38,9 @@ public enum ImagesServiceXPCKeys: String { case maxConcurrentDownloads case forceLoad case rejectedMembers + /// Pre-resolved registry `Authorization` header value, read from the + /// keychain on the client (CLI) side and forwarded to the images helper. + case registryAuthorization /// ContentStore case digest diff --git a/Sources/Services/ContainerImagesService/Server/ImagesService.swift b/Sources/Services/ContainerImagesService/Server/ImagesService.swift index 21a3e5146..a30cadbac 100644 --- a/Sources/Services/ContainerImagesService/Server/ImagesService.swift +++ b/Sources/Services/ContainerImagesService/Server/ImagesService.swift @@ -79,7 +79,7 @@ public actor ImagesService { return try await imageStore.list().map { $0.description.fromCZ } } - public func pull(reference: String, platform: Platform?, insecure: Bool, progressUpdate: ProgressUpdateHandler?, maxConcurrentDownloads: Int = 3) async throws + public func pull(reference: String, platform: Platform?, insecure: Bool, auth: Authentication?, progressUpdate: ProgressUpdateHandler?, maxConcurrentDownloads: Int = 3) async throws -> ImageDescription { self.log.debug( @@ -103,7 +103,7 @@ public actor ImagesService { ) } - let img = try await Self.withAuthentication(ref: reference) { auth in + let img = try await Self.withAuthentication(ref: reference, auth: auth) { auth in try await self.imageStore.pull( reference: reference, platform: platform, insecure: insecure, auth: auth, progress: ContainerizationProgressAdapter.handler(from: progressUpdate), maxConcurrentDownloads: maxConcurrentDownloads) @@ -114,7 +114,7 @@ public actor ImagesService { return img.description.fromCZ } - public func push(reference: String, platform: Platform?, insecure: Bool, progressUpdate: ProgressUpdateHandler?) async throws { + public func push(reference: String, platform: Platform?, insecure: Bool, auth: Authentication?, progressUpdate: ProgressUpdateHandler?) async throws { self.log.debug( "ImagesService: enter", metadata: [ @@ -135,7 +135,7 @@ public actor ImagesService { ) } - try await Self.withAuthentication(ref: reference) { auth in + try await Self.withAuthentication(ref: reference, auth: auth) { auth in try await self.imageStore.push( reference: reference, platform: platform, insecure: insecure, auth: auth, progress: ContainerizationProgressAdapter.handler(from: progressUpdate)) } @@ -412,25 +412,20 @@ extension ImagesService { extension ImagesService { private static func withAuthentication( - ref: String, _ body: @Sendable @escaping (_ auth: Authentication?) async throws -> T? + ref: String, auth: Authentication?, _ body: @Sendable @escaping (_ auth: Authentication?) async throws -> T? ) async throws -> T? { - var authentication: Authentication? let ref = try Reference.parse(ref) guard let host = ref.resolvedDomain else { throw ContainerizationError(.invalidArgument, message: "no host specified in image reference: \(ref)") } - authentication = Self.authenticationFromEnv(host: host) - if let authentication { - return try await body(authentication) - } - let keychain = KeychainHelper(securityDomain: Constants.keychainID) - do { - authentication = try keychain.lookup(hostname: host) - } catch let err as KeychainHelper.Error { - guard case .keyNotFound = err else { - throw ContainerizationError(.internalError, message: "error querying keychain for \(host)", cause: err) - } - } + // Environment credentials take precedence, preserving prior behavior. + // Keychain credentials are resolved on the client (CLI) side and passed + // in as `auth`. This helper must not read the keychain itself: its code + // identity (`com.apple.container.container-core-images`) differs from the + // writer's (`com.apple.container.cli`), so the keychain item's access + // control would reject the read and securityd would fail it with + // errSecInteractionNotAllowed in this non-interactive process. + let authentication = Self.authenticationFromEnv(host: host) ?? auth do { return try await body(authentication) } catch let err as RegistryClient.Error { @@ -459,6 +454,19 @@ extension ImagesService { } } +/// An `Authentication` backed by a pre-resolved `Authorization` header value. +/// +/// Registry credentials are read from the keychain on the client (CLI) side and +/// the resulting header is forwarded to the images helper over XPC, so the +/// helper never reads the keychain itself. +struct ResolvedAuthentication: Authentication { + let authorization: String + + func token() async throws -> String { + authorization + } +} + extension ImageDescription { public var toCZ: Containerization.Image.Description { .init(reference: self.reference, descriptor: self.descriptor) diff --git a/Sources/Services/ContainerImagesService/Server/ImagesServiceHarness.swift b/Sources/Services/ContainerImagesService/Server/ImagesServiceHarness.swift index e88b2368f..d19ef94eb 100644 --- a/Sources/Services/ContainerImagesService/Server/ImagesServiceHarness.swift +++ b/Sources/Services/ContainerImagesService/Server/ImagesServiceHarness.swift @@ -49,10 +49,11 @@ public struct ImagesServiceHarness: Sendable { } let insecure = message.bool(key: .insecureFlag) let maxConcurrentDownloads = message.int64(key: .maxConcurrentDownloads) + let auth = message.string(key: .registryAuthorization).map { ResolvedAuthentication(authorization: $0) } let progressUpdateService = ProgressUpdateService(message: message) let imageDescription = try await service.pull( - reference: ref, platform: platform, insecure: insecure, progressUpdate: progressUpdateService?.handler, maxConcurrentDownloads: Int(maxConcurrentDownloads)) + reference: ref, platform: platform, insecure: insecure, auth: auth, progressUpdate: progressUpdateService?.handler, maxConcurrentDownloads: Int(maxConcurrentDownloads)) let imageData = try JSONEncoder().encode(imageDescription) let reply = message.reply() @@ -75,9 +76,10 @@ public struct ImagesServiceHarness: Sendable { platform = try JSONDecoder().decode(ContainerizationOCI.Platform.self, from: platformData) } let insecure = message.bool(key: .insecureFlag) + let auth = message.string(key: .registryAuthorization).map { ResolvedAuthentication(authorization: $0) } let progressUpdateService = ProgressUpdateService(message: message) - try await service.push(reference: ref, platform: platform, insecure: insecure, progressUpdate: progressUpdateService?.handler) + try await service.push(reference: ref, platform: platform, insecure: insecure, auth: auth, progressUpdate: progressUpdateService?.handler) let reply = message.reply() return reply From 3f905c4164a02e1e5d723e89525d359f2ecd9b79 Mon Sep 17 00:00:00 2001 From: Hiroshi SHIBATA Date: Tue, 30 Jun 2026 20:31:10 +0900 Subject: [PATCH 2/2] Add regression test for CLI-forwarded registry credentials Pins that the images helper builds its Authentication from the Authorization header forwarded over XPC rather than reading the keychain itself, guarding against a regression to the errSecInteractionNotAllowed (-25308) failure. Extracts the harness credential decoding into authentication(from:) so it can be exercised without a running daemon. Co-Authored-By: Claude Opus 4.8 --- Package.swift | 9 +++ .../Server/ImagesServiceHarness.swift | 17 +++++- .../RegistryAuthForwardingTests.swift | 60 +++++++++++++++++++ 3 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 Tests/ContainerImagesServiceTests/RegistryAuthForwardingTests.swift diff --git a/Package.swift b/Package.swift index 140b9fee0..b55d5c714 100644 --- a/Package.swift +++ b/Package.swift @@ -307,6 +307,15 @@ let package = Package( ], path: "Sources/Services/ContainerImagesService/Client" ), + .testTarget( + name: "ContainerImagesServiceTests", + dependencies: [ + .product(name: "ContainerizationOCI", package: "containerization"), + "ContainerImagesService", + "ContainerImagesServiceClient", + "ContainerXPC", + ] + ), .executableTarget( name: "container-network-vmnet", dependencies: [ diff --git a/Sources/Services/ContainerImagesService/Server/ImagesServiceHarness.swift b/Sources/Services/ContainerImagesService/Server/ImagesServiceHarness.swift index d19ef94eb..3ac3c4b20 100644 --- a/Sources/Services/ContainerImagesService/Server/ImagesServiceHarness.swift +++ b/Sources/Services/ContainerImagesService/Server/ImagesServiceHarness.swift @@ -33,6 +33,19 @@ public struct ImagesServiceHarness: Sendable { self.service = service } + /// Build the registry `Authentication` forwarded from the CLI over XPC, or + /// nil when the request carried no credentials. + /// + /// Registry credentials are read from the keychain on the CLI side (the + /// `com.apple.container.cli` identity that wrote them) and forwarded here as + /// an `Authorization` header value. The images helper must not read the + /// keychain itself: its distinct code identity is rejected by the item's + /// access control, and securityd fails the read with + /// errSecInteractionNotAllowed (-25308) in this non-interactive process. + static func authentication(from message: XPCMessage) -> Authentication? { + message.string(key: .registryAuthorization).map { ResolvedAuthentication(authorization: $0) as Authentication } + } + @Sendable public func pull(_ message: XPCMessage) async throws -> XPCMessage { let ref = message.string(key: .imageReference) @@ -49,7 +62,7 @@ public struct ImagesServiceHarness: Sendable { } let insecure = message.bool(key: .insecureFlag) let maxConcurrentDownloads = message.int64(key: .maxConcurrentDownloads) - let auth = message.string(key: .registryAuthorization).map { ResolvedAuthentication(authorization: $0) } + let auth = Self.authentication(from: message) let progressUpdateService = ProgressUpdateService(message: message) let imageDescription = try await service.pull( @@ -76,7 +89,7 @@ public struct ImagesServiceHarness: Sendable { platform = try JSONDecoder().decode(ContainerizationOCI.Platform.self, from: platformData) } let insecure = message.bool(key: .insecureFlag) - let auth = message.string(key: .registryAuthorization).map { ResolvedAuthentication(authorization: $0) } + let auth = Self.authentication(from: message) let progressUpdateService = ProgressUpdateService(message: message) try await service.push(reference: ref, platform: platform, insecure: insecure, auth: auth, progressUpdate: progressUpdateService?.handler) diff --git a/Tests/ContainerImagesServiceTests/RegistryAuthForwardingTests.swift b/Tests/ContainerImagesServiceTests/RegistryAuthForwardingTests.swift new file mode 100644 index 000000000..f013b1732 --- /dev/null +++ b/Tests/ContainerImagesServiceTests/RegistryAuthForwardingTests.swift @@ -0,0 +1,60 @@ +//===----------------------------------------------------------------------===// +// Copyright © 2026 Apple Inc. and the container project authors. +// +// 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 ContainerImagesServiceClient +import ContainerXPC +import ContainerizationOCI +import Testing + +@testable import ContainerImagesService + +/// Regression coverage for reading registry credentials in the CLI and forwarding +/// them to the images helper over XPC (commit "Read registry credentials in the +/// CLI, not the images helper"). +/// +/// The bug was that the `container-core-images` helper read the keychain itself +/// and failed with errSecInteractionNotAllowed (-25308), because its code identity +/// differs from the CLI that wrote the item. The fix forwards the resolved +/// `Authorization` header over XPC. These tests pin that the helper consumes the +/// forwarded credential rather than touching the keychain. +@Suite +struct RegistryAuthForwardingTests { + private let authorization = "Basic dXNlcjpwYXNz" // base64("user:pass") + + /// The wrapper must hand back the forwarded `Authorization` value unchanged, + /// including its scheme prefix, so the registry handshake sees what the CLI read. + @Test func resolvedAuthenticationReturnsAuthorizationVerbatim() async throws { + let auth = ResolvedAuthentication(authorization: authorization) + #expect(try await auth.token() == authorization) + } + + /// A request carrying the credential must decode into an `Authentication` + /// whose token matches it — the helper consumes it instead of reading the keychain. + @Test func harnessDecodesForwardedAuthorization() async throws { + let message = XPCMessage(route: ImagesServiceXPCRoute.imagePull.rawValue) + message.set(key: .registryAuthorization, value: authorization) + + let auth = try #require(ImagesServiceHarness.authentication(from: message)) + #expect(try await auth.token() == authorization) + } + + /// No forwarded credential means anonymous access (e.g. public image pulls), + /// so no `Authentication` is produced. + @Test func harnessReturnsNilWithoutForwardedAuthorization() { + let message = XPCMessage(route: ImagesServiceXPCRoute.imagePull.rawValue) + #expect(ImagesServiceHarness.authentication(from: message) == nil) + } +}