ShellStream does not currently override the Read/Write async variants. They fall back to
the base class implementations which run the sync variants on a thread pool thread, only
allowing one call of either at a time in order to protect implementations that would
break if Read/Write were called simultaneously. In ShellStream, reads and writes are
independent so mutually excluding their use is unnecessary and can lead to effective
deadlocks.
We therefore override WriteAsync to get around this restriction. We do not override
ReadAsync because the sync implementation does not lend itself well to async given the
use of Monitor.Wait/Pulse. Note that while reading and writing simultaneously is allowed,
it is not intended that ShellStream is used with multiple simultaneous reads or multiple
simultaneous writes, so it is fine to keep the base one-at-a-time implementation on
ReadAsync.
Another note is that the new WriteAsync will be simple (synchronous) buffer copying in
most cases, with a call to FlushAsync in others. We also do not override FlushAsync, so
that will go onto a thread pool thread and potentially acquire some locks. But given that
the current base implementation of WriteAsync does that unconditionally, it makes the new
WriteAsync slightly better and certainly no worse than the current version.
This adds an SftpException which sits between the existing SftpPathNotFoundException/
SftpPermissionDeniedException and SshException, and which contains the response code
from the SSH_FXP_STATUS packet, along with a default message if one was not provided.
SftpPathNotFoundException also gains a Path property which is populated in cases where
it makes sense.
* Build the read-ahead mechanism into SftpFileStream
This change unifies the SFTP download implementations that exist via DownloadFile and
via SftpFileStream, by rewriting SftpFileStream to perform the same "read-aheads" as
DownloadFile. This brings the performance of downloads via SftpFileStream in line with
DownloadFile, such that the latter is now effectively SftpFileStream.CopyTo. It also
brings the recently added DownloadFileAsync up to speed since that was implemented via
SftpFileStream.CopyToAsync.
The methodology is a mix of the previous one and that within OpenSSH: the first call to
SftpFileStream.Read sends one read request to the server. The second sends two and when
not interrupted by Write or similar, the number of in-flight read requests continues to
scale up in this fashion.
I have measured CopyTo to be 3-20x faster than before, depending on file size and server
round-trip time.
* Check CanSeek in ReadAllBytes
* Squeeze out some performance
* Avoid rounding issues when checking Timeout values (#1700)
AsTimeout is called from the SshCommand constructor with
Timeout.InfiniteTimeSpan. In this scenario the range check should never
fail, but unfortunately it does in certain scenarios, due to a runtime
or compiler bug (as soon as optimizations are turned off the issue
miraculously disappears).
Closes#1700
* fix tests
---------
Co-authored-by: Robert Hague <rh@johnstreetcapital.com>
* CI: add Windows Integration Tests for .NET
see https://github.com/sshnet/SSH.NET/pull/1702#issuecomment-3342506642
* fix podman setup with Windows and .NET
* debug
* x
* x
* x
* revert
* Run Windows .NET tests in separate job
so they run in parallel and we avoid the Common_CreateMoreChannelsThanMaxSessions test failure.
* fix coverlet artifacts
* fix missing PermitTTY in RemoteSshdConfig Reset
this fixes a test failure in Common_CreateMoreChannelsThanMaxSessions
when running the tests multiple times against the same SSH server
instance.
see https://github.com/sshnet/SSH.NET/pull/1704#issuecomment-3343210311
* speed up Windows tests
turns out this is caused by DNS resolution taking about
2 seconds on every new connection...
* add windows integration tests to Publish needs:
---------
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
Co-authored-by: Robert Hague <rh@johnstreetcapital.com>
* Fix SftpFileAttributes file type detection
To get the file type, S_IFMT should be used as the mask. Instead it was using each file
type as the mask. It meant that e.g. a symbolic link would also show as a regular file
and a character device.
Also allow setting and retrieving the setuid/setgid/sticky bits
* fix build
The message loop currently sits in a call to Poll until the socket has data to read or
it is closed. This is unnecessary - it can equally just sit in the call to Receive.
The call to Poll in Session.IsConnected is also unnecessary - we can instead just call
Socket.Connected. This only returns the connection state as of the last operation, but
we are always performing operations in the message loop (or else we are not connected),
so it should work equally well while being cheaper.
Lastly, when shutting down the socket, shut down both sides rather than just the sending
side (SocketShutdown.Both rather than SocketShutdown.Send) - at this point we do not care
about reading anything else. This makes it (more) certain that we will break out of the
Receive call in the message loop, as has been noted in #355 for whatever remaining issues
still exist there.
* Bump actions/checkout from 4 to 5
* Bump actions/upload-pages-artifact from 3 to 4
* Bump actions/download-artifact from 4 to 5
* Bump the dependencies group with 5 updates
* fix
* fix locally failing portforwarding tests
port already in use
---------
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
When the key exchange produces less key material than is needed for the cipher or hmac
algorithms, there is an iterative procedure to produce more.
For example, a SHA-1 key exchange algorithm produces 20 bytes of key material.
A SHA-256 hmac uses a 32 byte key, so one iteration of the procedure produces another
20 bytes of key material for a total of 40 which is sufficient for the hmac key.
The library works correctly in such cases of one iteration, but the logic is wrong if
more than one iteration is needed. In #1660, the connection uses a SHA-1 kex algorithm
with a SHA-512 hmac (64 byte key), requiring 3 iterations of the extension procedure and
resulting in an error upon connection.
This change fixes the logic to use the output of the previous iteration per the spec.
closes#1660
SftpClient.CreateText does not truncate the file if it exists, contrary to
System.IO.File.CreateText which does. It is documented, but seems pretty
unintuitive and more like a mistake than a concious decision.
I considered leaving this 14 year old behaviour as it is, but turns out other
people have hit it as well (it also affects WriteAll{Bytes/Text/Lines}).
* Bump the dependencies group with 4 updates
Bumps BouncyCastle.Cryptography from 2.6.1 to 2.6.2
Bumps Meziantou.Analyzer from 2.0.205 to 2.0.210
Bumps MSTest from 3.9.3 to 3.10.0
Bumps SonarAnalyzer.CSharp from 10.13.0.120203 to 10.15.0.120848
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
* Refactor logging to allow a loggerfactory per session specified in the ConnectionInfo.
This commit introduces an `ILoggerFactory` to various classes, replacing the static logger factory with an instance-based approach for more flexible and session-specific logging. These changes improve the logging framework's flexibility and maintainability and allow unit testing of logging.
* Improvements bases on feedback. Fixed tests. Added documentation.
* Update src/Renci.SshNet/ConnectionInfo.cs
---------
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
* Fixes spelling errors in comments, private members, and public members in test projects
* Adds en-GB as spell checker option; reverts notable cases of American English
* convert file UTF-16 LE BOM -> UTF-8
---------
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
The library currently allocates 4 bytes (and some) for every 1 byte of file
downloaded(*). It could be 0. This takes it to 3.
(*)
1. Array allocated for read of encrypted packet from socket
2. Array for decrypted packet
3. Array for channel data (removed in this change)
4. Array for sftp data packet
The library currently does not check for matching host key algorithms until needed
at the end of the key exchange, in contrast to other algorithm types which are checked
beforehand. This leads to confusing or uninformative errors, normally from the server
(correctly) closing the connection.
This change moves that check alongside the rest of them, and also improves the error
messages that arise from no matching algorithms.
* Added GetAttributesAsync to SftpClient
* Adding integration tests + unit test
* Address warnings in test classes.
---------
Co-authored-by: William Decker <william.decker@syndigo.com>
Where beneficial, add additional overrides from the base Stream class. Namely the Span
variants and for PipeStream, the WriteAsync variants (see comments).
The change also adds an internal type borrowed from the runtime repo for easier buffer
management, which could also be used elsewhere.
* Bump the dependencies group with 5 updates
* use MEL 8.0.3
* use MSTest meta package
* revert Meziantou due to NRE
* add a more useful global.json and pin third party action
---------
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
* Add an OrderedDictionary implementation for algorithm priorities
During the key exchange, the algorithms to be used are chosen based on the order that
the client sends: first algorithm is most desirable. Currently, the algorithm
collections in ConnectionInfo are defined as IDictionary<,> and backed by
Dictionary<,>, which does not have any guarantees on the order of enumeration
(in practice, when only adding and not removing items it does enumerate in the order
that items were added as an implementation detail, but it's not great to rely on it).
This change adds IOrderedDictionary<,> and uses it in ConnectionInfo. On .NET 9,
this is backed by System.Collections.Generic.OrderedDictionary<,> and on lower
targets, it uses a relatively simple implementation backed by a List and a
Dictionary.
* use ThrowIfNegative
* CI: run .NET Framework Integration Tests on Windows
* use apt-get
* use vampire/setup-wsl
* Run Windows Integration Tests in separate job
so publish doesn't depend on it
* Ignore flakey Test from #1253 in CI
* Drop net6.0 target
* Update src/Renci.SshNet/Common/TaskToAsyncResult.cs
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
* remove redundant #if
for some reason this made the compiler suddenly
realize that the plain text variables are unused.
* use TargetFrameworkIdentifier
this doesn't work in Directory.Build.props, moved it to Directory.Build.targets.
* fix null reference warnings in Benchmarks
seems like the warnings were (somehow) disabled here
before and were fixed by the previous TargetFrameworkIdentifier
change.
* fix unused plainTextOffset in AesGcmCipher.BclImpl
* CI retry
* more cosmetics
* more
* update README
* Revert "use TargetFrameworkIdentifier"
This reverts commit 076ede161d.
---------
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
Co-authored-by: Robert Hague <rh@johnstreetcapital.com>
The new(-ish) implementation of SshCommand has a race condition for short-lived
commands where SSH_MSG_CHANNEL_CLOSE may be processed on the message loop thread
before SSH_MSG_CHANNEL_SUCCESS is waited upon on the Execute (main) thread. This
manifests in an ArgumentNull/NullReference exception on the wait handle because
the channel has already been closed and disposed.
Fix this by only delaying the channel dispose until the command dispose.
We currently don't recognise any global requests from the server, but if one is
sent, then per RFC 4253 section 4 we still need to reply when the server expects
one. So send SSH_MSG_REQUEST_FAILURE in this case.
DSA is removed at compile time from OpenSSH 9.8 and higher.
That means we can no longer test it in our integration tests. It seems like a
good time to remove it. From the OpenSSH release notes:
DSA, as specified in the SSHv2 protocol, is inherently weak - being
limited to a 160 bit private key and use of the SHA1 digest. Its
estimated security level is only 80 bits symmetric equivalent.
OpenSSH has disabled DSA keys by default since 2015 but has retained
run-time optional support for them. DSA was the only mandatory-to-
implement algorithm in the SSHv2 RFCs, mostly because alternative
algorithms were encumbered by patents when the SSHv2 protocol was
specified.
This has not been the case for decades at this point and better
algorithms are well supported by all actively-maintained SSH
implementations. We do not consider the costs of maintaining DSA
in OpenSSH to be justified and hope that removing it from OpenSSH
can accelerate its wider deprecation in supporting cryptography
libraries.
* Use System.Security.Cryptography in DesCipher and TripleDesCipher; Fall back to use BouncyCastle if BCL doesn't support
* Drop DesCipher; Replace PKCS7Padding with BouncyCastle's implementation.
* Restore `CbcCipherMode`
* Restore AesCipherMode; Use BlockImpl instead of BouncyCastleImpl for 3DES-CFB on lower targets.
* Restore the xml doc comment
* Tighten private key checking to reveal padding issue
* `Encrypt` should take into account padding for length of `inputBuffer` passed to `EncryptBlock` if padding is specified, no matter input is divisible or not.
* `Decrypt` should take into account unpadding for the final output if padding is specified.
* `Decrypt` should take into account *manual* padding for length of `inputBuffer` passed to `DecryptBlock` and unpadding for the final output if padding is not specified and mode is CFB or OFB.
* `Encrypt` should take into account *manual* padding for length of `inputBuffer` passed to `EncryptBlock` and unpadding for the final output if padding is not specified and mode is CFB or OFB.
* Rectify DES cipher tests. There's no padding in the data.
* Borrow `PadCount` method from BouncyCastle
* Manually pad input in CTR mode as well. Update AesCipherTest.
Co-Authored-By: Rob Hague <5132141+Rob-Hague@users.noreply.github.com>
* Manually pad/unpad for Aes CFB/OFB mode
* Update test/Renci.SshNet.Tests/Classes/Security/Cryptography/Ciphers/AesCipherTest.Gen.cs.txt
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
* Re-generate AES cipher tests
---------
Co-authored-by: Rob Hague <5132141+Rob-Hague@users.noreply.github.com>
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
* Drop net7.0 target
.NET 7 is EOL since May. The only .NET 7 features we use are
`ObjectDisposedException.ThrowIf` (moved to a throw helper) and
some newer regex features.
This feels a bit weird, but I suppose it is the expected course of action.
* fix build warning-as-error which is suddenly appearing on net6.0
IsAotCompatible not supported on net6.0
---------
Co-authored-by: Wojciech Nagórski <wojtpl2@gmail.com>
* Replace DiagnosticAbstrations with Microsoft.Extensions.Logging.Abstractions
* add documentation
* reduce allocations by SessionId hex conversion
generate the hex string once instead of every log
call and optimize ToHex().
* Update docfx/logging.md
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
* reduce log levels
* hook up testcontainers logging
* drop packet logs further down to trace
* add kex traces
---------
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
Rob Hague