Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors test code and documentation to improve clarity and maintainability. The changes remove unnecessary debug output from test files and streamline the README documentation.
Key changes include:
- Replaced print statements with proper XCTest assertions in test failure scenarios
- Enhanced test setup reliability with singleton-pattern setup tracking
- Simplified README documentation by removing SSH key generation examples
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/CitadelTests/SSHCertificateRealTests.swift | Converted class-level setup/teardown to instance-level with proper error handling via XCTFail |
| Tests/CitadelTests/SSHCertificateGenerator.swift | Added static properties to track setup state and prevent duplicate initialization |
| Tests/CitadelTests/KeyTests.swift | Removed debug print statements from key generation and parsing tests |
| Tests/CitadelTests/ECDSACertificateRealTests.swift | Same refactoring as SSHCertificateRealTests with improved error handling |
| Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift | Same refactoring pattern with additional whitespace formatting |
| README.md | Removed detailed SSH key generation examples, focusing on OpenSSH key parsing |
nedithgar
added a commit
that referenced
this pull request
Aug 2, 2025
…cluded) (#7) * Enhance RSA Key Management and OpenSSH Representation - Updated RSA.PrivateKey to include prime factors (p, q) and iqmp for improved performance and security. - Implemented CRT parameters calculation for RSA private keys. - Enhanced the OpenSSHPrivateKey protocol to support public key retrieval and optional wrapping in composite strings. - Added methods for reading and writing SSH formatted data, including handling of optional parameters. - Improved key generation and representation for various key types (Ed25519, ECDSA, RSA) with support for passphrase encryption. - Added comprehensive tests for key generation, representation, and encryption/decryption processes across different key types. * feat: add support for multiple RSA signature hash algorithms and corresponding tests * feat: add support for RSA certificate types and corresponding key detection tests * Add Ed25519 certificate support and related tests - Implemented Ed25519 certificate public key handling in Ed25519.swift. - Created SSHCertificate.swift to define the structure and parsing logic for SSH certificates. - Updated OpenSSHKey.swift and SSHKeyTypeDetection.swift to include Ed25519 certificate types. - Added Ed25519CertificateTests.swift to validate parsing, serialization, and equality of Ed25519 certificates. - Enhanced KeyTests.swift to include Ed25519 certificate type detection and descriptions. - Ensured compatibility with OpenSSH certificate format in tests. * feat: enhance RSA certificate handling and add backward compatibility tests * feat: add SSH key generation and export functionality with tests * feat: add passphrase protection and cipher options for private key export * refactor: remove unused authentication method and related tests from SSHKeyGenerator * feat: implement OpenSSH format export for RSA keys with support for comments, passphrases, and custom ciphers * feat: add PEM and DER support for RSA and Ed25519 keys - Implemented PEM representation for Insecure.RSA.PrivateKey and Curve25519.Signing.PrivateKey. - Added DER representation for Insecure.RSA.PrivateKey. - Enhanced SSHKeyGenerator to support PEM export for RSA and Ed25519 keys. - Created tests for PEM and DER round-trip conversions for Ed25519 and RSA keys. - Updated existing tests to reflect new PEM support in SSHKeyGenerator. * feat: add PEM export functionality for public keys in SSHKeyGenerator * feat: add SSH certificate authentication tests and utilities for multiple key types * feat: enhance certificate authentication tests with additional key types and validation * feat: implement certificate authentication tests for Ed25519, RSA, and ECDSA key types * feat: enhance SSH certificate handling for different key types and add real certificate tests * fix: replace macOS-specific Security framework with cross-platform random generation - Wrapped Security framework import with conditional compilation - Replaced SecRandomCopyBytes with Swift's built-in random API - Ensures RSA certificate functionality works on Linux in CI/CD * fix: completely remove Security framework dependency for cross-platform compatibility - Removed Security framework import from RSA.swift and OpenSSHKey.swift - Replaced all SecRandomCopyBytes calls with Swift's built-in UInt8.random(in:) - Ensures the library works on all platforms including Linux - All tests pass with the cross-platform implementation * fix: add 'any' keyword for OpenSSHPrivateKey protocol usage (Swift 6 compliance) - Added 'any' keyword to existential protocol type casts in SSHCert.swift - Fixes Swift 6 error: 'use of protocol as a type must be written any' * fix: remove upstream-citadel subproject reference * feat: add support for SSH certificate authentication methods and utilities * Update dependencies and enhance SSH certificate handling - Updated BigInt to version 5.7.0 and ColorizeSwift to version 1.7.0. - Added swift-nio-ssh package with version 0.3.5. - Updated swift-collections, swift-crypto, swift-log, swift-nio, and swift-system to their latest versions. - Enhanced ECDSACertificate, Ed25519, RSA, and other certificate handling to include original certificate data for serialization. - Refactored SSHAuthenticationMethod to support direct authentication without custom delegates. - Improved CertificateLoader to handle OpenSSH format certificates. - Added unit tests for new authentication methods and certificate handling. * Refactor/security (#8) * Add test certificate generation and loading utilities - Introduced `TestCertificateHelper` class for loading and parsing SSH certificates and private keys. - Implemented methods to parse Ed25519, P256, P384, P521, and RSA certificates. - Added a script `generate_test_certificates.sh` to automate the generation of test SSH certificates. - Created a `.gitignore` file for the test certificates directory to exclude private keys. - Added various test certificate files for different algorithms and scenarios, including expired and not yet valid certificates. * Update test certificates for various algorithms - Updated ECDSA P256 certificates with new public keys. - Updated ECDSA P384 certificates with new public keys. - Updated ECDSA P521 certificates with new public keys. - Updated Ed25519 certificates with new public keys. - Updated expired certificates with new public keys. - Updated limited principals certificates with new public keys. - Updated not yet valid certificates with new public keys. - Updated RSA certificates with new public keys. * feat: implement ASN.1 DER encoding for ECDSA signatures * feat: enhance SSH certificate constraints validation and error handling * feat: implement CA key verification and enhance certificate signature tests * Add CIDRMatcher for IPv4 and IPv6 support, implement PatternMatcher for wildcard matching - Enhanced CIDRMatcher to support both IPv4 and IPv6 address matching. - Introduced PatternMatcher for OpenSSH-compatible wildcard pattern matching. - Added comprehensive tests for CIDR matching, pattern matching, and validation. - Implemented user and hostname matching with support for negation and group patterns. - Validated CIDR lists and patterns to ensure compliance with expected formats. * feat: enhance principal validation to allow optional empty principals * feat: read nonce as the first field after key type in SSH certificate parsing * feat: add signature type extraction and validation for SSH certificates * fix: update original buffer handling for signature verification in SSHCertificate * feat: add RSA key length validation and tests for SSH certificates * feat: implement principal limit and no-touch-required extension handling in SSH certificates * Refactor SSH Certificate Tests to Use NIOSSH for Certificate Parsing - Updated RealCertificateTests to skip deprecated certificate parsing tests and utilize NIOSSH's native support for certificate handling. - Removed shell command execution for certificate generation and replaced with NIOSSHCertificateLoader for parsing certificates. - Adjusted TestCertificateHelper methods to return NIOSSHCertifiedPublicKey instead of specific key types. - Modified SSHCertificateRealTests to skip tests that rely on expired certificates and updated assertions to align with NIOSSH's structure. - Ensured all Ed25519, ECDSA, and RSA certificate parsing methods in TestCertificateHelper now utilize NIOSSH for consistency and reliability. * Remove test certificates and associated scripts - Deleted the .gitignore file for test certificates. - Removed public key files for various algorithms (ECDSA P256, P384, P521; Ed25519; RSA). - Deleted the script for generating test certificates. - Removed host and user certificate files, including those with special conditions (expired, not yet valid, limited principals). * refactor: make IP address validation cross-platform and improve code clarity - Remove dependency on Apple's Network framework for Linux compatibility - Implement custom IPv4/IPv6 parsing and validation using pure Swift - Replace magic numbers with named constants: - INET6_ADDRSTRLEN (46) for IPv6 address max length - MAX_CIDR_PREFIX_LENGTH (4) for CIDR notation - Test constants (MATCH, NO_MATCH, NEGATED_MATCH, ERROR) - Improve defensive programming with explicit switch cases for bit masking - Add comprehensive cross-platform IP validation tests - Support IPv6 short forms, IPv4-mapped addresses, and zone IDs This change enables the library to build and run on Linux and other non-Apple platforms while maintaining full SSH certificate validation functionality. * build: remove TestCertificates resource reference from Package.swift Remove the TestCertificates resource copy declaration as test certificates and associated scripts have been removed from the project. * refactor: improve IP address validation using getaddrinfo for robustness * refactor: remove deprecated certificate tests and update to NIOSSH integration * refactor: improve SSH certificate setup handling in tests (#9)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request includes updates to the
README.mddocumentation for SSH key handling and refactors several test files to remove unnecessary debug and error print statements, improving code clarity and reducing noise during test execution.Documentation Updates:
README.mdand streamlined the section to focus on OpenSSH key parsing as the primary helper functionality.Test Code Refactoring:
CertificateAuthenticationMethodRealTests,ECDSACertificateRealTests, andSSHCertificateRealTeststhat logged errors during setup and cleanup operations. [1] [2] [3] [4] [5] [6]KeyTestsrelated to key generation and parsing, focusing on assertions instead of logging intermediate values. [1] [2]