Skip to content

Conversation

@nedithgar
Copy link
Contributor

This pull request introduces a comprehensive SSH key type detection feature in the Citadel module, along with extensive unit tests to ensure the reliability and correctness of the implementation. The key changes include the addition of an SSHKeyDetection utility for detecting SSH key types, new error handling for invalid or unsupported key formats, and test cases to validate both public and private key detection.

SSH Key Type Detection Feature:

  • Sources/Citadel/SSHKeyTypeDetection.swift: Added the SSHKeyDetection utility to detect SSH key types from their string representations. This includes support for public keys (ssh-rsa, ssh-ed25519, ecdsa-sha2-nistp256, ecdsa-sha2-nistp384, ecdsa-sha2-nistp521) and private keys in OpenSSH format. The utility introduces error handling via the SSHKeyDetectionError enum for invalid or malformed key formats.

Unit Tests for SSH Key Detection:

  • Tests/CitadelTests/KeyTests.swift: Added multiple test cases to validate the functionality of SSHKeyDetection. These include:
    • Detection of all supported public key types and their descriptions.
    • Handling of keys with leading/trailing whitespace.
    • Error handling for invalid key formats, malformed keys, and unsupported types.
    • Validation of private key detection for ssh-ed25519, ssh-rsa, ecdsa-sha2-nistp256, ecdsa-sha2-nistp384, and ecdsa-sha2-nistp521.

Copilot AI review requested due to automatic review settings June 17, 2025 17:10

This comment was marked as outdated.

@nedithgar nedithgar requested a review from Copilot June 17, 2025 17:14

This comment was marked as outdated.

Copy link
Member

@Joannis Joannis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very cool PR, thank you! Just two comments and one nit. Please re-request a review if you push an update so I get an email

@Joannis
Copy link
Member

Joannis commented Jun 20, 2025

Very cool stuff

…ithms-down-the-line

Refactor SSHKeyType to a struct for improved E&M
* Enhance SSHKeyDetectionError with detailed error descriptions and additional cases

* Enhance SSHKeyDetectionError equality by including associated values for better error handling

* Fix SSHKeyDetectionError comparison by removing associated value for invalidKeyFormat

* Improve error description for unsupported key type in SSHKeyDetectionError
@nedithgar nedithgar requested review from Joannis and Copilot June 21, 2025 04:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new SSH key type detection feature to the Citadel module and comprehensive unit tests to validate it.

  • Introduces SSHKeyDetection with support for detecting public and OpenSSH private key types and related error handling.
  • Adds SSHKeyDetectionError enum for granular error cases.
  • Covers detection logic with extensive unit tests in KeyTests.swift.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
Sources/Citadel/SSHKeyTypeDetection.swift New utility for detecting SSH public/private key types and parsing logic.
Tests/CitadelTests/KeyTests.swift New unit tests covering valid key detection, whitespace handling, and error scenarios.
Comments suppressed due to low confidence (5)

Tests/CitadelTests/KeyTests.swift:178

  • Assert that the error thrown for an empty public key string is specifically SSHKeyDetectionError.invalidKeyFormat to ensure the correct error case is reported.
        XCTAssertThrowsError(try SSHKeyDetection.detectPublicKeyType(from: ""))

Tests/CitadelTests/KeyTests.swift:182

  • Add an assertion inside this block to verify the thrown error is SSHKeyDetectionError.invalidKeyFormat when the key prefix is present but no content follows.
        XCTAssertThrowsError(try SSHKeyDetection.detectPublicKeyType(from: emptyKey))

Tests/CitadelTests/KeyTests.swift:177

  • [nitpick] Consider adding a test case for an unsupported public key algorithm (e.g., "ssh-dss AAAA...") to check that SSHKeyDetection.detectPublicKeyType throws SSHKeyDetectionError.unsupportedKeyType.
        // Test empty string

Tests/CitadelTests/KeyTests.swift:284

  • [nitpick] The variable name ecdsa256PrivateKey is inconsistent with the public key tests (ecdsaP256PublicKey). Rename to ecdsaP256PrivateKey for clarity and consistency.
        let ecdsa256PrivateKey = """

Tests/CitadelTests/KeyTests.swift:295

  • [nitpick] Rename ecdsa256KeyType to ecdsaP256KeyType to match the SSHKeyType.ecdsaP256 enum case and maintain consistency with other test variable names.
        let ecdsa256KeyType = try SSHKeyDetection.detectPrivateKeyType(from: ecdsa256PrivateKey)

@Joannis Joannis merged commit d292854 into orlandos-nl:main Jun 23, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants