-
-
Notifications
You must be signed in to change notification settings - Fork 923
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
Allow signed keys #595
base: develop
Are you sure you want to change the base?
Allow signed keys #595
Conversation
|
||
private bool _isDisposed; | ||
|
||
/// <summary> |
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.
What's the rationale for implementation IDisposable?
/// Releases unmanaged resources and performs other cleanup operations before the | ||
/// <see cref="RsaCertificate"/> is reclaimed by garbage collection. | ||
/// </summary> | ||
~RsaCertificate() |
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.
Why does this class need a finalizer?
/// <summary> | ||
/// | ||
/// </summary> | ||
public class PublicKeyCertFile : IDisposable |
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.
If this class only supports RSA certificates, it should have a matching name.
Do you also plan to implement rsa-sha2-256-cert-v01@openssh.com and rsa-sha2-512-cert-v01@openssh.com?
/// <remarks>This method calls <see cref="System.IO.File.Open(string, System.IO.FileMode)"/> internally, this method does not catch exceptions from <see cref="System.IO.File.Open(string, System.IO.FileMode)"/>.</remarks> | ||
public PublicKeyCertFile(string fileName) | ||
{ | ||
if (string.IsNullOrEmpty(fileName)) |
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 argument check can be removed. File.Open(string path, FileMode mode, FileAccess access, FileShare share)
will already throw for an empty of null path.
/// <param name="fileName">Name of the file.</param> | ||
/// <exception cref="ArgumentNullException"><paramref name="fileName"/> is <c>null</c> or empty.</exception> | ||
/// <remarks>This method calls <see cref="System.IO.File.Open(string, System.IO.FileMode)"/> internally, this method does not catch exceptions from <see cref="System.IO.File.Open(string, System.IO.FileMode)"/>.</remarks> | ||
public PublicKeyCertFile(string fileName) |
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.
Please rename argument to path to match the argument name of File.Open(...)
.
/// Releases unmanaged resources and performs other cleanup operations before the | ||
/// <see cref="PublicKeyCertFile"/> is reclaimed by garbage collection. | ||
/// </summary> | ||
~PublicKeyCertFile() |
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.
Why does this class need a finalizer?
/// <param name="username">The username.</param> | ||
/// <param name="keyFile">The key files.</param> | ||
/// <exception cref="ArgumentException"><paramref name="username"/> is whitespace or <c>null</c>.</exception> | ||
public PrivateKeyCertAuthenticationMethod(string username, PrivateKeyFile keyFile) |
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.
Remove?
@@ -7,12 +10,23 @@ namespace Renci.SshNet.Security | |||
/// </summary> | |||
public class CertificateHostAlgorithm : HostAlgorithm |
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.
Does this class have to be public? We do not expose an instance publicly right now.
If not, RsaCertificate should also not be public.
public string Name | ||
{ | ||
get { return Utf8.GetString(_name, 0, _name.Length); } | ||
set { _name = Utf8.GetBytes(value); } |
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 any of these properties actually need a public setter?
/// <summary> | ||
/// | ||
/// </summary> | ||
public class RsaCertificate : IDisposable |
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.
If there's no use in CertificateHostAlgorithm being public, we should also make this class internal.
@robertkentish I did an initial sweep of your changes. Still need to dive a little deeper though. |
@robertkentish PR needs refreshing. |
PR as per issue #479. PRoposal to support public certs for RSA signatures as in the following manner.
var keyFile = new PrivateKeyFile(@"C:\temp\ssh_keys\id_rsa", "xxxx");
var certFile = new PublicKeyCertFile(@"C:\temp\ssh_keys\id_rsa-cert.pub");
var authMethod = new PrivateKeyCertAuthenticationMethod("user", keyFile, certFile);
var connectionInfo = new ConnectionInfo("192.168.1.1", "user", authMethod);
using (var client = new SshClient(connectionInfo))
{
client.Connect();
}