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 a ChannelOption for a server to query the authenticated username. #58

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

Conversation

jimstudt
Copy link

Some servers will need to know which user authenticated. This PR exposes that information through a ChannelOption named SSHChildChannelOptions.Types. UsernameOption which is a String? of the username if authenticated, otherwise nil.

The new code passes the username along with the chain of possible completions from an authentication request until it reaches the creation of the .userAuthSuccess message. This message is augmented with an associated String? to hold the username. Eventually, this message will arrive at the SSHConnectionStateMachine, where it will be recorded.

The NIOSSHHandler contains the SSHConnectionStateMachine, and there is a parent path reaching up from the SSHChildHandler to the NIOSSHHandler. This path is festooned with var username:String? properties to allow
the SSHChildHandler to reach up and back down to get the username when the UsernameOption is requested.

I wonder a bit about adding the associated type to the .userAuthSuccess SSHMessage, but most of them have associated
data already. I had to add explicit nil values 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 in NIOSSHHandler. It was already being done in SSHConnectionStateMachine, but there isn't a parent link back up to set the value in NIOSSHHandler.

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 little
short of understanding the BackToBackEmbeddedChannel test harness. I've been testing the PR from my SSHConsole
project's echo server and it appears to work fine.

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.
@swift-server-bot
Copy link

Can one of the admins verify this patch?

3 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

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

Thanks for this patch! This looks like a good start! I've left a few notes in the diff about how we might want to take this forward.

Copy link
Author

@jimstudt jimstudt left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@Joannis Joannis left a comment

Choose a reason for hiding this comment

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

Looks like a good set of changes to me!

@Joannis
Copy link
Collaborator

Joannis commented Nov 17, 2022

@Lukasa can we merge this?

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.

4 participants