Adds support for custom transport protection algorithms #97

artemredkin · 2021-10-13T16:00:35Z

Motivation:
In some cases AES GCM might not be supported by a server/client, and
clients might require custom algorithms to be implemented in their
projects.

Modifications:

Makes NIOSSHTransportProtection, NIOSSHSessionKeys and ExpectedKeySizes public
Makes transport algorithms list configurable
Adds sequence number of incoming and outgoing messages to parser and
serializer
Tests

artemredkin · 2021-10-13T16:03:10Z

Sources/NIOSSH/Constants.swift

@@ -12,6 +12,10 @@
 //
 //===----------------------------------------------------------------------===//

-enum Constants {
+public enum Constants {


Not sure this is the best place, maybe we need to type Defaults or smth?

I agree with you, I think Defaults would be better.

Decided to change this a bit, I will rename constant to bundledTransportProtectionSchemes, I quite like this idea from Joannis's PR, wdyt?

artemredkin · 2021-10-13T16:04:40Z

Sources/NIOSSH/Constants.swift

    static let version = "SSH-2.0-SwiftNIOSSH_1.0"
+
+    public static let defaultTransportProtectionSchemes: [NIOSSHTransportProtection.Type] = [


I made it public so it would be possible both using only custom provided algs and adding new ones in addition to defaults one, but we probably can make it a bit nicer...

I'm not sure that there's a better alternative than this.

artemredkin · 2021-10-13T16:07:01Z

Sources/NIOSSH/TransportProtection/AESGCM.swift

@@ -79,7 +79,7 @@ extension AESGCMTransportProtection: NIOSSHTransportProtection {
        // unencrypted!
    }

-    func decryptAndVerifyRemainingPacket(_ source: inout ByteBuffer) throws -> ByteBuffer {
+    func decryptAndVerifyRemainingPacket(_ source: inout ByteBuffer, sequence: UInt32) throws -> ByteBuffer {


CTR requires packet sequence number for MAC verification, same for encryption, and it seems that those sequences start when binary protocol starts...

Can we make it clear that AESGCM doesn't need it, by using sequence _: UInt32 here and below?

Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift

Joannis

Looks like a solid PR to me. I don't have anything to add to the code, but I don't think #50 can be closed after this PR. This PR only covers transport protection.

artemredkin · 2022-04-01T11:19:33Z

Agreed, but at least it should be referenced there, thank you!

Lukasa · 2022-04-05T15:24:46Z

Sources/NIOSSH/Constants.swift

@@ -12,6 +12,10 @@
 //
 //===----------------------------------------------------------------------===//

-enum Constants {
+public enum Constants {


I agree with you, I think Defaults would be better.

Lukasa · 2022-04-05T15:26:44Z

Sources/NIOSSH/Constants.swift

    static let version = "SSH-2.0-SwiftNIOSSH_1.0"
+
+    public static let defaultTransportProtectionSchemes: [NIOSSHTransportProtection.Type] = [


I'm not sure that there's a better alternative than this.

Sources/NIOSSH/Key Exchange/SSHKeyExchangeResult.swift

Lukasa · 2022-04-05T15:32:06Z

Sources/NIOSSH/SSHPacketParser.swift

+            self.sequence = 0
+        } else {
+            self.sequence += 1
+        }


This can be more succinctly spelled self.sequence &+= 1.

done, plus renamed to sequenceNumber as per Joannis Orlandos's PR suggestion

Lukasa · 2022-04-05T15:34:35Z

Sources/NIOSSH/SSHPacketSerializer.swift

+        } else {
+            self.sequence += 1
+        }
+    }


Same note here.

Lukasa · 2022-04-05T15:35:28Z

Sources/NIOSSH/TransportProtection/AESGCM.swift

@@ -79,7 +79,7 @@ extension AESGCMTransportProtection: NIOSSHTransportProtection {
        // unencrypted!
    }

-    func decryptAndVerifyRemainingPacket(_ source: inout ByteBuffer) throws -> ByteBuffer {
+    func decryptAndVerifyRemainingPacket(_ source: inout ByteBuffer, sequence: UInt32) throws -> ByteBuffer {


Can we make it clear that AESGCM doesn't need it, by using sequence _: UInt32 here and below?

Sources/NIOSSH/SSHClientConfiguration.swift

artemredkin · 2022-04-11T11:18:25Z

Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift

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

-    private static let defaultTransportProtectionSchemes: [NIOSSHTransportProtection.Type] = [
+    public static let bundledTransportProtectionSchemes: [NIOSSHTransportProtection.Type] = [


I decided to make this public so clients can have an ability to specify a fully custom set of algorithms (i.e. exclude bundled). But if they need to add custom, they will be able to specify bundled + custom

Ah, NIOSSHConnectionStateMachine is not public, moved to SSHConnectionRole

artemredkin · 2022-04-11T11:51:20Z

Sources/NIOSSH/TransportProtection/SSHTransportProtection.swift


    /// Encrypt an entire outbound packet
-    func encryptPacket(_ packet: NIOSSHEncryptablePayload, to outboundBuffer: inout ByteBuffer) throws
+    func encryptPacket(_ packet: NIOSSHEncryptablePayload, sequenceNumber: UInt32, to outboundBuffer: inout ByteBuffer) throws


One thing I haven't solved yet, is how to use NIOSSHEncryptablePayload to write tests for custom transport implementations. It uses internal type (SSHMessage), so there is no way to initialize it. I see three options:

Make it a protocol

Make it an enum (`SSHMessage|ByteBuffer)

Make encryptPacket similar to decrypt, where we will be passing an inout buffer with message already written there expecting client to encrypt it inplace
wdyt?

(right now I'm in favour of 3 tbh)

I think many implementation will begin with:

let packetLengthIndex = outboundBuffer.writerIndex let packetLengthLength = MemoryLayout<UInt32>.size let packetPaddingIndex = outboundBuffer.writerIndex + packetLengthLength let packetPaddingLength = MemoryLayout<UInt8>.size outboundBuffer.moveWriterIndex(forwardBy: packetLengthLength + packetPaddingLength)

maybe we should abstract in serializer?

I suspect (3) is the right approach, yes. We can also abstract the write pattern in the serializer as well. Can I suggest that we do that as a separate PR though to avoid this one growing further?

agreed 👍

artemredkin · 2022-04-11T16:46:36Z

Sources/NIOSSH/TransportProtection/SSHTransportProtection.swift

@@ -87,10 +87,10 @@ protocol NIOSSHTransportProtection: AnyObject {
    /// length, the padding, or the MAC), and update source to indicate the consumed bytes.
    /// It must also perform any integrity checking that
    /// is required and throw if the integrity check fails.
-    func decryptAndVerifyRemainingPacket(_ source: inout ByteBuffer) throws -> ByteBuffer
+    func decryptAndVerifyRemainingPacket(_ source: inout ByteBuffer, sequenceNumber: UInt32) throws -> ByteBuffer


Actually, two additional questions before we make this API public:

Why do we return a ByteBuffer here instead of in-place mutation?

There is a bit of a lost symmetry here, in plaintext mode padding and packet length are consumed by parser, but in ciphertext mode we have to do it manually, should this be handled by parser instead? And decryptors should only decrypt data in place and leave it? (Maybe we should drop mac bytes here?)

I think parser can handle mac bytes, we have macBytes property, we can use it to remove last bytes from the packet

As the doc comment says: it is expected to update the buffer to indicate what it consumed, and return the decrypted buffer. We avoid in-place mutation because we do not want to guarantee that all encryption algorithms have the exact same size input and output.

The absence of symmetry is because in the input case we need to be able to decrypt just the length so we know how much further data to read, and we wanted to minimise the number of times we need to do that operation. I don't think the parser can do it for us because the actual length is affected by the protection scheme.

We could pass the MAC separately, but I'm not sure we gain much do we?

let me think about this further, I'll revisit this together with refactoring encryptPacket

Just for my clarification, did this work get done?

I opened a draft here:
artemredkin#1
but so far I only refactored encryptPacket, we probably should have two separate PRs for encryptPacket and decryptAndVerifyRemainingPacket

Ok great, so you'd like to have this PR merged without further changes to this and we'll revisit in a follow-on PR?

yes! otherwise it's already quite large

Lukasa · 2022-04-12T12:06:26Z

Sources/NIOSSH/Role.swift

@@ -14,6 +14,10 @@

 /// The role of a given party in an SSH connection.
 public enum SSHConnectionRole {
+    public static let bundledTransportProtectionSchemes: [NIOSSHTransportProtection.Type] = [


I don't think this makes sense on SSHConnectionRole. This should go on config objects. We can avoid some duplication by using an internal central location.

moved to Constants

Sources/NIOSSH/SSHClientConfiguration.swift

Lukasa · 2022-04-12T12:36:26Z

Sources/NIOSSH/TransportProtection/SSHTransportProtection.swift

@@ -87,10 +87,10 @@ protocol NIOSSHTransportProtection: AnyObject {
    /// length, the padding, or the MAC), and update source to indicate the consumed bytes.
    /// It must also perform any integrity checking that
    /// is required and throw if the integrity check fails.
-    func decryptAndVerifyRemainingPacket(_ source: inout ByteBuffer) throws -> ByteBuffer
+    func decryptAndVerifyRemainingPacket(_ source: inout ByteBuffer, sequenceNumber: UInt32) throws -> ByteBuffer


As the doc comment says: it is expected to update the buffer to indicate what it consumed, and return the decrypted buffer. We avoid in-place mutation because we do not want to guarantee that all encryption algorithms have the exact same size input and output.

The absence of symmetry is because in the input case we need to be able to decrypt just the length so we know how much further data to read, and we wanted to minimise the number of times we need to do that operation. I don't think the parser can do it for us because the actual length is affected by the protection scheme.

We could pass the MAC separately, but I'm not sure we gain much do we?

Lukasa · 2022-04-12T12:37:20Z

Sources/NIOSSH/TransportProtection/SSHTransportProtection.swift


    /// Encrypt an entire outbound packet
-    func encryptPacket(_ packet: NIOSSHEncryptablePayload, to outboundBuffer: inout ByteBuffer) throws
+    func encryptPacket(_ packet: NIOSSHEncryptablePayload, sequenceNumber: UInt32, to outboundBuffer: inout ByteBuffer) throws


I suspect (3) is the right approach, yes. We can also abstract the write pattern in the serializer as well. Can I suggest that we do that as a separate PR though to avoid this one growing further?

Motivation: In some cases AES GCM might not be supported by a server/client, and clients might require custom algorithms to be implemented in their projects. Modifications: - Makes NIOSSHTransportProtection, NIOSSHSessionKeys and ExpectedKeySizes public - Makes transport algorithms list configurable - Adds sequence number of incoming and outgoing messages to parser and serializer - Tests

Lukasa

Great, let's ship this. Thanks so much for the work!

artemredkin added the enhancement New feature or request label Oct 13, 2021

artemredkin requested a review from Lukasa October 13, 2021 16:00

artemredkin commented Oct 13, 2021

View reviewed changes

Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift Outdated Show resolved Hide resolved

artemredkin force-pushed the custom_transport_protection_algs_support branch 3 times, most recently from f9170d0 to 48ae0bb Compare February 7, 2022 15:43

artemredkin force-pushed the custom_transport_protection_algs_support branch 2 times, most recently from 8e8a4f3 to 8bf1a9a Compare April 1, 2022 10:24

artemredkin changed the title ~~👷‍♂️Adds support for custom transport protection algorithms~~ Adds support for custom transport protection algorithms Apr 1, 2022

artemredkin force-pushed the custom_transport_protection_algs_support branch from 8bf1a9a to d056ef3 Compare April 1, 2022 10:25

artemredkin linked an issue Apr 1, 2022 that may be closed by this pull request

Third-Party Cryptographic Implementations #50

Closed

artemredkin requested a review from Joannis April 1, 2022 10:32

artemredkin force-pushed the custom_transport_protection_algs_support branch from d056ef3 to 1e7e46d Compare April 1, 2022 11:15

Joannis approved these changes Apr 1, 2022

View reviewed changes

Lukasa reviewed Apr 5, 2022

View reviewed changes

artemredkin force-pushed the custom_transport_protection_algs_support branch 2 times, most recently from 0d7cf3a to 7dba735 Compare April 11, 2022 10:24

artemredkin commented Apr 11, 2022

View reviewed changes

Sources/NIOSSH/SSHClientConfiguration.swift Show resolved Hide resolved

artemredkin force-pushed the custom_transport_protection_algs_support branch from 7dba735 to 89de739 Compare April 11, 2022 10:28

artemredkin requested a review from Lukasa April 11, 2022 10:37

artemredkin force-pushed the custom_transport_protection_algs_support branch from 89de739 to a4d3621 Compare April 11, 2022 11:16

artemredkin commented Apr 11, 2022

View reviewed changes

artemredkin force-pushed the custom_transport_protection_algs_support branch 2 times, most recently from 6bfb945 to a7e248c Compare April 11, 2022 11:43

artemredkin commented Apr 11, 2022

View reviewed changes

Lukasa reviewed Apr 12, 2022

View reviewed changes

artemredkin force-pushed the custom_transport_protection_algs_support branch 2 times, most recently from 9a27935 to e312215 Compare April 15, 2022 10:11

artemredkin force-pushed the custom_transport_protection_algs_support branch from e312215 to 2ced9ad Compare April 29, 2022 18:33

artemredkin force-pushed the custom_transport_protection_algs_support branch from 2ced9ad to 0ac56ea Compare June 28, 2022 13:19

artemredkin force-pushed the custom_transport_protection_algs_support branch from 0ac56ea to 0987147 Compare July 14, 2022 13:57

artemredkin force-pushed the custom_transport_protection_algs_support branch 2 times, most recently from 43d91e4 to c776967 Compare August 16, 2022 13:06

artemredkin force-pushed the custom_transport_protection_algs_support branch from c776967 to 01e5a89 Compare October 13, 2022 15:28

Lukasa approved these changes Oct 25, 2022

View reviewed changes

Lukasa merged commit d6940e9 into apple:main Oct 25, 2022

artemredkin mentioned this pull request Nov 22, 2022

Refactoring of the encryptPacket method #123

Merged

Lukasa added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Nov 23, 2022

		static let version = "SSH-2.0-SwiftNIOSSH_1.0"

		public static let defaultTransportProtectionSchemes: [NIOSSHTransportProtection.Type] = [

Adds support for custom transport protection algorithms #97

Adds support for custom transport protection algorithms #97

Conversation

artemredkin commented Oct 13, 2021 • edited

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

Joannis left a comment

Choose a reason for hiding this comment

artemredkin commented Apr 1, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

artemredkin commented Oct 13, 2021 •

edited