Add support for custom cryptography #62

Joannis · 2020-12-21T11:20:28Z

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 · 2020-12-21T11:21:03Z

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

Lukasa · 2021-01-05T09:52:54Z

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.

Joannis · 2021-01-21T23:55:11Z

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

Lukasa · 2021-01-22T17:11:47Z

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

…h-1 into jo-rsa-private-keys

Joannis · 2021-02-26T18:29:42Z

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

# Conflicts: # Package.swift

…ther symmetric key algorithms can be used

waylybaye · 2021-10-04T05:38:15Z

Hello, any plan to merge the RSA keys support?

Lukasa · 2021-10-04T08:19:15Z

@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 · 2021-10-04T11:34:56Z

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

Lukasa · 2021-11-12T15:31:35Z

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

Joannis · 2021-11-14T22:05:15Z

@Lukasa is that the only requested/required change?

Lukasa · 2021-11-15T08:24:01Z

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 · 2021-11-16T20:54:59Z

Done @Lukasa

Lukasa

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

Lukasa · 2021-11-23T12:27:42Z

Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift

@@ -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] = [


Why is this var?

Lukasa · 2021-11-23T12:27:54Z

Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift

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


Can we remove the whitespace changes in this file?

Sources/NIOSSH/Key Exchange/EllipticCurveKeyExchange.swift

Lukasa · 2021-11-23T12:32:09Z

Sources/NIOSSH/Key Exchange/EllipticCurveKeyExchange.swift

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


Can we remove this whitespace change?

Lukasa · 2021-11-23T12:44:24Z

Sources/NIOSSH/Keys And Signatures/NIOSSHPublicKey.swift

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


Do these need to be public?

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

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

Yes: https://datatracker.ietf.org/doc/html/rfc4253#section-8

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

Sources/NIOSSH/Keys And Signatures/NIOSSHSignature.swift

Tests/NIOSSHTests/EndToEndTests.swift

… configuration objects

Lukasa · 2021-11-25T16:27:12Z

Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift

@@ -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] = [


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

Sources/NIOSSH/Key Exchange/EllipticCurveKeyExchange.swift

Lukasa · 2021-11-25T17:11:58Z

Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift

    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?) {


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

Lukasa · 2021-11-25T17:16:52Z

Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift

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


Suggested change

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

serverAlgorithms = role.keyExchangeAlgorithmNames

Lukasa · 2021-11-25T17:16:58Z

Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift

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

        switch self.role {
        case .client:
-            clientAlgorithms = Self.supportedKeyExchangeAlgorithms
+            clientAlgorithms = role.keyExchangeAlgorithms.flatMap { $0.keyExchangeAlgorithmNames }


Suggested change

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

clientAlgorithms = role.keyExchangeAlgorithmNames

Lukasa · 2021-11-25T17:22:42Z

Sources/NIOSSH/Keys And Signatures/NIOSSHPublicKey.swift

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


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

Lukasa · 2021-11-25T17:23:08Z

Sources/NIOSSH/Keys And Signatures/NIOSSHPublicKey.swift

+    return _customKeyExchangeAlgorithms
+}
+
+internal let customAlgorithmsLock = NSLock()


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

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

Lukasa · 2021-11-25T17:25:20Z

Sources/NIOSSH/Keys And Signatures/NIOSSHPublicKey.swift

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


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

Lukasa · 2021-11-25T17:27:42Z

Sources/NIOSSH/Role.swift

+    }
+
+    internal var keyExchangeAlgorithmNames: [Substring] {
+        keyExchangeAlgorithms.flatMap { $0.keyExchangeAlgorithmNames }


Nit:

Suggested change

keyExchangeAlgorithms.flatMap { $0.keyExchangeAlgorithmNames }

self.keyExchangeAlgorithms.flatMap { $0.keyExchangeAlgorithmNames }

Lukasa · 2021-11-25T17:28:15Z

Sources/NIOSSH/TransportProtection/AESGCM.swift

@@ -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


Nit:

Suggested change

N throw NIOSSHError.invalidKeySize

throw NIOSSHError.invalidKeySize

swift-server-bot · 2022-05-15T07:27:40Z

Can one of the admins verify this patch?

Lukasa

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.

Lukasa · 2022-05-23T07:54:22Z

Sources/NIOSSH/Child Channels/ChildChannelOptions.swift

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

-extension SSHChildChannelOptions {
-    public enum Types {}
+public extension SSHChildChannelOptions {


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

Lukasa · 2022-05-23T07:54:28Z

Sources/NIOSSH/Child Channels/ChildChannelOptions.swift

 }

-extension SSHChildChannelOptions.Types {
+public extension SSHChildChannelOptions.Types {


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

Lukasa · 2022-05-23T07:54:34Z

Sources/NIOSSH/Child Channels/ChildChannelStateMachine.swift

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

-extension ChildChannelStateMachine {
-    fileprivate enum State: Hashable {
+private extension ChildChannelStateMachine {


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

Lukasa · 2022-05-23T07:54:49Z

Sources/NIOSSH/Child Channels/ChildChannelUserEvents.swift

@@ -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? {


This change is unnecessary, please remove.

…tests.

gwynne · 2022-05-23T08:09:29Z

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

Lukasa · 2022-05-23T08:26:13Z

Sources/NIOSSH/Keys And Signatures/CustomKeys.swift

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


Can you fix up this copyright header to read 2022?

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

Lukasa · 2022-05-23T08:30:25Z

Sources/NIOSSH/Keys And Signatures/NIOSSHPublicKey.swift

+    }
+}
+
+public enum NIOSSHAlgorithms {


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

@Joannis Can you weigh in on this?

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

@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?

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.

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! :)

Joannis added 2 commits October 28, 2020 21:36

Initial client-side RSA private key support

620d931

Implement RSA in a separate module, to be removed before a merge

cc00081

Joannis marked this pull request as draft December 21, 2020 11:20

Joannis mentioned this pull request Dec 21, 2020

underlyingCoreCryptoError while creating NIOSSHPrivateKey #60

Open

aaronvegh mentioned this pull request Jan 21, 2021

Support OpenSSH Keys #66

Open

Removed RSA, added a custom key test

17f5cad

Joannis added 3 commits February 12, 2021 23:20

Merge branch 'main' into jo-rsa-private-keys

f0e31b2

Add docs

077e954

Merge branch 'jo-rsa-private-keys' of github.com:joannis/swift-nio-ss…

fc5f565

…h-1 into jo-rsa-private-keys

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

Joannis added 7 commits February 26, 2021 19:29

Merge branch 'main' into jo-rsa-private-keys

497919b

Merge branch 'main' into jo-rsa-private-keys

a742b44

# Conflicts: # Package.swift

Support custom public key types for host keys

4ecf937

More transport options

5852c0c

Implemented passing sequence numbers, and adapter protocols so that o…

569c03e

…ther symmetric key algorithms can be used

Ignore the authentication banner

3837545

Enable old algorithms

316c8d6

Merge remote-tracking branch 'apple/main' into jo-rsa-private-keys

ee4b862

Merge remote-tracking branch 'apple/main' into jo-rsa-private-keys

241b456

# Conflicts: # Sources/NIOSSH/SSHMessages.swift

Joannis added 2 commits November 16, 2021 16:59

Remove conflicts with PR apple#98

d8c64fa

Fixed broken tests after merge. Added tests for all algorithms using …

a50df12

…frankenstein algorithms that test the correctness of SwiftNIO's agnostic approach to cryptograhic choices

Joannis changed the title ~~Add support for custom public-private key authentication types~~ Add support for custom cryptography Nov 17, 2021

Lukasa reviewed Nov 23, 2021

View reviewed changes

gwynne and others added 3 commits November 24, 2021 12:23

Merge branch 'apple:main' into jo-rsa-private-keys

3f42d1f

Define transport protection & key exchange types on the client/server…

f4efdbc

… configuration objects

Remove whitespace

642d961

Lukasa reviewed Nov 25, 2021

View reviewed changes

gwynne added 3 commits December 4, 2021 13:37

Fix typo

c5ccb88

Address some of the PR feedback

a4f1a81

Merge branch 'main' into jo-rsa-private-keys

c66f0f1

gwynne force-pushed the jo-rsa-private-keys branch from 719f5ce to 2a85b45 Compare May 15, 2022 08:12

Lukasa requested changes May 23, 2022

View reviewed changes

gwynne added 4 commits May 23, 2022 03:03

Remove defaulted parameters per PR feedback and fix a pile of broken …

df0b47e

…tests.

Encapsulate globals in an enum per PR feedback

f8b8add

Use fine-grained locking per PR feedback

b66b64f

Address soundness.sh issues with correct version of swiftformat.

d1fc273

gwynne force-pushed the jo-rsa-private-keys branch from 2a85b45 to d1fc273 Compare May 23, 2022 08:08

Lukasa requested changes May 23, 2022

View reviewed changes

Fix copyright header year

4b0e7ec

JaapWijnen force-pushed the jo-rsa-private-keys branch from 64deede to 4b0e7ec Compare August 11, 2022 09:11

Merge branch 'apple:main' into jo-rsa-private-keys

86a99b1

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

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

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

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

	N throw NIOSSHError.invalidKeySize
	throw NIOSSHError.invalidKeySize

Add support for custom cryptography #62

Are you sure you want to change the base?

Add support for custom cryptography #62

Conversation

Joannis commented Dec 21, 2020 • edited

Joannis commented Dec 21, 2020

Lukasa commented Jan 5, 2021

Joannis commented Jan 21, 2021

Lukasa commented Jan 22, 2021

Joannis commented Feb 26, 2021

waylybaye commented Oct 4, 2021

Lukasa commented Oct 4, 2021

waylybaye commented Oct 4, 2021

Lukasa commented Nov 12, 2021

Joannis commented Nov 14, 2021

Lukasa commented Nov 15, 2021

Joannis commented Nov 16, 2021

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swift-server-bot commented May 15, 2022

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwynne commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaapWijnen Jul 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaapWijnen Aug 12, 2022 • edited

Choose a reason for hiding this comment

Joannis commented Dec 21, 2020 •

edited

JaapWijnen Jul 11, 2022 •

edited

JaapWijnen Aug 12, 2022 •

edited