Add a ChannelOption for a server to query the authenticated username. #58
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A server may wish to know which user authenticated to decide what they are authorized to do. Modifications: - On an authentication request, keep the username until it can be associated with a userAuthSuccess message. - Once that userAuthSuccess message reaches the SSHConnectionStateMachine, record the successful username. - Carve an access path from a SSHChildChannel up through its multiplexor, to the NIOSSHHandler, and back down to the SSHConnectionStateMachine to get the username. - Implement a ChannelOption in SSHChildChannel for ChannelHandlers to query their username.
|
Can one of the admins verify this patch? |
3 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
Lukasa
reviewed
Nov 27, 2020
Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift
Outdated
Show resolved
Hide resolved
Sources/NIOSSH/Connection State Machine/Operations/AcceptsUserAuthMessages.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: Cory Benfield <lukasa@apple.com>
…hSuccess message.
…ection attributes. Use it to collect the authenticated username before that name is obliterated.
jimstudt
commented
Nov 27, 2020
There was a problem hiding this comment.
This touches a lot of files, but mostly that's just adding it to each state.
I noticed a couple comments about TODO work needing to limit the number of attempts for various activities, this 'connectionAttributes' area would be an easy place to implement a counter for those in the future.
Joannis
reviewed
Dec 8, 2020
|
@Lukasa can we merge this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some servers will need to know which user authenticated. This PR exposes that information through a
ChannelOptionnamedSSHChildChannelOptions.Types. UsernameOptionwhich is aString?of the username if authenticated, otherwise nil.The new code passes the
usernamealong with the chain of possible completions from an authentication request until it reaches the creation of the.userAuthSuccessmessage. This message is augmented with an associatedString?to hold the username. Eventually, this message will arrive at theSSHConnectionStateMachine, where it will be recorded.The
NIOSSHHandlercontains theSSHConnectionStateMachine, and there is a parent path reaching up from theSSHChildHandlerto theNIOSSHHandler. This path is festooned withvar username:String?properties to allowthe
SSHChildHandlerto reach up and back down to get the username when theUsernameOptionis requested.I wonder a bit about adding the associated type to the
.userAuthSuccessSSHMessage, but most of them have associateddata already. I had to add explicit
nilvalues wherever they are created without a username.I could have stored the actual username directly in the
NIOSSHHandler, but that would have required adding a per-message inspection inNIOSSHHandler. It was already being done inSSHConnectionStateMachine, but there isn't a parent link back up to set the value inNIOSSHHandler.I didn't find a test which sets up a server and checks its behavior in order to write tests. I note that the other
ChannelOptions are also untested and suspect that has something to do with it. My knowledge of NIO stops a littleshort of understanding the
BackToBackEmbeddedChanneltest harness. I've been testing the PR from my SSHConsoleproject's
echoserver and it appears to work fine.