-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
Development of EchoShell
- A simple shell emulator
#39
Conversation
…ser input and offers some basic commands like: help, history, clear, whoami, date and exit.
@@ -32,6 +33,7 @@ let package = Package( | |||
.product(name: "_CryptoExtras", package: "swift-crypto"), | |||
.product(name: "BigInt", package: "BigInt"), | |||
.product(name: "Logging", package: "swift-log"), | |||
.productItem(name: "ColorizeSwift", package: "ColorizeSwift") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand the desire to import this library - I don't want to add a dependency for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then I'll embed the necessary code parts of the library and mention the author above?
import Crypto | ||
import NIOSSH | ||
|
||
public extension NIOSSHPrivateKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These helpers cannot go in the library. But you can add them to your extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you consider to add this as an default directly to NIOSSHPrivateKey
?
import NIOSSH | ||
|
||
public extension NIOSSHPrivateKey { | ||
init(file: URL = FileManager.default.homeDirectoryForCurrentUser.appendingPathComponent("citadel_ssh_host_key")) throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URL should not use this default. It's best to leave the defaults empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
// Basic Commands | ||
// Each command must return an array of UINT8 or nil. Streams are currently not supported. | ||
|
||
public protocol Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand and agree with the desire for these helpers and standardisation, I don't think this can be used as public API in Citadel itself. Though feel free to add it as part of the Example project for now.
I think they're very helpful, but would need to go in their own module - separate from the Citadel
module itself. Though they can be in the same repo.
public protocol Command { | ||
var name: String { get } | ||
var description: String { get } | ||
func exec(input: [UInt8], session: EchoShellMaster) async throws -> [UInt8]? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we merge it as public API (so outside of an example), this exec
function signature ha to change.
First of all, the EchoShellMaster
type is very much an 'example' name. If we can rename this to CommandContext
that'd be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input
should also be more helpful than [UInt8]
. The 'bag of bytes' type we use in NIO (ByteBuffer) would definitely be preferred over this. But I think wrapping that ByteBuffer into a CommandInput
type is better. That way we can add helpers.
public struct CommandInput: Sendable {
private var buffer: ByteBuffer
}
extension CommandInput {
func readString() -> String? {
return buffer.getString(at: buffer.startIndex, length: buffer.readableBytes)
}
}
We should ensure there's room for improvement here, so that we can accept streams of data as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to accepting streams of data, we should also allow the func exec
to emit a stream of data. Or write data to the client in some way. Because you might have flexible input over time, or a form of subprocesses-commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with changing the name to CommandContext
and offering CommandInput
with the ByteBuffer.
I personally tend to avoid working with strings at cmd-line level as basically everything is within the UTF-8/ASCI range and String
just adds a lot of unnecessary overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with changing the return to an AsyncStream
.
private var previous_length: Int = 0 | ||
|
||
// Adds the input if it is not equal to the last one. This makes it easier to search the history. | ||
mutating func add_line(_ line: [UInt8]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can make a typealias
here for a Line
. Or a separate type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we agree on [UINT8]
or ByteBuffer
? I do like that you directly can iterate over [UINT8]
and use all array/collection defaults. Are they available for ByteBuffer
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ByteBuffer
, but you can use the readableBytesView
in ByteBuffer for that
mutating func go_back(currentLineCount: Int) -> ([UInt8], [UInt8])? { | ||
let index = (history.count - 1) - (pointer + 1) | ||
|
||
if index >= 0 && index <= history.count - 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this if
can be turned into a guard
. that way we keep the code's control flow clean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea.
return nil | ||
} | ||
|
||
mutating func go_forward(currentLineCount: Int) -> ([UInt8], [UInt8])? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make a type for this tuple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
} | ||
|
||
// `Zero` is needed to reset the internal pointer when pressing the Enter key. | ||
mutating func zero() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mutating func return()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the enter
key is the return
key. Hence that CR character is Carriage Return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, zero()
is named zero
for convenience reasons, as it sets the counters to original (zero) state, but I'm fine with return()
.
|
||
import Foundation | ||
|
||
public struct Terminal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal struct ControlCharacter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea. Would keep it public though. Some users might want to create their own commands and having access to the set of control character might be very helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair
f946d9d
to
ddae52b
Compare
Development of a shell emulator
EchoShell
that outputs the he user input and offers some basic commands like:help
,history
,clear
,whoami
,date
andexit
. In addition, a host key file handler was added.The
ShellDelegate
andSSHShellContext
changes provided by the repo owner have been adapted.