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

Support an SSH (SFTP) server #16

Merged
merged 25 commits into from
Nov 11, 2022
Merged

Support an SSH (SFTP) server #16

merged 25 commits into from
Nov 11, 2022

Conversation

Joannis
Copy link
Member

@Joannis Joannis commented Sep 20, 2022

No description provided.

@Joannis Joannis requested a review from JaapWijnen September 20, 2022 21:10
func channelRead(context: ChannelHandlerContext, data: NIOAny) {
let message = unwrapInboundIn(data)

self.logger.trace("SFTP IN: \(message.debugDescription)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this logging line no longer needed in the renamed SFTPClientInboundHandler? (It seems like a copy and rename and this is the only line that is missing in that new file)

@@ -126,6 +126,11 @@ public struct SFTPFileAttributes: CustomDebugStringConvertible {
// let extended_count: UInt32?

public static let none = SFTPFileAttributes()
public static let all: SFTPFileAttributes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer a more descriptive name since SFTPFileAttributes holds more than readwrite permissions so .all could be a little confusing in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should really be implementing this for permissions: http://learn.leighcotnoir.com/wp-content/uploads/2016/08/POSIX-file-permissions.pdf as optionset . Then we don't need this .all

@@ -126,6 +126,11 @@ public struct SFTPFileAttributes: CustomDebugStringConvertible {
// let extended_count: UInt32?
Copy link
Contributor

Choose a reason for hiding this comment

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

These two TODO's above are already public api?

@@ -286,13 +320,26 @@ public enum SFTPMessage {
/// No response, directory gets created or an error is thrown.
case mkdir(MkDir)

case stat(Stat)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally some nice comments like the other enums :) (Since I don't immediately know what stat and lstat are or what attributes we're talking about here)

Sources/Citadel/SFTP/SFTPMessage.swift Show resolved Hide resolved
)
}
case .attributes(let command):
print(command)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here for print statement

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a server-response type, so it cannot be received by a server

Copy link
Member Author

Choose a reason for hiding this comment

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

This should throw an error

Sources/Citadel/Server.swift Show resolved Hide resolved
Sources/Citadel/Server.swift Show resolved Hide resolved
}
}

final class CitadelServerDelegate {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a new and non protocol version of the removed SSHServerDelegate ? Won't it need those same protocol conformances that used to have or is the similarity just coincidence?

Sources/Citadel/Terminal/SSHTerminal.swift Show resolved Hide resolved
switch event {
case let event as SSHChannelRequestEvent.SubsystemRequest:
switch event.subsystem {
case "sftp":
Copy link
Member Author

@Joannis Joannis Sep 21, 2022

Choose a reason for hiding this comment

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

Change to internal enum with supported protocols

return XCTFail()
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Assertion, testdata.readablebytes == 0?

import BigInt

enum SSHServerError: Error {
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove this

@Joannis Joannis merged commit 6bd1d78 into main Nov 11, 2022
@Joannis Joannis deleted the feature/ssh-server branch November 11, 2022 09:25
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.

2 participants