PIV: Curve 25519 keys and api surface grooming #36
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
jdmoreira
force-pushed
the
joao/piv-curve25519-keys
branch
from
aef8022 to
bfb73f5
Compare
jdmoreira
changed the title
jdmoreira
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for Curve25519 key types and refactors the PIVSession public API to remove direct dependencies on Apple’s Security framework, replacing them with PIV-specific abstractions.
- Added Ed25519 and X25519 key types and a generic
PublicKeyenum. - Updated
PIVSessionto usePIV.RSASignatureAlgorithm,PIV.ECDSASignatureAlgorithm, etc., and replacedPIVSessionErrorwithPIV.SessionError. - Migrated full-stack and unit tests to Swift Testing, adjusting expectations to the new API.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| SecKey+Extensions.swift | Switched from PIVKeyType/PIVSessionError to PIV.KeyType/PIV.SessionError and added ed25519/x25519 support. |
| PIVDataTypes.swift | Introduced PIV.PinPolicy, PIV.Slot, PIV.KeyType, PIV.RSASignatureAlgorithm, etc. |
| PIVSession.swift | Overhauled sign/decrypt/putKey/calculateSecretKey methods to use new PIV enums and handle Curve25519. |
| PIVDataFormatter.swift | Replaced old padding helpers with unified formatting utilities for RSA/ECDSA/Curve25519. |
| FullStackTests & UnitTests | Migrated to Swift Testing DSL and updated calls to the revised API surface. |
Comments suppressed due to low confidence (2)
YubiKit/YubiKit/PIV/PIVDataFormatter.swift:213
- There are no unit tests covering
extractDataFromRSASigning, which could lead to undetected errors in signature extraction. Consider adding tests similar to the RSA encryption extraction tests.
internal static func extractDataFromRSASigning(_ data: Data, algorithm: PIV.RSASignatureAlgorithm) throws -> Data {
YubiKit/YubiKit/PIV/PIVSession.swift:1
- [nitpick] The
PIVSessionimplementation has grown very large with many overloads and specialized methods. Consider splitting functionality (e.g., key generation, import/export, signature operations) into protocol-conforming extensions or helper types to improve readability and maintainability.
// Copyright Yubico AB
|
|
||
| /// PIV management key type. | ||
| public enum ManagementKeyType: UInt8, Sendable { | ||
| /// 3-des (default) |
There was a problem hiding this comment.
Maybe worth noting: The default as of YK 5.7 is AES-192
| if pinPolicy != .`defaultPolicy` { | ||
| data.append(TKBERTLVRecord(tag: tagPinPolicy, value: pinPolicy.rawValue.data).data) | ||
| } | ||
| if touchPolicy != .`defaultPolicy` { | ||
| data.append(TKBERTLVRecord(tag: tagTouchPolicy, value: touchPolicy.rawValue.data).data) | ||
| } | ||
| let apdu = APDU( | ||
| cla: 0, | ||
| ins: insImportKey, | ||
| p1: keyType.rawValue, | ||
| p2: slot.rawValue, | ||
| command: data, | ||
| type: .extended | ||
| ) | ||
| try await send(apdu: apdu) |
There was a problem hiding this comment.
This code is duplicated for all the putKey variants, would it make sense to extract a function for this?
| @@ -363,9 +571,9 @@ public final actor PIVSession: Session { | |||
| /// - Parameters: | |||
| /// - managementKey: The management key as Data. | |||
| /// - keyType: The management key type. | |||
| public func authenticateWith(managementKey: Data, keyType: PIVManagementKeyType) async throws { | |||
| public func authenticateWith(managementKey: Data, keyType: PIV.ManagementKeyType) async throws { | |||
There was a problem hiding this comment.
For our other SDKs we've decided to deprecate being able to specify keyType here, as we can determine it by reading the ManagementKeyMetadata. We should probably do that from the start here for Swift?
Motivation
When introducing the new Curve25519 key types (which are unsupported by
SecKey) it became obvious that some of the public methods in PIVSession were too tied to Apple'sSecurityframework by receivingSecAlgorithm.This pull request is an attempt at abstracting the PIVSession public API from any concrete Apple dependency.
Under the hood we still rely on the
Securityframework for some helpers, and now also onCryptoKitfor Curve25519 validation, but the public api is now decoupled from these implementation details. This PR paves the way for any key type in the public api.Important changes
Curve25519Keys.swift