Support unknown global request types #18

Joannis · 2020-05-08T07:53:04Z

Fixed #17

Motivation:

Global requests come in many flavours, but are by no means a definitive list. The list is also not exhaustively populated by requests to forward a TCP socket. Especially the OpenSSH implementation(s) have a lot of custom global requests. More on that here.

Before this change, an unknown global request would throw an error which would be caught by the NIO.Channel wrapping this (global) SSH connection. This in turn tends to close the connection, which closes all running operations as well.

Modifications:

This PR tackles the above problems by introducing a new GlobalRequestMessage "unknown". The unknown case contains the message type and the remainder of the payload. Since the payload's format changes depending on the message type, there is no way to parse this payload into something immediately useful.

Instead, this unknown case can be ignored if the server does not want a reply. If it does want a reply, it's necessary to reply with SSH_MSG_REQUEST_FAILURE.

Result:

The message is not ignored, instead it can be handled by receiveGlobalRequest where future updates can allow extension of the SSH protocol through here. No errors are thrown to the channel, which (potentially) leads to a closed TCP connection. Instead, the appropriate SSH_MSG_REQUEST_FAILURE is returned to the sender.

TODO:

I'll need to have a test that checks whether the sender has, in fact, received SSH_MSG_REQUEST_FAILURE
Verify that the entire SSH packet's payload is read when reading a global request

Motivation: SSH would throw errors when an unknown global request was received, and the message would be ignored (in violation of spec). Solution: Now, a message of SSH_MSG_REQUEST_FAILURE will be returned to the sender, and the unknown message is passed through far enough that potential future implementations could handle custom global requests.

swift-server-bot · 2020-05-08T07:53:06Z

Can one of the admins verify this patch?

swift-server-bot · 2020-05-08T07:53:06Z

Can one of the admins verify this patch?

swift-server-bot · 2020-05-08T07:53:06Z

Can one of the admins verify this patch?

swift-server-bot · 2020-05-08T07:53:06Z

Can one of the admins verify this patch?

… requests

This adds (passing) tests for parsing and serialization. It also adds a broken test to verify that a reply of type SSH_MSG_REQUEST_FAILURE was received.

Joannis · 2020-05-09T11:09:07Z

@Lukasa I could use a review on this PR. But in addition to a review, there is one broken test that needs to verify that SSH_MSG_REQUEST_FAILURE was received by the sender of an unknown packet wanting a reply. I'm unable to figure out how to check for this reply architecturally. I also have a gut feeling that we need to handle receiving such a message (SSH_MSG_REQUEST_FAILURE)

Lukasa

I have a few notes, but basically this patch looks great. I think your various test cases are already covered, so I don't think you have anything to do here: feel free to promote this into a full PR.

Sources/NIOSSH/NIOSSHHandler.swift

Sources/NIOSSH/GlobalRequestDelegate.swift

Sources/NIOSSH/SSHMessages.swift

Lukasa · 2020-05-11T06:50:20Z

@swift-server-bot add to whitelist

Joannis · 2020-05-11T06:59:37Z

@Lukasa I assume that it looks great if the test succeeds, right? Or do you believe that test to be unnecessary? I appreciate the nitpicking, it's clever to preface those the way you did. I'll take that page out of your book :)

Lukasa · 2020-05-11T07:00:17Z

Haha yes, naturally the CI needs to pass, but the code doesn't seem structurally wrong.

Lukasa · 2020-05-11T07:00:54Z

