Skip to content
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

Add support for custom cryptography #62

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

Joannis
Copy link
Collaborator

@Joannis Joannis commented Dec 21, 2020

Notes:

I've implemented RSA in a separate module within this repo, to be split out of this repository before merging. It serves as an example implementation for this PR.

The RSA implementation now resides in Citadel

I believe there are a couple of API design choices, like protocol names, that should be improved. I'm happy to take any feedback, of course.

@Joannis Joannis marked this pull request as draft December 21, 2020 11:20
@Joannis
Copy link
Collaborator Author

Joannis commented Dec 21, 2020

@Lukasa please do review once you'e back from a well deserved vacation.

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 5, 2021

This seems like a great start! The big missing link here seems to be user authentication, which I'm fine to omit for now as dealing simply with user and host keys will solve the 90% use-case handily. As this PR demonstrates, it's also much easier to implement!

I'd be entirely happy with you productising this PR.

@aaronvegh aaronvegh mentioned this pull request Jan 21, 2021
@Joannis
Copy link
Collaborator Author

Joannis commented Jan 21, 2021

@Lukasa I've updated it all except the docs. Before I add the docs, what do you think of registering SSH public keys?

@Lukasa
Copy link
Collaborator

Lukasa commented Jan 22, 2021

I think that's going to be necessary. We can add a thread-safe registry allowing what amount to "plugins" for SSH key types.

@Joannis
Copy link
Collaborator Author

Joannis commented Feb 26, 2021

I appearantly forgot to push my commit & ping you. Is this sufficient?

@Joannis Joannis marked this pull request as ready for review February 26, 2021 18:29
@waylybaye
Copy link

Hello, any plan to merge the RSA keys support?

@Lukasa
Copy link
Collaborator

Lukasa commented Oct 4, 2021

@waylybaye This PR sort of got lost in the shuffle, but I'd happily review a version of this brought up-to-date with the current main.

@waylybaye
Copy link

@Lukasa Great! The only thing blocking me to use nio-ssh is the lack of RSA support. Thank you and Joannis.

@Lukasa
Copy link
Collaborator

Lukasa commented Nov 12, 2021

This conflicts with the changes made in #98, can we clean that up?

@Joannis
Copy link
Collaborator Author

Joannis commented Nov 14, 2021

@Lukasa is that the only requested/required change?

@Lukasa
Copy link
Collaborator

Lukasa commented Nov 15, 2021

Nope, it just cleans the diff up enough that I can do a proper review. 😄

# Conflicts:
#	Sources/NIOSSH/SSHMessages.swift
…frankenstein algorithms that test the correctness of SwiftNIO's agnostic approach to cryptograhic choices
@Joannis
Copy link
Collaborator Author

Joannis commented Nov 16, 2021

Done @Lukasa

@Joannis Joannis changed the title Add support for custom public-private key authentication types Add support for custom cryptography Nov 17, 2021
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Great, this is a really solid start! I've left some notes in the diff.

