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

PR #971 with conflicts resolved and building #1156

Closed
wants to merge 1 commit into from

Conversation

et1975
Copy link

@et1975 et1975 commented Aug 3, 2023

This brings #971 up to date and hopefully nudging along the solution for #825.

@et1975
Copy link
Author

et1975 commented Aug 9, 2023

@Rob-Hague or anyone else can comment/approve?

@Rob-Hague
Copy link
Collaborator

@Rob-Hague or anyone else can comment/approve?

I don't speak with any authority but I have given this problem some thought so I will leave my opinion here.

It's great that there is a (presumably) working solution already. One can pull down the code and build the library to use it.

However I do not think the implementation is good enough to be merged into the repo, mainly with regard to the duplication of the RsaKey and RsaDigitalSignature classes. I know why it was done this way: a Key is contracted to provide a DigitalSignature but all DigitalSignature implementations require a Key. So you end up with the implementations coupled together. Ideally RsaDigitalSignature would take a HashAlgorithm (or similar) in the constructor but then this needs to be passed through RsaKey, which needn't be concerned with the implementation of a signature.

I think the solution is to remove the Sign and Verify methods from Key: ultimately a key itself does not sign something. But that could be a rather big (breaking) change, including on other types like KeyHostAlgorithm. If I think a bit harder I will consider opening an issue with a proposal.

@WojciechNagorski
Copy link
Collaborator

@et1975 I'm waiting for more permissions for this project and intend to reactivate this project once I get them.

@WojciechNagorski
Copy link
Collaborator

WojciechNagorski commented Sep 6, 2023

@Rob-Hague Can you propose your solution/PR? I already have repo and Nuget permissions so we can handle this topic.

@et1975
Copy link
Author

et1975 commented Sep 19, 2023

Closing in favour of #1177

@et1975 et1975 closed this Sep 19, 2023
@WojciechNagorski
Copy link
Collaborator

@et1975 Did you test #1177 ?

@et1975
Copy link
Author

et1975 commented Sep 19, 2023

I'm not using this library directly, will have to see what it means for Posh-SSH.

@WojciechNagorski
Copy link
Collaborator

Ok, we need to test everything and confirm if it's ok and then we'll release it. Then PoshSSH can update.

@Rob-Hague
Copy link
Collaborator

FYI I have tested it against several different servers (including servers which are not OpenSSH)

@WojciechNagorski
Copy link
Collaborator

Version 2023.0.0 has been published https://www.nuget.org/packages/SSH.NET/2023.0.0

1 similar comment
@WojciechNagorski
Copy link
Collaborator

Version 2023.0.0 has been published https://www.nuget.org/packages/SSH.NET/2023.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants