-
Notifications
You must be signed in to change notification settings - Fork 12
Implement SSH certificate authentication #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Introduced SSHConfigurationCertificateTests to validate client and server configurations with trusted host and user CA keys. - Implemented tests for adding, clearing, and validating trusted CA keys in SSHClientConfiguration and SSHServerConfiguration. - Added tests to ensure proper interaction between client and server configurations, including handling multiple certificate types and empty configurations. - Created SSHKeyExchangeCertificateTests to validate host certificate handling during key exchange, including validation with correct and incorrect CAs, critical options, and delegate interactions. - Included test fixtures for certificate validation scenarios.
…method Implement SSH certificate authentication and enhance documentation
There was a problem hiding this 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 pull request implements SSH certificate authentication support to enhance security in SwiftNIO SSH. The implementation adds certificate validation capabilities for both user and host authentication, allowing clients and servers to authenticate using X.509-style SSH certificates signed by trusted Certificate Authorities (CAs).
- Adds certificate-based authentication for both user and host keys with CA validation
- Introduces configuration options for trusted CAs and acceptable critical options
- Updates authentication state machines to handle certificate validation and delegate integration
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
Tests/NIOSSHTests/SSHKeyExchangeCertificateTests.swift | Tests for host certificate validation during key exchange |
Tests/NIOSSHTests/SSHConfigurationCertificateTests.swift | Tests for certificate-related configuration options |
Tests/NIOSSHTests/CertifiedKeyTests.swift | Extended tests for certificate authentication flows |
Tests/NIOSSHTests/CertificateAuthenticationIntegrationTests.swift | Integration tests for certificate authentication components |
Sources/NIOSSH/User Authentication/UserAuthenticationStateMachine.swift | Certificate validation logic in user authentication |
Sources/NIOSSH/User Authentication/UserAuthenticationMethod.swift | Certificate support in authentication methods |
Sources/NIOSSH/SSHServerConfiguration.swift | Server configuration for trusted user CAs |
Sources/NIOSSH/SSHClientConfiguration.swift | Client configuration for trusted host CAs |
Sources/NIOSSH/Keys And Signatures/ClientServerAuthenticationDelegate.swift | Host certificate validation delegate method |
Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift | Host certificate validation in key exchange |
Sources/NIOSSH/Docs.docc/index.md | Documentation updates for certificate support |
import NIOEmbedded | ||
@testable import NIOSSH | ||
import XCTest | ||
|
Copilot
AI
Jul 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test class lacks documentation explaining its purpose and scope. Consider adding a class-level comment describing what aspects of SSH certificate authentication in key exchange are being tested.
/// This test class validates the behavior of SSH certificate authentication during key exchange. | |
/// It includes tests for: | |
/// - Successful validation of host certificates signed by a trusted certificate authority (CA). | |
/// - Failure scenarios when certificates are signed by an untrusted CA. | |
/// - Handling of critical options in SSH certificates. |
Copilot uses AI. Check for mistakes.
let nonce = ByteBuffer(repeating: 0, count: 32) | ||
let baseKey = try NIOSSHPublicKey(openSSHPublicKey: Fixtures.p256UserBase) | ||
// Use a generated CA key instead of parsing from fixtures | ||
let caPrivateKey = createPrivateKey() |
Copilot
AI
Jul 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The helper function createPrivateKey() is defined at the bottom of the file but referenced here. Consider defining it closer to where it's first used or making it a static method for better code organization.
Copilot uses AI. Check for mistakes.
let nonce = ByteBuffer(repeating: 0, count: 32) | ||
let baseKey = try NIOSSHPublicKey(openSSHPublicKey: Fixtures.p256UserBase) | ||
// Use a generated CA key instead of parsing from fixtures | ||
let caPrivateKey = createPrivateKey() |
Copilot
AI
Jul 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The same helper function createPrivateKey() is used again. This duplication suggests the helper should be more prominent or the tests should be refactored to share common setup.
let caPrivateKey = createPrivateKey() | |
let caPrivateKey = TestHelpers.createPrivateKey() |
Copilot uses AI. Check for mistakes.
} | ||
|
||
// Check if this is a certificate and validate it | ||
var validatedCertificate: NIOSSHCertifiedPublicKey? = nil |
Copilot
AI
Jul 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using explicit 'nil' initialization is unnecessary in Swift. Consider using 'var validatedCertificate: NIOSSHCertifiedPublicKey?' instead.
var validatedCertificate: NIOSSHCertifiedPublicKey? = nil | |
var validatedCertificate: NIOSSHCertifiedPublicKey? |
Copilot uses AI. Check for mistakes.
} | ||
} | ||
|
||
struct HostBased { |
Copilot
AI
Jul 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message was updated but the init() method comment above still references 'PublicKeyRequest' which appears to be a copy-paste error from the original code.
struct HostBased { | |
struct HostBased { | |
/// HostBased authentication is currently unimplemented. | |
/// This method will trigger a runtime error if called. |
Copilot uses AI. Check for mistakes.
Also consider making this PR further upstream in the Apple Repo. It shouldn't be much different from your PR here. Just for ease of reviewing, could you point me to the part in the SSH spec that you followed? |
Sure thing! I'd be happy to contribute this to the upstream Apple However, I am worried about acceptance since the last related code changes of this feature appear to date back to 2021. For your reference, I've included the relevant implementation details below. Primary References:
Key Changes Implemented:
The implementation maintains full SSH protocol compatibility by treating certificates as a special case of public key authentication, exactly as specified in RFC 4252. |
After further investigation, I've concluded that a contribution to the upstream apple/swift-nio-ssh repository is not currently feasible. The implementation has a hard dependency on unmerged code from a pending pull request in apple/swift-nio-ssh, specifically on APIs like Because of that, opening a PR on the Apple repo might not be an option for now. |
This pull request introduces SSH certificate authentication support to SwiftNIO SSH, enhancing both client and server functionality. The changes include updates to support certificate-based authentication for both user and host keys, validation mechanisms, and new configuration options for trusted certificate authorities (CAs). Below is a breakdown of the most important changes:
Documentation Updates
Core Functionality Enhancements
SSHKeyExchangeStateMachine
to validate host certificates when trusted CAs are configured. Added logic to handle certificate validation and delegate additional checks to the server authentication delegate.validateHostCertificate
inNIOSSHClientServerAuthenticationDelegate
for validating host certificates, with a default implementation for backward compatibility.Configuration Additions
trustedHostCAKeys
andhostname
properties toSSHClientConfiguration
for host certificate validation.trustedUserCAKeys
andacceptableCriticalOptions
properties toSSHServerConfiguration
for user certificate validation.User Authentication Updates
NIOSSHUserAuthenticationRequest.Request.PublicKey
to include certificate information (certifiedKey
) and updated initializers to support certified keys.UserAuthenticationStateMachine
to validate user certificates against trusted CAs and pass the validated certificate to the delegate. [1] [2]Additional Notes