07:51:40 Test Case 'SSHConnectionStateMachineTests.testUnknownGlobalRequestCanTriggerResponse' started at 2020-05-11 06:51:40.849
07:51:40 /code/Tests/NIOSSHTests/SSHConnectionStateMachineTests.swift:196: error: SSHConnectionStateMachineTests.testUnknownGlobalRequestCanTriggerResponse : failed - Unexpected result: Optional(NIOSSH.SSHConnectionStateMachine.StateMachineInboundProcessResult.globalRequest(NIOSSH.SSHMessage.GlobalRequestMessage(wantReply: true, type: NIOSSH.SSHMessage.GlobalRequestMessage.RequestType.unknown("test", ByteBuffer { readerIndex: 0, writerIndex: 12, readableBytes: 12, capacity: 12, slice: _ByteBufferSlice { 40..<52 }, storage: 0x00005643189f7ca0 (64 bytes) }))))
07:51:40 /code/Tests/NIOSSHTests/SSHConnectionStateMachineTests.swift:355: error: SSHConnectionStateMachineTests.testUnknownGlobalRequestCanTriggerResponse : failed - No reply was received, where a reply was requested
07:51:40 /code/Tests/NIOSSHTests/SSHConnectionStateMachineTests.swift:196: error: SSHConnectionStateMachineTests.testUnknownGlobalRequestCanTriggerResponse : failed - Unexpected result: Optional(NIOSSH.SSHConnectionStateMachine.StateMachineInboundProcessResult.globalRequest(NIOSSH.SSHMessage.GlobalRequestMessage(wantReply: false, type: NIOSSH.SSHMessage.GlobalRequestMessage.RequestType.unknown("test", ByteBuffer { readerIndex: 0, writerIndex: 12, readableBytes: 12, capacity: 12, slice: _ByteBufferSlice { 40..<52 }, storage: 0x00005643189f7ca0 (64 bytes) }))))
07:51:40 Test Case 'SSHConnectionStateMachineTests.testUnknownGlobalRequestCanTriggerResponse' failed (0.01 seconds)

This changes the global request implementation to support sending unknown global requests and changes the exposed types to support these cases as well. In addition, the pending global requests can now receive unknown responses and emit failures.

Lukasa

I don't strongly object to this change, but I think we need to refactor the code internally. There are more promises flowing around than we need. We only need a promise when the user is giving us something that they want to be notified on completion of. As we see all inbound and outbound messages, we never need promises internally, and so we shouldn't allocate them.

Sources/NIOSSH/NIOSSHHandler.swift

Joannis · 2020-05-11T12:46:26Z

I strongly agree with your architectural vision without promises, especially for the internals. The vision that's consistently present throughout this project is one I look forward to applying elsewhere.

Reflecting back upon this bit of code, I'd recommend making the helper initializer on SSHMessage.RequestSuccessMessage to throw an error if the buffer.readableBytes > 0 by the end of reading that message. What do you think?

Expose the GlobalRequestResponse type publicly whilst allowing future additions to the API. Fixed the bugs to pass the tests and updates the code according to feedback with the exception of one promise allocation.

Change the internal storage of the global requests queue to support a variety of expected successful responses

Joannis · 2020-05-11T15:37:48Z

All the tests succeed locally. As I don't have access to the CI, I cannot see why the first test failed.

Lukasa · 2020-05-11T15:39:22Z

The sanity builder is just running scripts/sanity.sh, so you can try that.

Joannis · 2020-05-11T18:03:46Z

sanity.sh doesn't emit any errors on my machine. Just => Checking format... %.
I did notice that the script has a few copyright headers that contain the YEARS placeholder instead of the actual years it's applicative to. I think the regex replaces that, I just skimmed over it. But it's an FYI, in case that's not what it does.

Joannis · 2020-05-12T10:13:25Z

For reference: PR #24 is what I ran into, and should prevent people from running into the same issue later on.

Lukasa

Great, looking really good. A few notes here and there, nothing substantial, just minor code structure feedback.

Sources/NIOSSH/GlobalRequestDelegate.swift

Sources/NIOSSH/NIOSSHHandler.swift

Sources/NIOSSH/SSHMessages.swift

Update TCPForwardingResponse's comment to match the new situation. Make use of the `fail` helper on a pending global request in dropAllPendingGlobalRequests. Mak the RequestSuccessMessage internal again. Remove an redundant line of documentation.

Lukasa

Cool, looks really good here. Two minor notes.

Sources/NIOSSH/NIOSSHHandler.swift

Sources/NIOSSH/SSHMessages.swift

Lukasa

Great, ok, we're really close here. I have a tiny nit in the diff and we need to fix up the merge conflict. When that's done I'll do a sanity review to confirm the merge went ok, and then we should be ready to go!

