Merged
Conversation
- 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.
- 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.
…or 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.
…ing in SSH certificates
- 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.
- 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).
…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.
Remove the TestCertificates resource copy declaration as test certificates and associated scripts have been removed from the project.
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a comprehensive refactoring focused on simplifying certificate handling and improving security validation in the Citadel SSH library. The changes primarily migrate from custom certificate implementations to NIOSSH's built-in functionality while introducing new utility classes for pattern matching and address validation.
- Replaces custom certificate classes with NIOSSH native certificate support
- Adds comprehensive pattern matching utilities for OpenSSH-compatible validation
- Introduces helper classes for testing with dynamically generated certificates
Reviewed Changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/CitadelTests/TestCertificateHelper.swift | New helper class for loading and parsing real SSH certificates generated by ssh-keygen |
| Tests/CitadelTests/SSHCertificateRealTests.swift | Tests using real SSH certificates with NIOSSH's native certificate support |
| Tests/CitadelTests/SSHCertificateGenerator.swift | Utility for dynamically generating SSH certificates during test runs |
| Tests/CitadelTests/RealCertificateTests.swift | Refactored to skip deprecated tests and redirect to new implementations |
| Tests/CitadelTests/PatternMatcherTests.swift | Comprehensive tests for OpenSSH-compatible pattern matching functionality |
| Tests/CitadelTests/NonceFixTest.swift | Updated to skip deprecated certificate structure tests |
| Tests/CitadelTests/NIOSSHCertificateAuthTests.swift | Simplified to remove deprecated CertificateConverter usage |
| Tests/CitadelTests/KeyTests.swift | Updated RSA certificate key type tests to use new algorithm structure |
| Tests/CitadelTests/Ed25519CertificateTests.swift | Removed - functionality migrated to NIOSSH |
| Tests/CitadelTests/ECDSACertificateTests.swift | Removed - functionality migrated to NIOSSH |
| Tests/CitadelTests/ECDSACertificateRealTests.swift | New tests for ECDSA certificates using real certificate generation |
| Tests/CitadelTests/CrossPlatformIPTests.swift | Tests for cross-platform IP address validation and CIDR matching |
| Tests/CitadelTests/CertificateValidationTests.swift | Updated to skip deprecated certificate validation tests |
| Tests/CitadelTests/CertificateSecurityValidationTests.swift | Updated to skip deprecated security validation tests |
| Tests/CitadelTests/CertificateAuthenticationTests.swift | Simplified to skip deprecated certificate authentication tests |
| Tests/CitadelTests/CertificateAuthenticationMethodRealTests.swift | New comprehensive tests for certificate authentication using real certificates |
| Tests/CitadelTests/CertificateAuthenticationIntegrationTests.swift | Updated to skip mock certificate tests and use real certificates |
| Tests/CitadelTests/AddressValidatorTests.swift | Comprehensive tests for OpenSSH-compatible address validation |
| Sources/Citadel/Utilities/PatternMatcher.swift | New OpenSSH-compatible pattern matching implementation |
| Sources/Citadel/Utilities/CIDRMatcher.swift | New CIDR matching utility supporting IPv4 and IPv6 |
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 introduces several changes to the Citadel codebase, focusing on simplifying certificate handling, improving error handling, and enhancing
ByteBufferutilities. Key updates include the removal of custom certificate-related classes for Ed25519 and RSA in favor of NIOSSH's built-in functionality, the addition of a newRSAErrorenum, and the introduction of Citadel-specificByteBufferextensions for SSH-related operations.Simplification of Certificate Handling:
CertificatePublicKeyclass for Ed25519 certificates, as NIOSSH now provides built-in support for handling Ed25519-certified public keys.CertificatePublicKeyclass for RSA certificates, consolidating functionality and relying on NIOSSH for certificate-related operations.Improved Error Handling:
RSAErrorenum to provide more descriptive and structured error messages for RSA-related operations.Enhancements to
ByteBufferUtilities:ByteBufferextensions to complement NIOSSH, including methods for reading and writing SSH strings, data, bignums, and buffers. These utilities improve compatibility and streamline SSH-related operations.Resource Management:
resourcessection in thePackage.swiftfile to includeTestCertificates, ensuring test resources are properly bundled.