-
-
Notifications
You must be signed in to change notification settings - Fork 939
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
Abstract out the hash algorithm from RsaDigitalSignature #1170
Conversation
|
||
|
||
// The following fails due to the _isPrivate decision in RsaCipher.Transform. Is that really correct? | ||
//Assert.IsTrue(digitalSignature.Verify(data, signedBytes)); |
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 is surely a bug, but I would rather wait until the test migrations are done before looking at it.
The Encrypt
and Decrypt
methods on RsaCipher
both call the private Transform
method which changes its behaviour based (only) upon whether the RsaKey
member has private key information:
if (_isPrivate) |
So Encrypt
and Decrypt
both perform encryption (m ^ e mod N) when given a key with only public information, and both perform decryption when given a key with private key information. They only differ by some processing of padding.
It works because the base class CipherDigitalSignature
calls Encrypt
when signing, when really the signing operation is 'decryption' and the verification operation is to encrypt (but Verify
calls Decrypt
).
return _cipher.Encrypt(derEncodedHash).TrimLeadingZeros(); |
937ac0a
to
fc64a52
Compare
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.
Overall it looks good, but I don't know why digitalSignature.Verify(data, signedBytes)
doesn't work. Any idea?
|
||
|
||
// The following fails due to the _isPrivate decision in RsaCipher.Transform. Is that really correct? | ||
//Assert.IsTrue(digitalSignature.Verify(data, signedBytes)); |
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.
Any idea why this isn't working?
}, signedBytes); | ||
|
||
// The following fails due to the _isPrivate decision in RsaCipher.Transform. Is that really correct? | ||
//Assert.IsTrue(digitalSignature.Verify(data, signedBytes)); |
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.
Any idea why this isn't working?
CollectionAssert.AreEqual(expectedSignedBytes, rsaKey.Sign(data)); | ||
|
||
// The following fails due to the _isPrivate decision in RsaCipher.Transform. Is that really correct? | ||
//Assert.IsTrue(digitalSignature.Verify(data, signedBytes)); |
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.
Any idea why this isn't working?
Yes, see my comment above. The encrypt/decrypt methods change based on whether the key has private key information. I think it should be changed, but it seems like long-standing behaviour, and I would rather not change it here. |
I'm afraid to merge without further changes. I agree to these changes, but I don't know if they will fit nicely into further changes. Maybe you want to complete the entire ssh-sha256 and ssh-sha-512 implementation on this PR? |
Sure, the rest of the changes are based on this so the PR will include this either way. |
I know. Great! |
fc64a52
to
a0bb979
Compare
Included in #1177 |
Version 2023.0.0 has been published https://www.nuget.org/packages/SSH.NET/2023.0.0 |
1 similar comment
Version 2023.0.0 has been published https://www.nuget.org/packages/SSH.NET/2023.0.0 |
This is a step towards implementing the rsa-sha2 key host algorithms