Sources/NIOSSH/SSHMessages.swift

Co-authored-by: Cory Benfield <[email protected]>

Lukasa

Awesome, looks fantastic. Thanks so much for this! ✨ 🍰

Joannis marked this pull request as draft May 8, 2020 07:59

Joannis added 2 commits May 8, 2020 10:13

Fully consume the incoming packet payload when reading unknown global…

5d96401

… requests

Added tests for the changes global requests functionality

e8762c6

This adds (passing) tests for parsing and serialization. It also adds a broken test to verify that a reply of type SSH_MSG_REQUEST_FAILURE was received.

Lukasa requested changes May 11, 2020

View reviewed changes

Sources/NIOSSH/NIOSSHHandler.swift Outdated Show resolved Hide resolved

Sources/NIOSSH/NIOSSHHandler.swift Outdated Show resolved Hide resolved

Joannis added 2 commits May 11, 2020 14:50

Updated according to feedback and fixed the code to pass the tests

71d5ac8

Expose the GlobalRequestResponse type publicly whilst allowing future additions to the API. Fixed the bugs to pass the tests and updates the code according to feedback with the exception of one promise allocation.

Support a variety of global request response types

b5b78e8

Change the internal storage of the global requests queue to support a variety of expected successful responses

Joannis marked this pull request as ready for review May 11, 2020 15:36

Joannis requested a review from Lukasa May 11, 2020 15:36

Fix the formatting using sanity.sh

6b8fbad

Joannis changed the title ~~[WIP] Support unknown global request types~~ Support unknown global request types May 12, 2020

Lukasa requested changes May 12, 2020

View reviewed changes

Joannis added 2 commits May 12, 2020 15:28

Update according to Cory's feedback

30c2232

Update TCPForwardingResponse's comment to match the new situation. Make use of the `fail` helper on a pending global request in dropAllPendingGlobalRequests. Mak the RequestSuccessMessage internal again. Remove an redundant line of documentation.

Add a succeed function on PendingGlobalRequestResponse

5657652

Joannis requested a review from Lukasa May 12, 2020 14:04

Make the default implementation public

d07d4eb

Lukasa requested changes May 13, 2020

View reviewed changes

Sources/NIOSSH/NIOSSHHandler.swift Outdated Show resolved Hide resolved

Sources/NIOSSH/SSHMessages.swift Outdated Show resolved Hide resolved

Deduplicate serialization of TCPForwardingResponse in NIOSSHHandler

aff56f4

Joannis requested a review from Lukasa May 13, 2020 08:26

Lukasa requested changes May 13, 2020

View reviewed changes

Sources/NIOSSH/SSHMessages.swift Outdated Show resolved Hide resolved

Joannis and others added 2 commits May 13, 2020 10:36

Update Sources/NIOSSH/SSHMessages.swift

5cad352

Co-authored-by: Cory Benfield <[email protected]>

Merge branch 'master' into jo-unknown-global-requests

546f78f

Joannis requested a review from Lukasa May 13, 2020 08:37

Lukasa approved these changes May 13, 2020

View reviewed changes

Lukasa merged commit 1862954 into apple:master May 13, 2020

Support unknown global request types #18

Support unknown global request types #18

Uh oh!

Conversation

Joannis commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-server-bot commented May 8, 2020

Uh oh!

swift-server-bot commented May 8, 2020

Uh oh!

swift-server-bot commented May 8, 2020

Uh oh!

swift-server-bot commented May 8, 2020

Uh oh!

Joannis commented May 9, 2020

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lukasa commented May 11, 2020

Uh oh!

Joannis commented May 11, 2020

Uh oh!

Lukasa commented May 11, 2020

Uh oh!

Lukasa commented May 11, 2020

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Joannis commented May 11, 2020

Uh oh!

Joannis commented May 11, 2020

Uh oh!

Lukasa commented May 11, 2020

Uh oh!

Joannis commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Joannis commented May 12, 2020

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Joannis commented May 8, 2020 •

edited

Loading

Joannis commented May 11, 2020 •

edited

Loading