From 4103fdba04d18857e3e68f5d9bf7a52a5161188d Mon Sep 17 00:00:00 2001
From: Nedithgar Amirka <150447520+nedithgar@users.noreply.github.com>
Date: Sat, 2 Aug 2025 11:07:24 +0800
Subject: [PATCH 1/4] refactor: remove SSH key generation section and clarify
 OpenSSH key parsing

---
 README.md | 41 +----------------------------------------
 1 file changed, 1 insertion(+), 40 deletions(-)

diff --git a/README.md b/README.md
index 7dd7d35..f6eab39 100644
--- a/README.md
+++ b/README.md
@@ -379,46 +379,7 @@ When you implement SFTP in Citadel, you're responsible for taking care of logist
 
 ## Helpers
 
-### SSH Key Generation
-
-Citadel provides a high-level API for generating SSH key pairs programmatically:
-
-```swift
-// Generate Ed25519 key pair (recommended for most cases)
-let keyPair = SSHKeyGenerator.generateEd25519()
-
-// Generate RSA key pair
-let rsaKeyPair = SSHKeyGenerator.generateRSA(bits: 4096)
-
-// Generate ECDSA key pair
-let ecdsaKeyPair = SSHKeyGenerator.generateECDSA(curve: .p256)
-
-// Export keys in various formats
-
-/// OpenSSH format
-let privateKeyString = try keyPair.privateKeyOpenSSHString(comment: "user@example.com")
-let publicKeyString = try keyPair.publicKeyOpenSSHString() // ssh-ed25519 AAAA...
-
-/// PEM format
-let publicKeyPEM = try keyPair.publicKeyPEMString()
-let privateKeyPEM = try keyPair.privateKeyPEMString()
-
-// Export with passphrase protection
-let encryptedKey = try keyPair.privateKeyOpenSSHString(
-    comment: "user@example.com",
-    passphrase: "secure_passphrase",
-    cipher: "aes256-ctr"  // Supported: "none", "aes128-ctr", "aes256-ctr"
-)
-
-// Save keys to files
-try privateKeyString.write(toFile: "~/.ssh/id_ed25519", atomically: true, encoding: .utf8)
-try publicKeyString.write(toFile: "~/.ssh/id_ed25519.pub", atomically: true, encoding: .utf8)
-```
-
-### OpenSSH Key Parsing
-
-We support extensions on PrivateKey types such as our own `Insecure.RSA.PrivateKey`, as well as existing SwiftCrypto types like `Curve25519.Signing.PrivateKey`:
-
+The most important helper most people need is OpenSSH key parsing. We support extensions on PrivateKey types such as our own `Insecure.RSA.PrivateKey`, as well as existing SwiftCrypto types like `Curve25519.Signing.PrivateKey`:
 ```swift
 // Parse an OpenSSH RSA private key. This is the same format as the one used by OpenSSH
 let sshFile = try String(contentsOf: ..)

From d422f11dedee7551ba48ee1423df81a05e762d59 Mon Sep 17 00:00:00 2001
From: Nedithgar Amirka <150447520+nedithgar@users.noreply.github.com>
Date: Sat, 2 Aug 2025 11:13:37 +0800
Subject: [PATCH 2/4] refactor: remove error print statements from certificate
 tests

---
 .../CertificateAuthenticationMethodRealTests.swift       | 2 --
 Tests/CitadelTests/ECDSACertificateRealTests.swift       | 2 --
 Tests/CitadelTests/KeyTests.swift                        | 9 ---------
 Tests/CitadelTests/SSHCertificateRealTests.swift         | 2 --
 4 files changed, 15 deletions(-)

diff --git a/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift b/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift
index 1fbc8d2..de693d0 100644
--- a/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift
+++ b/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift
@@ -13,7 +13,6 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
             try SSHCertificateGenerator.ensureSSHKeygenAvailable()
             try SSHCertificateGenerator.setUp()
         } catch {
-            print("Failed to set up certificate generation: \(error)")
         }
     }
     
@@ -23,7 +22,6 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
         do {
             try TestCertificateHelper.cleanUp()
         } catch {
-            print("Failed to clean up certificates: \(error)")
         }
     }
     
diff --git a/Tests/CitadelTests/ECDSACertificateRealTests.swift b/Tests/CitadelTests/ECDSACertificateRealTests.swift
index 07417d8..5344467 100644
--- a/Tests/CitadelTests/ECDSACertificateRealTests.swift
+++ b/Tests/CitadelTests/ECDSACertificateRealTests.swift
@@ -15,7 +15,6 @@ final class ECDSACertificateRealTests: XCTestCase {
             try SSHCertificateGenerator.ensureSSHKeygenAvailable()
             try SSHCertificateGenerator.setUp()
         } catch {
-            print("Failed to set up certificate generation: \(error)")
         }
     }
     
@@ -25,7 +24,6 @@ final class ECDSACertificateRealTests: XCTestCase {
         do {
             try TestCertificateHelper.cleanUp()
         } catch {
-            print("Failed to clean up certificates: \(error)")
         }
     }
     
diff --git a/Tests/CitadelTests/KeyTests.swift b/Tests/CitadelTests/KeyTests.swift
index 4868cba..0750e7d 100644
--- a/Tests/CitadelTests/KeyTests.swift
+++ b/Tests/CitadelTests/KeyTests.swift
@@ -355,8 +355,6 @@ final class KeyTests: XCTestCase {
         // Verify we can read it back
         let p256Parsed = try P256.Signing.PrivateKey(sshECDSA: p256SSH)
         // Check if public keys match
-        print("Original P256 public key: \(p256Key.publicKey.x963Representation.base64EncodedString())")
-        print("Parsed P256 public key: \(p256Parsed.publicKey.x963Representation.base64EncodedString())")
         XCTAssertEqual(p256Key.publicKey.x963Representation, p256Parsed.publicKey.x963Representation)
         
         // Test ECDSA P-384 key generation and export
@@ -408,10 +406,6 @@ final class KeyTests: XCTestCase {
             passphrase: passphrase
         )
         
-        // Debug: print the generated key
-        print("Generated encrypted key:")
-        print(ed25519Encrypted)
-        
         // Should contain encryption markers in the base64 content, not the PEM wrapper
         let lines = ed25519Encrypted.split(separator: "\n")
         if lines.count > 2 {
@@ -419,9 +413,6 @@ final class KeyTests: XCTestCase {
             if let decodedData = Data(base64Encoded: base64Content) {
                 // The decoded data starts with openssh-key-v1\0 and contains cipher and kdf strings
                 let decodedString = String(decoding: decodedData, as: UTF8.self)
-                print("Decoded binary contains openssh-key-v1: \(decodedString.contains("openssh-key-v1"))")
-                print("Decoded binary contains aes256-ctr: \(decodedString.contains("aes256-ctr"))")
-                print("Decoded binary contains bcrypt: \(decodedString.contains("bcrypt"))")
                 XCTAssertTrue(decodedString.contains("aes256-ctr") || decodedString.contains("aes128-ctr"))
                 XCTAssertTrue(decodedString.contains("bcrypt"))
             } else {
diff --git a/Tests/CitadelTests/SSHCertificateRealTests.swift b/Tests/CitadelTests/SSHCertificateRealTests.swift
index d2ea2d6..f2e91bf 100644
--- a/Tests/CitadelTests/SSHCertificateRealTests.swift
+++ b/Tests/CitadelTests/SSHCertificateRealTests.swift
@@ -13,7 +13,6 @@ final class SSHCertificateRealTests: XCTestCase {
             try SSHCertificateGenerator.ensureSSHKeygenAvailable()
             try SSHCertificateGenerator.setUp()
         } catch {
-            print("Failed to set up certificate generation: \(error)")
         }
     }
     
@@ -23,7 +22,6 @@ final class SSHCertificateRealTests: XCTestCase {
         do {
             try TestCertificateHelper.cleanUp()
         } catch {
-            print("Failed to clean up certificates: \(error)")
         }
     }
     

From 4b125dac22bf86e203a0fb0e87f6200ae229a8e3 Mon Sep 17 00:00:00 2001
From: Nedithgar Amirka <150447520+nedithgar@users.noreply.github.com>
Date: Sat, 2 Aug 2025 11:24:41 +0800
Subject: [PATCH 3/4] refactor: enhance error handling and setup verification
 in certificate tests

---
 ...ificateAuthenticationMethodRealTests.swift | 33 +++++++++++++++++++
 .../ECDSACertificateRealTests.swift           | 20 +++++++++++
 .../SSHCertificateGenerator.swift             |  4 +++
 .../SSHCertificateRealTests.swift             | 20 +++++++++++
 4 files changed, 77 insertions(+)

diff --git a/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift b/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift
index de693d0..c99b5e1 100644
--- a/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift
+++ b/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift
@@ -13,6 +13,7 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
             try SSHCertificateGenerator.ensureSSHKeygenAvailable()
             try SSHCertificateGenerator.setUp()
         } catch {
+            XCTFail("Certificate generation setup failed: \(error)")
         }
     }
     
@@ -22,12 +23,24 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
         do {
             try TestCertificateHelper.cleanUp()
         } catch {
+            XCTFail("Certificate cleanup failed: \(error)")
+        }
+    }
+    
+    // MARK: - Helper Methods
+    
+    /// Check if certificate setup was successful, fail test if not
+    private func requireCertificateSetup() {
+        guard SSHCertificateGenerator.isSetupSuccessful else {
+            XCTFail("Certificate generation setup failed - ssh-keygen may not be available")
         }
     }
     
     // MARK: - Ed25519 Certificate Tests
     
     func testEd25519CertificateWithValidCertificate() throws {
+        requireCertificateSetup()
+        
         let (privateKey, certificate) = try TestCertificateHelper.parseEd25519Certificate(
             certificateFile: "user_ed25519-cert.pub",
             privateKeyFile: "user_ed25519"
@@ -56,6 +69,8 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     }
     
     func testEd25519CertificateWithExpiredCertificate() throws {
+        requireCertificateSetup()
+        
         // SKIP TEST: Time-based validation tests require certificates with specific validity periods
         // The test certificates are generated with 1 hour validity and may have been regenerated
         // making this test unreliable. The time validation logic is tested in CertificateSecurityValidationTests
@@ -63,6 +78,8 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     }
     
     func testEd25519CertificateWithWrongPrincipal() throws {
+        requireCertificateSetup()
+        
         // Generate a certificate with limited principals
         let certificate = try TestCertificateHelper.generateLimitedPrincipalsCertificate()
         
@@ -85,6 +102,8 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - P256 Certificate Tests
     
     func testP256CertificateValidation() throws {
+        requireCertificateSetup()
+        
         let (privateKey, certificate) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
             privateKeyFile: "user_ecdsa_p256"
@@ -115,6 +134,8 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - RSA Certificate Tests
     
     func testRSACertificateValidation() throws {
+        requireCertificateSetup()
+        
         // SKIP TEST: RSA certificates are not supported by NIOSSH
         // While Citadel can parse and validate RSA certificates correctly,
         // NIOSSH (the underlying SSH library) does not support RSA certificates
@@ -144,6 +165,8 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     }
     
     func testRSACertificateWithHostType() throws {
+        requireCertificateSetup()
+        
         // SKIP TEST: Certificate type validation is not enforced in user authentication
         // The current implementation only validates certificate type when checking
         // principals (username for user certs, hostname for host certs).
@@ -186,6 +209,8 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - P384 Certificate Tests
     
     func testP384CertificateWithMultiplePrincipals() throws {
+        requireCertificateSetup()
+        
         let (privateKey, certificate) = try TestCertificateHelper.parseP384Certificate(
             certificateFile: "user_ecdsa_p384-cert.pub",
             privateKeyFile: "user_ecdsa_p384"
@@ -212,6 +237,8 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - P521 Certificate Tests
     
     func testP521CertificateValidation() throws {
+        requireCertificateSetup()
+        
         let (privateKey, certificate) = try TestCertificateHelper.parseP521Certificate(
             certificateFile: "user_ecdsa_p521-cert.pub",
             privateKeyFile: "user_ecdsa_p521"
@@ -229,6 +256,8 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - Time-based Certificate Tests
     
     func testNotYetValidCertificate() throws {
+        requireCertificateSetup()
+        
         // SKIP TEST: Time-based validation tests require certificates with specific validity periods
         // The test certificates are generated with specific future timestamps that may not be reliable
         // The time validation logic is tested in CertificateSecurityValidationTests
@@ -238,6 +267,8 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - Critical Options Tests
     
     func testCertificateWithCriticalOptions() throws {
+        requireCertificateSetup()
+        
         // Generate a new Ed25519 private key for this test
         let privateKey = Curve25519.Signing.PrivateKey()
         
@@ -262,6 +293,8 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - Extensions Tests
     
     func testCertificateWithAllExtensions() throws {
+        requireCertificateSetup()
+        
         // Generate a new Ed25519 private key for this test
         let privateKey = Curve25519.Signing.PrivateKey()
         
diff --git a/Tests/CitadelTests/ECDSACertificateRealTests.swift b/Tests/CitadelTests/ECDSACertificateRealTests.swift
index 5344467..393d23b 100644
--- a/Tests/CitadelTests/ECDSACertificateRealTests.swift
+++ b/Tests/CitadelTests/ECDSACertificateRealTests.swift
@@ -15,6 +15,7 @@ final class ECDSACertificateRealTests: XCTestCase {
             try SSHCertificateGenerator.ensureSSHKeygenAvailable()
             try SSHCertificateGenerator.setUp()
         } catch {
+            XCTFail("Certificate generation setup failed: \(error)")
         }
     }
     
@@ -24,12 +25,23 @@ final class ECDSACertificateRealTests: XCTestCase {
         do {
             try TestCertificateHelper.cleanUp()
         } catch {
+            XCTFail("Certificate cleanup failed: \(error)")
+        }
+    }
+    
+    // MARK: - Helper Methods
+    
+    /// Check if certificate setup was successful, fail test if not
+    private func requireCertificateSetup() {
+        guard SSHCertificateGenerator.isSetupSuccessful else {
+            XCTFail("Certificate generation setup failed - ssh-keygen may not be available")
         }
     }
     
     // MARK: - P256 Certificate Tests
     
     func testP256CertificateParsingWithRealCertificate() throws {
+        requireCertificateSetup()
         let (privateKey, certificate) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
             privateKeyFile: "user_ecdsa_p256"
@@ -48,6 +60,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     }
     
     func testP256CertificateValidation() throws {
+        requireCertificateSetup()
         // Principal validation with fresh certificates
         let (_, certificate) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
@@ -77,6 +90,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     // MARK: - P384 Certificate Tests
     
     func testP384CertificateParsingWithRealCertificate() throws {
+        requireCertificateSetup()
         let (privateKey, certificate) = try TestCertificateHelper.parseP384Certificate(
             certificateFile: "user_ecdsa_p384-cert.pub",
             privateKeyFile: "user_ecdsa_p384"
@@ -95,6 +109,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     }
     
     func testP384CertificateMultiplePrincipals() throws {
+        requireCertificateSetup()
         let (_, certificate) = try TestCertificateHelper.parseP384Certificate(
             certificateFile: "user_ecdsa_p384-cert.pub",
             privateKeyFile: "user_ecdsa_p384"
@@ -127,6 +142,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     // MARK: - P521 Certificate Tests
     
     func testP521CertificateParsingWithRealCertificate() throws {
+        requireCertificateSetup()
         let (privateKey, certificate) = try TestCertificateHelper.parseP521Certificate(
             certificateFile: "user_ecdsa_p521-cert.pub",
             privateKeyFile: "user_ecdsa_p521"
@@ -147,6 +163,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     // MARK: - Certificate Equality Tests
     
     func testCertificateEqualityWithRealCertificates() throws {
+        requireCertificateSetup()
         // Generate two P256 certificates with the same configuration
         let (_, cert1) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
@@ -178,6 +195,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     // MARK: - Invalid Certificate Tests
     
     func testInvalidCertificateData() throws {
+        requireCertificateSetup()
         // Test with completely invalid data
         let invalidData = Data("This is not a certificate".utf8)
         XCTAssertThrowsError(try NIOSSHCertificateLoader.loadFromBinaryData(invalidData)) { error in
@@ -195,6 +213,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     }
     
     func testCertificateTimeValidation() throws {
+        requireCertificateSetup()
         // Generate a certificate with known validity period
         let (_, certificate) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
@@ -219,6 +238,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     // MARK: - Key Size Tests
     
     func testAllCurveSizes() throws {
+        requireCertificateSetup()
         // Test that the public key sizes are correct for each curve
         let (_, p256Cert) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
diff --git a/Tests/CitadelTests/SSHCertificateGenerator.swift b/Tests/CitadelTests/SSHCertificateGenerator.swift
index 1361fbd..4096c9a 100644
--- a/Tests/CitadelTests/SSHCertificateGenerator.swift
+++ b/Tests/CitadelTests/SSHCertificateGenerator.swift
@@ -4,6 +4,9 @@ import XCTest
 /// Helper class to generate SSH certificates dynamically during test runs
 enum SSHCertificateGenerator {
     
+    /// Track whether setup was successful
+    static var isSetupSuccessful = false
+    
     /// Temporary directory for generated certificates
     static var tempDirectory: URL {
         FileManager.default.temporaryDirectory.appendingPathComponent("CitadelTestCerts-\(ProcessInfo.processInfo.processIdentifier)")
@@ -12,6 +15,7 @@ enum SSHCertificateGenerator {
     /// Setup the temporary directory
     static func setUp() throws {
         try FileManager.default.createDirectory(at: tempDirectory, withIntermediateDirectories: true)
+        isSetupSuccessful = true
     }
     
     /// Clean up the temporary directory
diff --git a/Tests/CitadelTests/SSHCertificateRealTests.swift b/Tests/CitadelTests/SSHCertificateRealTests.swift
index f2e91bf..5054d12 100644
--- a/Tests/CitadelTests/SSHCertificateRealTests.swift
+++ b/Tests/CitadelTests/SSHCertificateRealTests.swift
@@ -13,6 +13,7 @@ final class SSHCertificateRealTests: XCTestCase {
             try SSHCertificateGenerator.ensureSSHKeygenAvailable()
             try SSHCertificateGenerator.setUp()
         } catch {
+            XCTFail("Certificate generation setup failed: \(error)")
         }
     }
     
@@ -22,12 +23,23 @@ final class SSHCertificateRealTests: XCTestCase {
         do {
             try TestCertificateHelper.cleanUp()
         } catch {
+            XCTFail("Certificate cleanup failed: \(error)")
+        }
+    }
+    
+    // MARK: - Helper Methods
+    
+    /// Check if certificate setup was successful, fail test if not
+    private func requireCertificateSetup() {
+        guard SSHCertificateGenerator.isSetupSuccessful else {
+            XCTFail("Certificate generation setup failed - ssh-keygen may not be available")
         }
     }
     
     // MARK: - Basic Certificate Parsing Tests
     
     func testEd25519CertificateParsing() throws {
+        requireCertificateSetup()
         let (_, certificate) = try TestCertificateHelper.parseEd25519Certificate(
             certificateFile: "user_ed25519-cert.pub",
             privateKeyFile: "user_ed25519"
@@ -44,6 +56,7 @@ final class SSHCertificateRealTests: XCTestCase {
     }
     
     func testP256CertificateParsing() throws {
+        requireCertificateSetup()
         let (_, certificate) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
             privateKeyFile: "user_ecdsa_p256"
@@ -59,6 +72,7 @@ final class SSHCertificateRealTests: XCTestCase {
     }
     
     func testP384CertificateParsing() throws {
+        requireCertificateSetup()
         let (_, certificate) = try TestCertificateHelper.parseP384Certificate(
             certificateFile: "user_ecdsa_p384-cert.pub",
             privateKeyFile: "user_ecdsa_p384"
@@ -74,6 +88,7 @@ final class SSHCertificateRealTests: XCTestCase {
     }
     
     func testP521CertificateParsing() throws {
+        requireCertificateSetup()
         let (_, certificate) = try TestCertificateHelper.parseP521Certificate(
             certificateFile: "user_ecdsa_p521-cert.pub",
             privateKeyFile: "user_ecdsa_p521"
@@ -92,6 +107,7 @@ final class SSHCertificateRealTests: XCTestCase {
     // MARK: - Host Certificate Tests
     
     func testHostCertificateParsing() throws {
+        requireCertificateSetup()
         let certificate = try TestCertificateHelper.generateHostCertificate()
         
         XCTAssertEqual(certificate.keyID, "test-host")
@@ -121,6 +137,7 @@ final class SSHCertificateRealTests: XCTestCase {
     // MARK: - Critical Options Tests
     
     func testCriticalOptions() throws {
+        requireCertificateSetup()
         let certificate = try TestCertificateHelper.generateCriticalOptionsCertificate()
         
         XCTAssertEqual(certificate.keyID, "restricted-cert")
@@ -145,6 +162,7 @@ final class SSHCertificateRealTests: XCTestCase {
     // MARK: - Principal Validation Tests
     
     func testLimitedPrincipals() throws {
+        requireCertificateSetup()
         let certificate = try TestCertificateHelper.generateLimitedPrincipalsCertificate()
         
         XCTAssertEqual(certificate.keyID, "limited-cert")
@@ -178,6 +196,7 @@ final class SSHCertificateRealTests: XCTestCase {
     // MARK: - Extensions Tests
     
     func testAllExtensions() throws {
+        requireCertificateSetup()
         let certificate = try TestCertificateHelper.generateAllExtensionsCertificate()
         
         XCTAssertEqual(certificate.keyID, "full-cert")
@@ -194,6 +213,7 @@ final class SSHCertificateRealTests: XCTestCase {
     // MARK: - Authentication Method Tests
     
     func testCertificateAuthenticationMethods() throws {
+        requireCertificateSetup()
         // Test certificate authentication with fresh certificates
         let (privateKey, certificate) = try TestCertificateHelper.parseEd25519Certificate(
             certificateFile: "user_ed25519-cert.pub",

From 40697da14a32603f851aed3a9ae312351e701c72 Mon Sep 17 00:00:00 2001
From: Nedithgar Amirka <150447520+nedithgar@users.noreply.github.com>
Date: Sat, 2 Aug 2025 11:33:35 +0800
Subject: [PATCH 4/4] refactor: improve SSH certificate setup handling in tests

---
 ...ificateAuthenticationMethodRealTests.swift | 72 +++++++++----------
 .../ECDSACertificateRealTests.swift           | 66 ++++++++---------
 .../SSHCertificateGenerator.swift             |  6 ++
 .../SSHCertificateRealTests.swift             | 66 ++++++++---------
 4 files changed, 101 insertions(+), 109 deletions(-)

diff --git a/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift b/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift
index c99b5e1..8f3eca4 100644
--- a/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift
+++ b/Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift
@@ -6,18 +6,32 @@ import _CryptoExtras
 /// Tests for certificate authentication methods using real SSH certificates
 final class CertificateAuthenticationMethodRealTests: XCTestCase {
     
-    override class func setUp() {
+    override func setUp() {
         super.setUp()
-        // Generate certificates dynamically for tests
-        do {
-            try SSHCertificateGenerator.ensureSSHKeygenAvailable()
-            try SSHCertificateGenerator.setUp()
-        } catch {
-            XCTFail("Certificate generation setup failed: \(error)")
+        // Generate certificates dynamically for tests (only once)
+        if !SSHCertificateGenerator.hasAttemptedSetup {
+            SSHCertificateGenerator.hasAttemptedSetup = true
+            do {
+                try SSHCertificateGenerator.ensureSSHKeygenAvailable()
+                try SSHCertificateGenerator.setUp()
+                SSHCertificateGenerator.isSetupSuccessful = true
+            } catch {
+                SSHCertificateGenerator.setupError = error
+                SSHCertificateGenerator.isSetupSuccessful = false
+            }
+        }
+        
+        // Check setup success for each test
+        if !SSHCertificateGenerator.isSetupSuccessful {
+            if let error = SSHCertificateGenerator.setupError {
+                XCTFail("Certificate generation setup failed: \(error)")
+            } else {
+                XCTFail("Certificate generation setup failed")
+            }
         }
     }
     
-    override class func tearDown() {
+    override func tearDown() {
         super.tearDown()
         // Clean up generated certificates
         do {
@@ -27,20 +41,10 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
         }
     }
     
-    // MARK: - Helper Methods
-    
-    /// Check if certificate setup was successful, fail test if not
-    private func requireCertificateSetup() {
-        guard SSHCertificateGenerator.isSetupSuccessful else {
-            XCTFail("Certificate generation setup failed - ssh-keygen may not be available")
-        }
-    }
-    
     // MARK: - Ed25519 Certificate Tests
     
     func testEd25519CertificateWithValidCertificate() throws {
-        requireCertificateSetup()
-        
+                
         let (privateKey, certificate) = try TestCertificateHelper.parseEd25519Certificate(
             certificateFile: "user_ed25519-cert.pub",
             privateKeyFile: "user_ed25519"
@@ -69,8 +73,7 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     }
     
     func testEd25519CertificateWithExpiredCertificate() throws {
-        requireCertificateSetup()
-        
+                
         // SKIP TEST: Time-based validation tests require certificates with specific validity periods
         // The test certificates are generated with 1 hour validity and may have been regenerated
         // making this test unreliable. The time validation logic is tested in CertificateSecurityValidationTests
@@ -78,8 +81,7 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     }
     
     func testEd25519CertificateWithWrongPrincipal() throws {
-        requireCertificateSetup()
-        
+                
         // Generate a certificate with limited principals
         let certificate = try TestCertificateHelper.generateLimitedPrincipalsCertificate()
         
@@ -102,8 +104,7 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - P256 Certificate Tests
     
     func testP256CertificateValidation() throws {
-        requireCertificateSetup()
-        
+                
         let (privateKey, certificate) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
             privateKeyFile: "user_ecdsa_p256"
@@ -134,8 +135,7 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - RSA Certificate Tests
     
     func testRSACertificateValidation() throws {
-        requireCertificateSetup()
-        
+                
         // SKIP TEST: RSA certificates are not supported by NIOSSH
         // While Citadel can parse and validate RSA certificates correctly,
         // NIOSSH (the underlying SSH library) does not support RSA certificates
@@ -165,8 +165,7 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     }
     
     func testRSACertificateWithHostType() throws {
-        requireCertificateSetup()
-        
+                
         // SKIP TEST: Certificate type validation is not enforced in user authentication
         // The current implementation only validates certificate type when checking
         // principals (username for user certs, hostname for host certs).
@@ -209,8 +208,7 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - P384 Certificate Tests
     
     func testP384CertificateWithMultiplePrincipals() throws {
-        requireCertificateSetup()
-        
+                
         let (privateKey, certificate) = try TestCertificateHelper.parseP384Certificate(
             certificateFile: "user_ecdsa_p384-cert.pub",
             privateKeyFile: "user_ecdsa_p384"
@@ -237,8 +235,7 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - P521 Certificate Tests
     
     func testP521CertificateValidation() throws {
-        requireCertificateSetup()
-        
+                
         let (privateKey, certificate) = try TestCertificateHelper.parseP521Certificate(
             certificateFile: "user_ecdsa_p521-cert.pub",
             privateKeyFile: "user_ecdsa_p521"
@@ -256,8 +253,7 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - Time-based Certificate Tests
     
     func testNotYetValidCertificate() throws {
-        requireCertificateSetup()
-        
+                
         // SKIP TEST: Time-based validation tests require certificates with specific validity periods
         // The test certificates are generated with specific future timestamps that may not be reliable
         // The time validation logic is tested in CertificateSecurityValidationTests
@@ -267,8 +263,7 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - Critical Options Tests
     
     func testCertificateWithCriticalOptions() throws {
-        requireCertificateSetup()
-        
+                
         // Generate a new Ed25519 private key for this test
         let privateKey = Curve25519.Signing.PrivateKey()
         
@@ -293,8 +288,7 @@ final class CertificateAuthenticationMethodRealTests: XCTestCase {
     // MARK: - Extensions Tests
     
     func testCertificateWithAllExtensions() throws {
-        requireCertificateSetup()
-        
+                
         // Generate a new Ed25519 private key for this test
         let privateKey = Curve25519.Signing.PrivateKey()
         
diff --git a/Tests/CitadelTests/ECDSACertificateRealTests.swift b/Tests/CitadelTests/ECDSACertificateRealTests.swift
index 393d23b..ebd3e41 100644
--- a/Tests/CitadelTests/ECDSACertificateRealTests.swift
+++ b/Tests/CitadelTests/ECDSACertificateRealTests.swift
@@ -8,18 +8,32 @@ import NIOSSH
 /// Tests for ECDSA certificates using real certificates generated by ssh-keygen
 final class ECDSACertificateRealTests: XCTestCase {
     
-    override class func setUp() {
+    override func setUp() {
         super.setUp()
-        // Generate certificates dynamically for tests
-        do {
-            try SSHCertificateGenerator.ensureSSHKeygenAvailable()
-            try SSHCertificateGenerator.setUp()
-        } catch {
-            XCTFail("Certificate generation setup failed: \(error)")
+        // Generate certificates dynamically for tests (only once)
+        if !SSHCertificateGenerator.hasAttemptedSetup {
+            SSHCertificateGenerator.hasAttemptedSetup = true
+            do {
+                try SSHCertificateGenerator.ensureSSHKeygenAvailable()
+                try SSHCertificateGenerator.setUp()
+                SSHCertificateGenerator.isSetupSuccessful = true
+            } catch {
+                SSHCertificateGenerator.setupError = error
+                SSHCertificateGenerator.isSetupSuccessful = false
+            }
+        }
+        
+        // Check setup success for each test
+        if !SSHCertificateGenerator.isSetupSuccessful {
+            if let error = SSHCertificateGenerator.setupError {
+                XCTFail("Certificate generation setup failed: \(error)")
+            } else {
+                XCTFail("Certificate generation setup failed")
+            }
         }
     }
     
-    override class func tearDown() {
+    override func tearDown() {
         super.tearDown()
         // Clean up generated certificates
         do {
@@ -29,20 +43,10 @@ final class ECDSACertificateRealTests: XCTestCase {
         }
     }
     
-    // MARK: - Helper Methods
-    
-    /// Check if certificate setup was successful, fail test if not
-    private func requireCertificateSetup() {
-        guard SSHCertificateGenerator.isSetupSuccessful else {
-            XCTFail("Certificate generation setup failed - ssh-keygen may not be available")
-        }
-    }
-    
     // MARK: - P256 Certificate Tests
     
     func testP256CertificateParsingWithRealCertificate() throws {
-        requireCertificateSetup()
-        let (privateKey, certificate) = try TestCertificateHelper.parseP256Certificate(
+                let (privateKey, certificate) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
             privateKeyFile: "user_ecdsa_p256"
         )
@@ -60,8 +64,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     }
     
     func testP256CertificateValidation() throws {
-        requireCertificateSetup()
-        // Principal validation with fresh certificates
+                // Principal validation with fresh certificates
         let (_, certificate) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
             privateKeyFile: "user_ecdsa_p256"
@@ -90,8 +93,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     // MARK: - P384 Certificate Tests
     
     func testP384CertificateParsingWithRealCertificate() throws {
-        requireCertificateSetup()
-        let (privateKey, certificate) = try TestCertificateHelper.parseP384Certificate(
+                let (privateKey, certificate) = try TestCertificateHelper.parseP384Certificate(
             certificateFile: "user_ecdsa_p384-cert.pub",
             privateKeyFile: "user_ecdsa_p384"
         )
@@ -109,8 +111,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     }
     
     func testP384CertificateMultiplePrincipals() throws {
-        requireCertificateSetup()
-        let (_, certificate) = try TestCertificateHelper.parseP384Certificate(
+                let (_, certificate) = try TestCertificateHelper.parseP384Certificate(
             certificateFile: "user_ecdsa_p384-cert.pub",
             privateKeyFile: "user_ecdsa_p384"
         )
@@ -142,8 +143,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     // MARK: - P521 Certificate Tests
     
     func testP521CertificateParsingWithRealCertificate() throws {
-        requireCertificateSetup()
-        let (privateKey, certificate) = try TestCertificateHelper.parseP521Certificate(
+                let (privateKey, certificate) = try TestCertificateHelper.parseP521Certificate(
             certificateFile: "user_ecdsa_p521-cert.pub",
             privateKeyFile: "user_ecdsa_p521"
         )
@@ -163,8 +163,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     // MARK: - Certificate Equality Tests
     
     func testCertificateEqualityWithRealCertificates() throws {
-        requireCertificateSetup()
-        // Generate two P256 certificates with the same configuration
+                // Generate two P256 certificates with the same configuration
         let (_, cert1) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
             privateKeyFile: "user_ecdsa_p256"
@@ -195,8 +194,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     // MARK: - Invalid Certificate Tests
     
     func testInvalidCertificateData() throws {
-        requireCertificateSetup()
-        // Test with completely invalid data
+                // Test with completely invalid data
         let invalidData = Data("This is not a certificate".utf8)
         XCTAssertThrowsError(try NIOSSHCertificateLoader.loadFromBinaryData(invalidData)) { error in
             XCTAssertTrue(error is NIOSSHCertificateLoadingError)
@@ -213,8 +211,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     }
     
     func testCertificateTimeValidation() throws {
-        requireCertificateSetup()
-        // Generate a certificate with known validity period
+                // Generate a certificate with known validity period
         let (_, certificate) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
             privateKeyFile: "user_ecdsa_p256"
@@ -238,8 +235,7 @@ final class ECDSACertificateRealTests: XCTestCase {
     // MARK: - Key Size Tests
     
     func testAllCurveSizes() throws {
-        requireCertificateSetup()
-        // Test that the public key sizes are correct for each curve
+                // Test that the public key sizes are correct for each curve
         let (_, p256Cert) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
             privateKeyFile: "user_ecdsa_p256"
diff --git a/Tests/CitadelTests/SSHCertificateGenerator.swift b/Tests/CitadelTests/SSHCertificateGenerator.swift
index 4096c9a..25f32d9 100644
--- a/Tests/CitadelTests/SSHCertificateGenerator.swift
+++ b/Tests/CitadelTests/SSHCertificateGenerator.swift
@@ -7,6 +7,12 @@ enum SSHCertificateGenerator {
     /// Track whether setup was successful
     static var isSetupSuccessful = false
     
+    /// Track setup error if any
+    static var setupError: Error?
+    
+    /// Track whether setup has been attempted
+    static var hasAttemptedSetup = false
+    
     /// Temporary directory for generated certificates
     static var tempDirectory: URL {
         FileManager.default.temporaryDirectory.appendingPathComponent("CitadelTestCerts-\(ProcessInfo.processInfo.processIdentifier)")
diff --git a/Tests/CitadelTests/SSHCertificateRealTests.swift b/Tests/CitadelTests/SSHCertificateRealTests.swift
index 5054d12..6132e1f 100644
--- a/Tests/CitadelTests/SSHCertificateRealTests.swift
+++ b/Tests/CitadelTests/SSHCertificateRealTests.swift
@@ -6,18 +6,32 @@ import Crypto
 /// Tests using real SSH certificates generated by ssh-keygen
 final class SSHCertificateRealTests: XCTestCase {
     
-    override class func setUp() {
+    override func setUp() {
         super.setUp()
-        // Generate certificates dynamically for tests
-        do {
-            try SSHCertificateGenerator.ensureSSHKeygenAvailable()
-            try SSHCertificateGenerator.setUp()
-        } catch {
-            XCTFail("Certificate generation setup failed: \(error)")
+        // Generate certificates dynamically for tests (only once)
+        if !SSHCertificateGenerator.hasAttemptedSetup {
+            SSHCertificateGenerator.hasAttemptedSetup = true
+            do {
+                try SSHCertificateGenerator.ensureSSHKeygenAvailable()
+                try SSHCertificateGenerator.setUp()
+                SSHCertificateGenerator.isSetupSuccessful = true
+            } catch {
+                SSHCertificateGenerator.setupError = error
+                SSHCertificateGenerator.isSetupSuccessful = false
+            }
+        }
+        
+        // Check setup success for each test
+        if !SSHCertificateGenerator.isSetupSuccessful {
+            if let error = SSHCertificateGenerator.setupError {
+                XCTFail("Certificate generation setup failed: \(error)")
+            } else {
+                XCTFail("Certificate generation setup failed")
+            }
         }
     }
     
-    override class func tearDown() {
+    override func tearDown() {
         super.tearDown()
         // Clean up generated certificates
         do {
@@ -27,20 +41,10 @@ final class SSHCertificateRealTests: XCTestCase {
         }
     }
     
-    // MARK: - Helper Methods
-    
-    /// Check if certificate setup was successful, fail test if not
-    private func requireCertificateSetup() {
-        guard SSHCertificateGenerator.isSetupSuccessful else {
-            XCTFail("Certificate generation setup failed - ssh-keygen may not be available")
-        }
-    }
-    
     // MARK: - Basic Certificate Parsing Tests
     
     func testEd25519CertificateParsing() throws {
-        requireCertificateSetup()
-        let (_, certificate) = try TestCertificateHelper.parseEd25519Certificate(
+                let (_, certificate) = try TestCertificateHelper.parseEd25519Certificate(
             certificateFile: "user_ed25519-cert.pub",
             privateKeyFile: "user_ed25519"
         )
@@ -56,8 +60,7 @@ final class SSHCertificateRealTests: XCTestCase {
     }
     
     func testP256CertificateParsing() throws {
-        requireCertificateSetup()
-        let (_, certificate) = try TestCertificateHelper.parseP256Certificate(
+                let (_, certificate) = try TestCertificateHelper.parseP256Certificate(
             certificateFile: "user_ecdsa_p256-cert.pub",
             privateKeyFile: "user_ecdsa_p256"
         )
@@ -72,8 +75,7 @@ final class SSHCertificateRealTests: XCTestCase {
     }
     
     func testP384CertificateParsing() throws {
-        requireCertificateSetup()
-        let (_, certificate) = try TestCertificateHelper.parseP384Certificate(
+                let (_, certificate) = try TestCertificateHelper.parseP384Certificate(
             certificateFile: "user_ecdsa_p384-cert.pub",
             privateKeyFile: "user_ecdsa_p384"
         )
@@ -88,8 +90,7 @@ final class SSHCertificateRealTests: XCTestCase {
     }
     
     func testP521CertificateParsing() throws {
-        requireCertificateSetup()
-        let (_, certificate) = try TestCertificateHelper.parseP521Certificate(
+                let (_, certificate) = try TestCertificateHelper.parseP521Certificate(
             certificateFile: "user_ecdsa_p521-cert.pub",
             privateKeyFile: "user_ecdsa_p521"
         )
@@ -107,8 +108,7 @@ final class SSHCertificateRealTests: XCTestCase {
     // MARK: - Host Certificate Tests
     
     func testHostCertificateParsing() throws {
-        requireCertificateSetup()
-        let certificate = try TestCertificateHelper.generateHostCertificate()
+                let certificate = try TestCertificateHelper.generateHostCertificate()
         
         XCTAssertEqual(certificate.keyID, "test-host")
         XCTAssertEqual(certificate.serial, 100)
@@ -137,8 +137,7 @@ final class SSHCertificateRealTests: XCTestCase {
     // MARK: - Critical Options Tests
     
     func testCriticalOptions() throws {
-        requireCertificateSetup()
-        let certificate = try TestCertificateHelper.generateCriticalOptionsCertificate()
+                let certificate = try TestCertificateHelper.generateCriticalOptionsCertificate()
         
         XCTAssertEqual(certificate.keyID, "restricted-cert")
         XCTAssertEqual(certificate.serial, 202)
@@ -162,8 +161,7 @@ final class SSHCertificateRealTests: XCTestCase {
     // MARK: - Principal Validation Tests
     
     func testLimitedPrincipals() throws {
-        requireCertificateSetup()
-        let certificate = try TestCertificateHelper.generateLimitedPrincipalsCertificate()
+                let certificate = try TestCertificateHelper.generateLimitedPrincipalsCertificate()
         
         XCTAssertEqual(certificate.keyID, "limited-cert")
         XCTAssertEqual(certificate.serial, 203)
@@ -196,8 +194,7 @@ final class SSHCertificateRealTests: XCTestCase {
     // MARK: - Extensions Tests
     
     func testAllExtensions() throws {
-        requireCertificateSetup()
-        let certificate = try TestCertificateHelper.generateAllExtensionsCertificate()
+                let certificate = try TestCertificateHelper.generateAllExtensionsCertificate()
         
         XCTAssertEqual(certificate.keyID, "full-cert")
         XCTAssertEqual(certificate.serial, 204)
@@ -213,8 +210,7 @@ final class SSHCertificateRealTests: XCTestCase {
     // MARK: - Authentication Method Tests
     
     func testCertificateAuthenticationMethods() throws {
-        requireCertificateSetup()
-        // Test certificate authentication with fresh certificates
+                // Test certificate authentication with fresh certificates
         let (privateKey, certificate) = try TestCertificateHelper.parseEd25519Certificate(
             certificateFile: "user_ed25519-cert.pub",
             privateKeyFile: "user_ed25519"