RSA support and modularity add-ons #87

dscreve · 2025-01-25T19:06:12Z

Make ChallengeGenerator more flexible and overridable
Add custom challenge for authentication and registration
Implement Codable in most object Add public init on most objects
Implements RSA (required for Windows Hello)
Add OpenAPI support on PublicKeyCredentialRequestOptions and PublicKeyCredentialCreationOptions

… challenge for authentication and registration) Implement Codable in most object Add public init on most object Implements RSA

dimitribouniol

Thank you for this PR! It seems like there are four main things this introduces, and I'd like to propose you split out some of them to keep changes clear and to the point:

RSA support: Please keep this PR focussed on RSA support, and add appropriate tests — what's here so far looks good so far!
Public inits, mutable properties, and full Codable support: Please split this out into it's own PR, as it seems to be quite inconsistently applied, and could benefit from a more careful review to make sure we get everything properly.
Customizable Challenges: I'm not sure there is good motivation to customize this, but either way, not sure RSA support is blocked by it, so no need to block it on reaching consensus here.
VaporOpenAPI support: this feels like it belongs in a completely separate package. Vapor Community has many such packages, and it may find a more appropriate home there.

dimitribouniol · 2025-01-26T20:36:35Z

Package.swift

@@ -36,7 +37,8 @@ let package = Package(
                .product(name: "Crypto", package: "swift-crypto"),
                .product(name: "_CryptoExtras", package: "swift-crypto"),
                .product(name: "Logging", package: "swift-log"),
-            ],
+                .product(name:"VaporToOpenAPI",package:"VaporToOpenAPI"),


This doesn't feel appropriate to add to this package as it isn't guaranteed to be used with vapor or OpenAPI. As an alternative to removing it completely, could this instead live in a separate package that augments swift-webauthn for those who want this functionality.

dimitribouniol · 2025-01-26T20:38:32Z

Sources/WebAuthn/Ceremonies/Authentication/AuthenticationCredential.swift

+    public func encode(to encoder: Encoder) throws {
+         var container = encoder.container(keyedBy: CodingKeys.self)
+
+         try container.encode(id, forKey: .id)
+         try container.encode(rawID.base64URLEncodedString(), forKey: .rawID)
+         try container.encode(response, forKey: .response)
+         try container.encodeIfPresent(authenticatorAttachment, forKey: .authenticatorAttachment)
+         try container.encode(type, forKey: .type)
+      }


nit: formatting

dimitribouniol · 2025-01-26T20:39:10Z

Sources/WebAuthn/Ceremonies/Authentication/AuthenticatorAssertionResponse.swift

+    public func encode(to encoder: Encoder) throws {
+         var container = encoder.container(keyedBy: CodingKeys.self)
+
+        try container.encode(clientDataJSON.base64URLEncodedString(), forKey: .clientDataJSON)
+        try container.encode(authenticatorData.base64URLEncodedString(), forKey: .authenticatorData)
+        try container.encode(signature.base64URLEncodedString(), forKey: .signature)
+        try container.encodeIfPresent(userHandle?.base64URLEncodedString(),forKey: .userHandle)
+        try container.encodeIfPresent(attestationObject?.base64URLEncodedString(),forKey: .attestationObject)
+      }


nit: formatting

dimitribouniol · 2025-01-26T20:40:15Z

Sources/WebAuthn/Ceremonies/Authentication/PublicKeyCredentialRequestOptions.swift

+    public init(_ src : PublicKeyCredentialRequestOptions) {
+        self.challenge=src.challenge
+        self.timeout=src.timeout
+        self.relyingPartyID=src.relyingPartyID
+        self.allowCredentials=src.allowCredentials
+        self.userVerification = src.userVerification
+    }


There is no point to this, you can just copy it since it's a struct.

dimitribouniol · 2025-01-26T20:41:30Z

Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift

