-
-
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
Adding Pagant Support #705
Conversation
Would this support not make more sense on a Appa built with the library than on the library that provides protocol core support?
|
Honestly I'm not sure what an Appa is in this context. I looked at it this way, The library handles the entire key exchange back and forth. Since a pagent isn't getting a key but instead using side channel as a challenge response, it would require changes on the core to widen interfaces to allow that new mechanism. This would then lead to a shim everyone who might want pagent support would need to use (add an extra layer of indirection and a possible security vector) or each application would need to do it themselves. Rather than sending the people down the road of re-inventing the wheel, (especially since the documentation on how to do this, would basically be the same code I wrote, only in outside programs) I figured it was better to do it once, in the core. Maybe I'm missing something here, so I'm open to other suggestions. It would also be trivial to make this a compile time option instead, if there's a concern about performance/security. |
Sorry was typing a n my phone, I meant App. A better one would be OpenSSH ssh-agent since it would mean compatibility with Windows, Mac and Linux. Specially since OpenSSH is now part of Windows. Only providing my opinion as a user. Im not a project Dev :)
|
Ah I see where you're coming from. Implementing the same thing in every app probably is a bad idea. You get a bunch of copies of the example code, with stylistic or other arbitrary changes, a shim layer that gets created later that a number of people will rely on, and then a fork of the original code base. This creates a mess and makes it very hard to push out security updates. As for the platforms:
Pagent solved my immediate problem and I was able to find good examples to work from. That said I would expect in the future more ssh agent protocols to be implemented based on need. They might even be split out to a plugin mechanism. |
@@ -0,0 +1,228 @@ | |||
#if NETFRAMEWORK && !NET20 && !NET35 |
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.
.NET standard for .NET core is supported by the SSH.NET library in general so new additions to the code base should continue to honor that.
} | ||
|
||
/// <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.
Missing comment content
public IAgentProtocol Protocol { get; private set; } | ||
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="PrivateKeyAuthenticationMethod"/> class. |
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.
wrong cref
/// <summary> | ||
/// Initializes a new instance of the <see cref="PrivateKeyAuthenticationMethod"/> class. | ||
/// </summary> | ||
/// <param name="username">The username.</param> |
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.
nit: fragment sentences (sentences without subject-verb-complement) should not end with dot ('.')
/// <returns></returns> | ||
public override AuthenticationResult Authenticate (Session session) { | ||
if (Protocol == null) | ||
return AuthenticationResult.Failure; |
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 a more specific error / a failure reason be provided?
yield break; | ||
} | ||
|
||
string mmFileName = Path.GetRandomFileName (); |
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.
use var consistently unless type is ambiguous
} | ||
|
||
int position = 9; | ||
for (int i = 0; i < numberOfIdentities; i++) { |
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.
avoid single letter parameters as they are ambiguous. Name indexes after what index of they are. E.g. identityIndex
|
||
accessor.ReadArray (position, blob, 0, blobSize); | ||
position += blobSize; | ||
int commnetLenght = IPAddress.HostToNetworkOrder (accessor.ReadInt32 (position)); |
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.
typo commnet
position += commnetLenght; | ||
|
||
string comment = Encoding.ASCII.GetString (commentChars); | ||
string type = Encoding.ASCII.GetString (blob, 4, blob[3]); // needs more testing kind of hack |
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.
Resolve the hack prior to merge to main branch
mmFile.SetAccessControl (security); | ||
using (var accessor = mmFile.CreateViewAccessor ()) { | ||
|
||
accessor.Write (0, IPAddress.NetworkToHostOrder (AGENT_MAX_MSGLEN - 4)); |
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.
Here would a be good place to have code comments to explain what we are doing, point to documentation and/or specification that apply to this implementation.
I'm closing PR. This is an old issue with no votes. |
Adds pagent support.
That said, the dependencies requires .Net 4.0. Pagent support in non-windows desktop environment does not make sense, so this requirement seems reasonable.