@@ -60,7 +60,7 @@ struct SSHConnectionStateMachine {
/// The state of this state machine.
private var state: State

private static let defaultTransportProtectionSchemes: [NIOSSHTransportProtection.Type] = [
internal static var defaultTransportProtectionSchemes: [NIOSSHTransportProtection.Type] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this var?

@@ -818,7 +819,7 @@ struct SSHConnectionStateMachine {
case .userAuthRequest(let message):
try state.writeUserAuthRequest(message, into: &buffer)
self.state = .userAuthentication(state)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove the whitespace changes in this file?

precondition(self.ourRole.isClient, "Only clients may receive a server key exchange packet!")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove this whitespace change?


@discardableResult
public func writeWithoutHeader(to buffer: inout ByteBuffer) -> Int {
return buffer.writeSSHHostKeyWithoutHeader(self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these need to be public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't quite need writeWithoutHeader in my use case, but the write(to: ) function is required for implementing DH Group 14 SHA1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's necessary to write the host key in that context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove writeWithoutHeader then? It'd be good to avoid public API surface we're not sure we need.

Sources/NIOSSH/Keys And Signatures/NIOSSHPublicKey.swift Outdated Show resolved Hide resolved
Tests/NIOSSHTests/EndToEndTests.swift Show resolved Hide resolved
Tests/NIOSSHTests/EndToEndTests.swift Outdated Show resolved Hide resolved
@@ -60,12 +60,12 @@ struct SSHConnectionStateMachine {
/// The state of this state machine.
private var state: State

private static let defaultTransportProtectionSchemes: [NIOSSHTransportProtection.Type] = [
public static let bundledTransportProtectionSchemes: [NIOSSHTransportProtection.Type] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be internal, as the type is not public.

private var previousSessionIdentifier: ByteBuffer?

init(allocator: ByteBufferAllocator, loop: EventLoop, role: SSHConnectionRole, remoteVersion: String, protectionSchemes: [NIOSSHTransportProtection.Type], previousSessionIdentifier: ByteBuffer?) {
init(allocator: ByteBufferAllocator, loop: EventLoop, role: SSHConnectionRole, remoteVersion: String, keyExchangeAlgorithms: [NIOSSHKeyExchangeAlgorithmProtocol.Type] = SSHKeyExchangeStateMachine.bundledKeyExchangeImplementations, transportProtectionSchemes: [NIOSSHTransportProtection.Type] = SSHConnectionStateMachine.bundledTransportProtectionSchemes, previousSessionIdentifier: ByteBuffer?) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason to default the keyExchangeAlgorithms and transportProtectionSchemes here?

serverAlgorithms = peerKeyExchangeAlgorithms
clientHostKeyAlgorithms = self.supportedHostKeyAlgorithms
serverHostKeyAlgorithms = peerHostKeyAlgorithms
case .server:
clientAlgorithms = peerKeyExchangeAlgorithms
serverAlgorithms = Self.supportedKeyExchangeAlgorithms
serverAlgorithms = role.keyExchangeAlgorithms.flatMap { $0.keyExchangeAlgorithmNames }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
serverAlgorithms = role.keyExchangeAlgorithms.flatMap { $0.keyExchangeAlgorithmNames }
serverAlgorithms = role.keyExchangeAlgorithmNames

@@ -369,13 +377,13 @@ struct SSHKeyExchangeStateMachine {

switch self.role {
case .client:
clientAlgorithms = Self.supportedKeyExchangeAlgorithms
clientAlgorithms = role.keyExchangeAlgorithms.flatMap { $0.keyExchangeAlgorithmNames }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
clientAlgorithms = role.keyExchangeAlgorithms.flatMap { $0.keyExchangeAlgorithmNames }
clientAlgorithms = role.keyExchangeAlgorithmNames

fileprivate var _customTransportProtectionSchemes = [NIOSSHTransportProtection.Type]()
fileprivate var _customKeyExchangeAlgorithms = [NIOSSHKeyExchangeAlgorithmProtocol.Type]()
fileprivate var _customPublicKeyAlgorithms: [NIOSSHPublicKeyProtocol.Type] = []
fileprivate var _customSignatures: [NIOSSHSignatureProtocol.Type] = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we nest these inside an enum without cases just to give them a namespace?

return _customKeyExchangeAlgorithms
}

internal let customAlgorithmsLock = NSLock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably use fine-grained locking here, so can we have one lock per field? It'll reduce contention on the locks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally, instead of NSLock can we add a dependency on NIOConcurrencyHelpers and use that Lock?


@discardableResult
public func writeWithoutHeader(to buffer: inout ByteBuffer) -> Int {
return buffer.writeSSHHostKeyWithoutHeader(self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove writeWithoutHeader then? It'd be good to avoid public API surface we're not sure we need.

}

internal var keyExchangeAlgorithmNames: [Substring] {
keyExchangeAlgorithms.flatMap { $0.keyExchangeAlgorithmNames }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit:

Suggested change
keyExchangeAlgorithms.flatMap { $0.keyExchangeAlgorithmNames }
self.keyExchangeAlgorithms.flatMap { $0.keyExchangeAlgorithmNames }

@@ -43,7 +43,7 @@ internal class AESGCMTransportProtection {
required init(initialKeys: NIOSSHSessionKeys) throws {
guard initialKeys.outboundEncryptionKey.bitCount == Self.keySizes.encryptionKeySize * 8,
initialKeys.inboundEncryptionKey.bitCount == Self.keySizes.encryptionKeySize * 8 else {
throw NIOSSHError.invalidKeySize
N throw NIOSSHError.invalidKeySize
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit:

Suggested change
N throw NIOSSHError.invalidKeySize
throw NIOSSHError.invalidKeySize

@swift-server-bot
Copy link

Can one of the admins verify this patch?

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

I started reviewing this, but the change is massive because it has reformatted the codebase. Can you please use the exact version of swift-format that CI is using and re-run to clean up the format commit? Our Dockerfile can assist with that.

@@ -31,34 +31,34 @@ public struct SSHChildChannelOptions {
public static let peerMaximumMessageLength: SSHChildChannelOptions.Types.PeerMaximumMessageLengthOption = .init()
}

extension SSHChildChannelOptions {
public enum Types {}
public extension SSHChildChannelOptions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIO style is to place the access modifiers within the extensions, not on the extension itself.

}

extension SSHChildChannelOptions.Types {
public extension SSHChildChannelOptions.Types {
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIO style is to place the access modifiers within the extensions, not on the extension itself.

@@ -24,8 +24,8 @@ struct ChildChannelStateMachine {
}
}

extension ChildChannelStateMachine {
fileprivate enum State: Hashable {
private extension ChildChannelStateMachine {
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIO style is to place the access modifiers within the extensions, not on the extension itself.

@@ -299,7 +299,7 @@ extension SSHChannelRequestEvent {
/// Constructs a channel request event and wraps it up in an Any.
///
/// This is usually used just prior to firing this into the pipeline.
internal static func fromMessage(_ message: SSHMessage.ChannelRequestMessage) -> Any? {
static func fromMessage(_ message: SSHMessage.ChannelRequestMessage) -> Any? {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is unnecessary, please remove.

@gwynne
Copy link
Contributor

gwynne commented May 23, 2022

@Lukasa Rebased out all the swiftformat changes and re-applied the correct version, diff should be much better now 🤞

//
// This source file is part of the SwiftNIO open source project
//
// Copyright (c) YEARS Apple Inc. and the SwiftNIO project authors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you fix up this copyright header to read 2022?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed. Is there a reason not to be using swiftformat's fileHeader rule for this? AFAIK the version in use supports it.

}
}

public enum NIOSSHAlgorithms {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I remain uncertain about doing this with global state. Can we consider placing this on per connection configuration instead of using this global config?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Joannis Can you weigh in on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that a per-connection basis is a better solution. But we will need a registry for sure, since we can have a variety of Host Keys for clients, and accepted public keys for servers

Copy link

@JaapWijnen JaapWijnen Jul 11, 2022

Choose a reason for hiding this comment

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

@Lukasa @gwynne While I was picking this up I was discussing this with @Joannis and want to introduce a per connection solution. But while discussing, with the way the custom keys currently work we could get rid of the BackingKey enums that are currently internal in the library, by making the bundled keys conform to NIOSSHPrivateKeyProtocol and similar protocols. This would eliminate a lot of switch statements and simplify a lot of methods quite a bit. We don't immediately see any issues, but I wanted to check in with you, what are your thoughts on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the switch statements are that big a deal: they provide fast-paths for the most common use-cases where the compiler can be aware of exactly what the implementation is. The extensible backing pattern should certainly run this through a protocol though.

Copy link

@JaapWijnen JaapWijnen Aug 12, 2022

Choose a reason for hiding this comment

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

That's true it would make the whole implementation more consistent however and remove a lot of boilerplate/duplication. So not something the user benefits from perse but still could be interesting in terms of maintainability/accessible for new devs checking out the repo? I put in quite some work to get it towards that end. Would you be interested in seeing a PR?
(it still needs a little work but if you're not interested in merging that kind of work in to the repo I'll probably leave it as is and just finish up your last remarks regarding not using global state for registering algorithms, which btw already needs quite a bit of moving around of methods in order to pass the registered algorithms down to the right ByteBuffer extensions)
I'm hoping to finish up the work and then ideally divide it into a few PR's so it's a bit more manageable for you to review. Anyway let me know! :)

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.

6 participants