+    public init(challenge: [UInt8],user: PublicKeyCredentialUserEntity,relyingParty: PublicKeyCredentialRelyingPartyEntity,publicKeyCredentialParameters: [PublicKeyCredentialParameters],
+                timeout: Duration?,attestation: AttestationConveyancePreference) {


nit: formatting

Suggested change

public init(challenge: [UInt8],user: PublicKeyCredentialUserEntity,relyingParty: PublicKeyCredentialRelyingPartyEntity,publicKeyCredentialParameters: [PublicKeyCredentialParameters],

timeout: Duration?,attestation: AttestationConveyancePreference) {

public init(

challenge: [UInt8],

user: PublicKeyCredentialUserEntity,relyingParty: PublicKeyCredentialRelyingPartyEntity,

publicKeyCredentialParameters: [PublicKeyCredentialParameters],

timeout: Duration?,

attestation: AttestationConveyancePreference

) {

dimitribouniol · 2025-01-26T20:43:30Z

Sources/WebAuthn/Ceremonies/Shared/CredentialPublicKey.swift

+        //throw WebAuthnError.unsupported
+        let rsaSignature = _RSA.Signing.RSASignature(rawRepresentation: signature)
+
+        var rsaPadding: _RSA.Signing.Padding
+        switch algorithm {
+        case .algRS1, .algRS256, .algRS384, .algRS512:
+            rsaPadding = .insecurePKCS1v1_5
+        case .algPS256, .algPS384, .algPS512:
+            rsaPadding = .PSS
+        default:
+            throw WebAuthnError.unsupportedCOSEAlgorithmForRSAPublicKey
+        }
+
+        guard try _RSA.Signing.PublicKey(derRepresentation: rawRepresentation).isValidSignature(
+            rsaSignature,
+            for: data,
+            padding: rsaPadding
+        ) else {
+            throw WebAuthnError.invalidSignature
+        }


Could you please add tests for these?

I've written the tests, but unfortunately, CryptoKit does not allow the creation of 256 bit private key...So, tests cannot be performed.

Are you sure you are referencing the right key size? https://www.rfc-editor.org/rfc/rfc8812.html

You are right : I inverted key size and hash.
I fix it.

dimitribouniol · 2025-01-26T20:44:29Z

Sources/WebAuthn/Helpers/ChallengeGenerator.swift

-package struct ChallengeGenerator: Sendable {
-    var generate: @Sendable () -> [UInt8]
+public protocol ChallengeGenerator : Sendable {
+    func generate() -> [UInt8]
+}

-    package static var live: Self {
-        .init(generate: { [UInt8].random(count: 32) })
+public struct DefaultChallengeGenerator: Sendable, ChallengeGenerator {
+    public func generate() -> [UInt8] {
+        return [UInt8].random(count: 32)
    }
+
+    public static let live = DefaultChallengeGenerator()


I don't think I see the motivation for changing this.

dscreve · 2025-01-26T21:49:28Z

Hello,I agree with the need to split this PR and I will do it. Tests for RSA have been performed on a real Windows Hello environment. I’ll write some automated tests.Customisable challenge is required because, like many webauthn implementations, this one cannot be used in a stateless environment if you run a load balanced infrastructure : You cannot ensure that start/finish are both executed on the same server, then challenge cannot be stored in memory and it is a security problem if you store them in a database. The only solution is to have a custom challenge that allows a custom prevalidation.I cannot migrate vaporToOpenApi on a separate package because @OpenApiDescriptable annotation must be applied to structure directly. I will remove but I will need to duplicate the structure in my server package for documentation purpose.David ScrèveLe 26 janv. 2025 à 21:52, Dimitri Bouniol ***@***.***> a écrit : @dimitribouniol requested changes on this pull request. Thank you for this PR! It seems like there are four main things this introduces, and I'd like to propose you split out some of them to keep changes clear and to the point: RSA support: Please keep this PR focussed on RSA support, and add appropriate tests — what's here so far looks good so far! Public inits, mutable properties, and full Codable support: Please split this out into it's own PR, as it seems to be quite inconsistently applied, and could benefit from a more careful review to make sure we get everything properly. Customizable Challenges: I'm not sure there is good motivation to customize this, but either way, not sure RSA support is blocked by it, so no need to block it on reaching consensus here. VaporOpenAPI support: this feels like it belongs in a completely separate package. Vapor Community has many such packages, and it may find a more appropriate home there. In Package.swift:

@@ -36,7 +37,8 @@ let package = Package(

.product(name: "Crypto", package: "swift-crypto"), .product(name: "_CryptoExtras", package: "swift-crypto"), .product(name: "Logging", package: "swift-log"), - ], + .product(name:"VaporToOpenAPI",package:"VaporToOpenAPI"), This doesn't feel appropriate to add to this package as it isn't guaranteed to be used with vapor or OpenAPI. As an alternative to removing it completely, could this instead live in a separate package that augments swift-webauthn for those who want this functionality. In Sources/WebAuthn/Ceremonies/Authentication/AuthenticationCredential.swift:

+ public func encode(to encoder: Encoder) throws {

+ var container = encoder.container(keyedBy: CodingKeys.self) + + try container.encode(id, forKey: .id) + try container.encode(rawID.base64URLEncodedString(), forKey: .rawID) + try container.encode(response, forKey: .response) + try container.encodeIfPresent(authenticatorAttachment, forKey: .authenticatorAttachment) + try container.encode(type, forKey: .type) + } nit: formatting In Sources/WebAuthn/Ceremonies/Authentication/AuthenticatorAssertionResponse.swift:

+ public func encode(to encoder: Encoder) throws {

+ var container = encoder.container(keyedBy: CodingKeys.self) + + try container.encode(clientDataJSON.base64URLEncodedString(), forKey: .clientDataJSON) + try container.encode(authenticatorData.base64URLEncodedString(), forKey: .authenticatorData) + try container.encode(signature.base64URLEncodedString(), forKey: .signature) + try container.encodeIfPresent(userHandle?.base64URLEncodedString(),forKey: .userHandle) + try container.encodeIfPresent(attestationObject?.base64URLEncodedString(),forKey: .attestationObject) + } nit: formatting In Sources/WebAuthn/Ceremonies/Authentication/PublicKeyCredentialRequestOptions.swift:

+ public init(_ src : PublicKeyCredentialRequestOptions) {

+ self.challenge=src.challenge + self.timeout=src.timeout + self.relyingPartyID=src.relyingPartyID + self.allowCredentials=src.allowCredentials + self.userVerification = src.userVerification + } There is no point to this, you can just copy it since it's a struct. In Sources/WebAuthn/Ceremonies/Registration/PublicKeyCredentialCreationOptions.swift:

+ public init(challenge: [UInt8],user: PublicKeyCredentialUserEntity,relyingParty: PublicKeyCredentialRelyingPartyEntity,publicKeyCredentialParameters: [PublicKeyCredentialParameters],

+ timeout: Duration?,attestation: AttestationConveyancePreference) { nit: formatting ⬇️ Suggested change - public init(challenge: [UInt8],user: PublicKeyCredentialUserEntity,relyingParty: PublicKeyCredentialRelyingPartyEntity,publicKeyCredentialParameters: [PublicKeyCredentialParameters], - timeout: Duration?,attestation: AttestationConveyancePreference) { + public init( + challenge: [UInt8], + user: PublicKeyCredentialUserEntity,relyingParty: PublicKeyCredentialRelyingPartyEntity, + publicKeyCredentialParameters: [PublicKeyCredentialParameters], + timeout: Duration?, + attestation: AttestationConveyancePreference + ) { In Sources/WebAuthn/Ceremonies/Shared/CredentialPublicKey.swift:

+ //throw WebAuthnError.unsupported

+ let rsaSignature = _RSA.Signing.RSASignature(rawRepresentation: signature) + + var rsaPadding: _RSA.Signing.Padding + switch algorithm { + case .algRS1, .algRS256, .algRS384, .algRS512: + rsaPadding = .insecurePKCS1v1_5 + case .algPS256, .algPS384, .algPS512: + rsaPadding = .PSS + default: + throw WebAuthnError.unsupportedCOSEAlgorithmForRSAPublicKey + } + + guard try _RSA.Signing.PublicKey(derRepresentation: rawRepresentation).isValidSignature( + rsaSignature, + for: data, + padding: rsaPadding + ) else { + throw WebAuthnError.invalidSignature + } Could you please add tests for these? In Sources/WebAuthn/Helpers/ChallengeGenerator.swift:

-package struct ChallengeGenerator: Sendable {

- var generate: @sendable () -> [UInt8] +public protocol ChallengeGenerator : Sendable { + func generate() -> [UInt8] +} - package static var live: Self { - .init(generate: { [UInt8].random(count: 32) }) +public struct DefaultChallengeGenerator: Sendable, ChallengeGenerator { + public func generate() -> [UInt8] { + return [UInt8].random(count: 32) } + + public static let live = DefaultChallengeGenerator() I don't think I see the motivation for changing this. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

dscreve · 2025-01-27T17:04:05Z

Actually, RSA cannot be tested because CryptoKit does not allow creation of RSA key with size lower than 2048 bits...while WebAuthn use 325,384 and 512 bits.

dscreve added 4 commits January 22, 2025 16:23

Make Challenge more flexible (external ChallengeGenerator, and custom…

a84410b

… challenge for authentication and registration) Implement Codable in most object Add public init on most object Implements RSA

Add OpenAPI support

96e4c00

Codable fix

d2ab6d5

Tested and stable release

53d4f77

dimitribouniol requested changes Jan 26, 2025

View reviewed changes

Add RSA tests

6d4bc8f

dscreve closed this Jan 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RSA support and modularity add-ons #87

RSA support and modularity add-ons #87

Uh oh!

dscreve commented Jan 25, 2025

Uh oh!

dimitribouniol left a comment

Uh oh!

dimitribouniol Jan 26, 2025

Uh oh!

dimitribouniol Jan 26, 2025

Uh oh!

dimitribouniol Jan 26, 2025

Uh oh!

dimitribouniol Jan 26, 2025

Uh oh!

dimitribouniol Jan 26, 2025

Uh oh!

dimitribouniol Jan 26, 2025

Uh oh!

dscreve Jan 27, 2025

Uh oh!

dimitribouniol Jan 27, 2025

Uh oh!

dscreve Jan 27, 2025

Uh oh!

dimitribouniol Jan 26, 2025

Uh oh!

dscreve commented Jan 26, 2025 via email

Uh oh!

dscreve commented Jan 27, 2025

Uh oh!

Uh oh!

		public init(challenge: [UInt8],user: PublicKeyCredentialUserEntity,relyingParty: PublicKeyCredentialRelyingPartyEntity,publicKeyCredentialParameters: [PublicKeyCredentialParameters],
		timeout: Duration?,attestation: AttestationConveyancePreference) {

RSA support and modularity add-ons #87

RSA support and modularity add-ons #87

Uh oh!

Conversation

dscreve commented Jan 25, 2025

Uh oh!

dimitribouniol 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!

dscreve commented Jan 26, 2025 via email

Uh oh!

dscreve commented Jan 27, 2025

Uh oh!

Uh oh!