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 inShell parameter to for SSH command execution #36

Merged
merged 13 commits into from
May 28, 2023

Conversation

Marcocanc
Copy link
Contributor

@Marcocanc Marcocanc commented May 22, 2023

This PR adds the option to execute commands in a shell, by launching a shell channel request and sending the command into the channel once the request succeeded.

@Marcocanc Marcocanc mentioned this pull request May 22, 2023
Copy link
Member

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

Looking good! Let's see if we can clean up these final bits. My main remaining question is if we should allow people to choose a shell (e.g. /bin/sh, /bin/bash, /bin/zsh)

Sources/Citadel/TTY/Client/TTY.swift Outdated Show resolved Hide resolved
Sources/Citadel/TTY/Client/TTY.swift Outdated Show resolved Hide resolved
Sources/Citadel/TTY/Client/TTY.swift Outdated Show resolved Hide resolved
@Joannis
Copy link
Member

Joannis commented May 22, 2023

Also nice to see a PR of yours! We're using Cilicon ourselves, so thank you for that :)

@Marcocanc
Copy link
Contributor Author

Thanks for the review, this was just a super rough PoC with a bunch of copy-paste. Will clean it up tomorrow!

We're using Cilicon ourselves

Great to hear! Citadel is making its way into Cilicon in an exciting 2.0 update soon 😊

@Marcocanc
Copy link
Contributor Author

Looking good! Let's see if we can clean up these final bits. My main remaining question is if we should allow people to choose a shell (e.g. /bin/sh, /bin/bash, /bin/zsh)

I don't think SSH allows picking a shell. If I understood it correctly it will always uses the default shell of the logged in user.
In the spec it says:

This message will request that the user's default shell (typically defined in /etc/passwd in UNIX systems) be started at the other end.

@Joannis
Copy link
Member

Joannis commented May 23, 2023

@Marcocanc since this executes a command as the shell launches, I believe we can do something like /bin/sh -c, but I don't know if it's any added value

@Marcocanc
Copy link
Contributor Author

@Marcocanc since this executes a command as the shell launches, I believe we can do something like /bin/sh -c, but I don't know if it's any added value

Yeah I don't see much value in that tbh. After all users can just wrap their command that way themselves.

@Joannis
Copy link
Member

Joannis commented May 24, 2023

Fair! Then I'm pretty happy with this

@Marcocanc Marcocanc changed the title Proposal: Add executeInShell Add inShell parameter to for SSH command execution May 24, 2023
@Marcocanc Marcocanc marked this pull request as ready for review May 24, 2023 12:33
Copy link
Member

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

I dig this! Thanks for the PR! I f you don't mind, I want to let this sit for a day or so to review it in the back of my head

Sources/Citadel/TTY/Client/TTY.swift Show resolved Hide resolved
let commandData = SSHChannelData(type: .channel,
data: .byteBuffer(ByteBuffer(string: command + ";exit\n")))
channel.writeAndFlush(commandData, promise: nil)
hasReceivedChannelSuccess = true
Copy link
Member

Choose a reason for hiding this comment

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

My one nit is that hasReceivedChannelSuccess should be always set, otherwise if we access that variable in a later feature down the line it will cause confusion

Copy link

Choose a reason for hiding this comment

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

I’m wondering why “exit” is always called after user’s command?

Copy link
Member

Choose a reason for hiding this comment

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

@xuxulll because this is a helper that only executes a single command.

Copy link

Choose a reason for hiding this comment

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

@Joannis thanks for the explanation, do you have any plan to support interactive shell?

Sources/Citadel/TTY/Client/TTY.swift Outdated Show resolved Hide resolved
@Joannis Joannis merged commit d731663 into orlandos-nl:main May 28, 2023
@Marcocanc
Copy link
Contributor Author

@Joannis Cilicon 2.0 (which includes Citadel) is now released 😊 https://github.com/traderepublic/Cilicon

@Joannis
Copy link
Member

Joannis commented Jun 14, 2023

Cool! Thanks for the heads up @Marcocanc

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.

3 participants