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

Added SshCommand.InputStream to allow writing to stdin of SshCommand #1293

Merged
merged 12 commits into from
Feb 6, 2024

Conversation

realvizu
Copy link
Contributor

@realvizu realvizu commented Jan 23, 2024

Changes:

  • Added SshCommand.InputStream (type: ChannelInputStream), which sends the data written to the stream through a command's channel.
  • Added an integration test that echos back the sent data using the "cat" command.
  • All unit tests pass.

I you have recommendations for further unit tests, please advise.


I started the work by trying to run all unit tests on my dev machine to see if they all pass. Unfortunately a lot of them failed, so I had to make the following modifications (see the changes in my first commit in this PR):

  • Excluded MD5 tests for the net462 target, because I got System.InvalidOperationException: 'This implementation is not part of the Windows Platform FIPS validated cryptographic algorithms.'
  • Modified SshdConfig.cs: do not throw for "Include", just do nothing.
  • Modified failing dos2unix parameters in Dockerfile.TestServer.
  • Added a line to .gitattributes to force LF line ending for key files used by integration tests, otherwise using them causes error.
  • Modified SftpClientTest.Test_Sftp_Multiple_Async_Upload_And_Download_10Files_5MB_Each to upload only 2 files instead of 10 because for more than 2 files I got timeout errors.

If you know a better fix for any of the above modifications, please advise.

ferenc.vizkeleti added 3 commits January 23, 2024 12:23
Excluded MD5 tests on net462 because I get System.InvalidOperationException: 'This implementation is not part of the Windows Platform FIPS validated cryptographic algorithms.'
SshdConfig: do not throw for "Include", just do nothing.
Modified failing dos2unix parameters in Dockerfile.TestServer.
Forceing LF line ending for key files used by integration tests, otherwise using them causes error.
SftpClientTest.Test_Sftp_Multiple_Async_Upload_And_Download_10Files_5MB_Each times out for maxFiles=10, decreasing to 2 to make the test pass.
@WojciechNagorski
Copy link
Collaborator

It looks good to me! But I need more time to test it!

Do you have a specific function in mind that you want to implement using this functionality?
Did you see that it can be useful in dotnet/Docker.DotNet#540

@WojciechNagorski
Copy link
Collaborator

image
Those tests work on my machine.

Copy link
Collaborator

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

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

I don't think we should send channel EOF in Write. How about calling it only in Dispose and initialising InputStream lazily, something like:

var cmd = ...
using (cmd.InputStream) // This initialises InputStream
{
    myStream.CopyTo(cmd.InputStream);
} // This disposes InputStream and sends EOF

?

src/Renci.SshNet/Common/ChannelInputStream.cs Outdated Show resolved Hide resolved
test/Renci.SshNet.TestTools.OpenSSH/SshdConfig.cs Outdated Show resolved Hide resolved
test/Renci.SshNet.Tests/Renci.SshNet.Tests.csproj Outdated Show resolved Hide resolved
ferenc.vizkeleti added 3 commits January 29, 2024 09:30
… SshCommand.InputStream with CreateInputStream to emphasise that a (disposable) resource is created here. EndExecute also closes the _inputStream to make sure that EOF is sent (in case the user forgot to dispose the input stream). Added more unit tests: sending the input one byte at a time, not disposing the input stream, calling CreateInputStream before BeginExecute or AfterEndExecute throws exception.
@realvizu
Copy link
Contributor Author

It looks good to me! But I need more time to test it!

Do you have a specific function in mind that you want to implement using this functionality? Did you see that it can be useful in dotnet/Docker.DotNet#540

I had a very specific use case: I had to call some legacy system that is implemented as a set of SSH commands and one of the commands requires sending a file via input stream. These changes made it work.

@realvizu
Copy link
Contributor Author

Thanks a lot @WojciechNagorski and @Rob-Hague for the comments and suggestions!

  • I reverted all the FIPS-related changes and the change made to SftpClientTest.Upload.cs.
  • I moved the EOF sending to Dispose and added a unit test that validates that now multiple calls to Write works as expected.
  • I've adapted @Rob-Hague 's suggestion about making InputStream lazily initialized with some modification. I wanted to make it more apparent that a resource is created here (that should be disposed after using), so I changed the InputStream from a property to a CreateInputStream method.
  • Also, because the input stream uses the channel created in BeginExecute and closed in EndExecute, I added a check that throws invalid operation exception if CreateInputStream is called when there's no channel to use. Also added some unit tests for this.

Copy link
Collaborator

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

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

Nice

src/Renci.SshNet/Common/ChannelInputStream.cs Outdated Show resolved Hide resolved
src/Renci.SshNet/Common/ChannelInputStream.cs Outdated Show resolved Hide resolved
src/Renci.SshNet/Common/ChannelInputStream.cs Outdated Show resolved Hide resolved
src/Renci.SshNet/SshCommand.cs Outdated Show resolved Hide resolved
@realvizu
Copy link
Contributor Author

realvizu commented Feb 2, 2024

Shall I do a squash+rebase onto develop before merging the PR?

@Rob-Hague
Copy link
Collaborator

It will be merged with "Squash and merge" so it is not necessary

(it requires a CODEOWNERS approval before it can merge)

@WojciechNagorski
Copy link
Collaborator

There is a build problem:

C:\projects\ssh-net\test\Renci.SshNet.IntegrationTests\SshClientTests.cs(37,33): error CS7036: There is no argument given that corresponds to the required parameter 'offset' of 'Stream.Write(byte[], int, int)' [C:\projects\ssh-net\test\Renci.SshNet.IntegrationTests\Renci.SshNet.IntegrationTests.csproj::TargetFramework=net48]

@WojciechNagorski WojciechNagorski merged commit 2d0e03b into sshnet:develop Feb 6, 2024
1 check passed
@WojciechNagorski
Copy link
Collaborator

This issue has been fixed in the 2024.0.0 version.

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