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.

Sign up for GitHub

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

Jump to bottom

Use System.Security.Cryptography for RSA #1373

Merged
merged 7 commits into from
May 10, 2024
Merged

Use System.Security.Cryptography for RSA #1373

merged 7 commits into from
May 10, 2024

Conversation

Rob-Hague
Copy link
Collaborator

@Rob-Hague Rob-Hague commented Apr 8, 2024 •
edited
Loading

And remove the hand-rolled implementation

Before:

Method Mean Error StdDev Gen0 Allocated
Sign 7,399.7 μs 20.87 μs 18.50 μs 2890.6250 5915.73 KB
Verify 132.8 μs 0.16 μs 0.14 μs 22.2168 45.74 KB

After:

Method Mean Error StdDev Gen0 Allocated
Sign 480.00 μs 0.794 μs 0.663 μs - 369 B
Verify 15.05 μs 0.017 μs 0.016 μs 0.1678 368 B

closes #1388

@Rob-Hague
Copy link
Collaborator Author

After this, I will do something similar for DSA, and then switch BigInteger over to the BCL

@scott-xu
Copy link
Collaborator

scott-xu commented Apr 9, 2024

One other thing: we can refer System.Memory nuget package when target to net462 and netstandard2.0 so that the code can use Span<T> ReadOnlySpan<T> BinaryPrimitives etc.

@Rob-Hague
Copy link
Collaborator Author

Yes, it is on my slow-moving todo list, perhaps starting with reviving #1160. Feel free to get it started, I probably won't work on it in the immediate future

@FreekSchoenmakersBuckaroo FreekSchoenmakersBuckaroo mentioned this pull request Apr 30, 2024
Closed
@WojciechNagorski WojciechNagorski merged commit f9908a2 into sshnet:develop May 10, 2024
1 check passed
@Rob-Hague Rob-Hague deleted the rsa branch May 10, 2024 12:24
@Rob-Hague Rob-Hague mentioned this pull request Jul 11, 2024
Open
Rob-Hague added a commit to Rob-Hague/SSH.NET that referenced this pull request Jul 27, 2024
@Rob-Hague
Use System.Security.Cryptography for DSA
d520f9d 
This is the analogue of the RSA change sshnet#1373 for DSA. This has a couple of caveats:

- The BCL supports only FIPS 186-compliant keys, that is, public (P, Q) lengths
  of (512 <= P <= 1024, 160) for FIPS 186-1/186-2; and (2048, 256), (3072, 256)
  for FIPS 186-3/186-4. The latter also specifies (2048, 224) but due to a quirk
  in the Windows API, the BCL does not support Q values of length 224[^1].
- OpenSSH, based on the SSH spec, only supports (supported) Q values of length 160,
  but appears to also work in non-FIPS-compliant cases such as in our integration
  tests with a (2048, 160) host key. That test now fails and I changed that host key
  to (1024, 160).

This basically means that (1024, 160) is the largest DSA key size supported by both
SSH.NET and OpenSSH. However, given that OpenSSH deprecated DSA in 2015[^2], and the
alternative that I have been considering is just to delete support for DSA in the
library, this change seems reasonable to me. I don't think we can justify keeping the
current handwritten code around.

I think we may still consider dropping DSA from the library, I just had this branch
laying around and figured I'd finish it off.

[^1]: https://github.com/dotnet/runtime/blob/fadd8313653f71abd0068c8bf914be88edb2c8d3/src/libraries/Common/src/System/Security/Cryptography/DSACng.ImportExport.cs#L259-L265
[^2]: https://www.openssh.com/txt/release-7.0
@Rob-Hague Rob-Hague mentioned this pull request Jul 27, 2024
Merged
Rob-Hague added a commit to Rob-Hague/SSH.NET that referenced this pull request Aug 1, 2024
@Rob-Hague
Use System.Security.Cryptography for DSA
a73df07 
This is the analogue of the RSA change sshnet#1373 for DSA. This has a couple of caveats:

- The BCL supports only FIPS 186-compliant keys, that is, public (P, Q) lengths
  of (512 <= P <= 1024, 160) for FIPS 186-1/186-2; and (2048, 256), (3072, 256)
  for FIPS 186-3/186-4. The latter also specifies (2048, 224) but due to a quirk
  in the Windows API, the BCL does not support Q values of length 224[^1].
- OpenSSH, based on the SSH spec, only supports (supported) Q values of length 160,
  but appears to also work in non-FIPS-compliant cases such as in our integration
  tests with a (2048, 160) host key. That test now fails and I changed that host key
  to (1024, 160).

This basically means that (1024, 160) is the largest DSA key size supported by both
SSH.NET and OpenSSH. However, given that OpenSSH deprecated DSA in 2015[^2], and the
alternative that I have been considering is just to delete support for DSA in the
library, this change seems reasonable to me. I don't think we can justify keeping the
current handwritten code around.

I think we may still consider dropping DSA from the library, I just had this branch
laying around and figured I'd finish it off.

[^1]: https://github.com/dotnet/runtime/blob/fadd8313653f71abd0068c8bf914be88edb2c8d3/src/libraries/Common/src/System/Security/Cryptography/DSACng.ImportExport.cs#L259-L265
[^2]: https://www.openssh.com/txt/release-7.0
Rob-Hague added a commit to Rob-Hague/SSH.NET that referenced this pull request Aug 10, 2024
@Rob-Hague
Use System.Security.Cryptography for DSA
2ea0e65 
This is the analogue of the RSA change sshnet#1373 for DSA. This has a couple of caveats:

- The BCL supports only FIPS 186-compliant keys, that is, public (P, Q) lengths
  of (512 <= P <= 1024, 160) for FIPS 186-1/186-2; and (2048, 256), (3072, 256)
  for FIPS 186-3/186-4. The latter also specifies (2048, 224) but due to a quirk
  in the Windows API, the BCL does not support Q values of length 224[^1].
- OpenSSH, based on the SSH spec, only supports (supported) Q values of length 160,
  but appears to also work in non-FIPS-compliant cases such as in our integration
  tests with a (2048, 160) host key. That test now fails and I changed that host key
  to (1024, 160).

This basically means that (1024, 160) is the largest DSA key size supported by both
SSH.NET and OpenSSH. However, given that OpenSSH deprecated DSA in 2015[^2], and the
alternative that I have been considering is just to delete support for DSA in the
library, this change seems reasonable to me. I don't think we can justify keeping the
current handwritten code around.

I think we may still consider dropping DSA from the library, I just had this branch
laying around and figured I'd finish it off.

[^1]: https://github.com/dotnet/runtime/blob/fadd8313653f71abd0068c8bf914be88edb2c8d3/src/libraries/Common/src/System/Security/Cryptography/DSACng.ImportExport.cs#L259-L265
[^2]: https://www.openssh.com/txt/release-7.0
Rob-Hague added a commit to Rob-Hague/SSH.NET that referenced this pull request Aug 10, 2024
@Rob-Hague
Use System.Security.Cryptography for DSA
7200076 
This is the analogue of the RSA change sshnet#1373 for DSA. This has a couple of caveats:

- The BCL supports only FIPS 186-compliant keys, that is, public (P, Q) lengths
  of (512 <= P <= 1024, 160) for FIPS 186-1/186-2; and (2048, 256), (3072, 256)
  for FIPS 186-3/186-4. The latter also specifies (2048, 224) but due to a quirk
  in the Windows API, the BCL does not support Q values of length 224[^1].
- OpenSSH, based on the SSH spec, only supports (supported) Q values of length 160,
  but appears to also work in non-FIPS-compliant cases such as in our integration
  tests with a (2048, 160) host key. That test now fails and I changed that host key
  to (1024, 160).

This basically means that (1024, 160) is the largest DSA key size supported by both
SSH.NET and OpenSSH. However, given that OpenSSH deprecated DSA in 2015[^2], and the
alternative that I have been considering is just to delete support for DSA in the
library, this change seems reasonable to me. I don't think we can justify keeping the
current handwritten code around.

I think we may still consider dropping DSA from the library, I just had this branch
laying around and figured I'd finish it off.

[^1]: https://github.com/dotnet/runtime/blob/fadd8313653f71abd0068c8bf914be88edb2c8d3/src/libraries/Common/src/System/Security/Cryptography/DSACng.ImportExport.cs#L259-L265
[^2]: https://www.openssh.com/txt/release-7.0
WojciechNagorski pushed a commit that referenced this pull request Aug 11, 2024
@Rob-Hague
Use System.Security.Cryptography for DSA (#1458)
fe65570 
* Use System.Security.Cryptography for DSA

This is the analogue of the RSA change #1373 for DSA. This has a couple of caveats:

- The BCL supports only FIPS 186-compliant keys, that is, public (P, Q) lengths
  of (512 <= P <= 1024, 160) for FIPS 186-1/186-2; and (2048, 256), (3072, 256)
  for FIPS 186-3/186-4. The latter also specifies (2048, 224) but due to a quirk
  in the Windows API, the BCL does not support Q values of length 224[^1].
- OpenSSH, based on the SSH spec, only supports (supported) Q values of length 160,
  but appears to also work in non-FIPS-compliant cases such as in our integration
  tests with a (2048, 160) host key. That test now fails and I changed that host key
  to (1024, 160).

This basically means that (1024, 160) is the largest DSA key size supported by both
SSH.NET and OpenSSH. However, given that OpenSSH deprecated DSA in 2015[^2], and the
alternative that I have been considering is just to delete support for DSA in the
library, this change seems reasonable to me. I don't think we can justify keeping the
current handwritten code around.

I think we may still consider dropping DSA from the library, I just had this branch
laying around and figured I'd finish it off.

[^1]: https://github.com/dotnet/runtime/blob/fadd8313653f71abd0068c8bf914be88edb2c8d3/src/libraries/Common/src/System/Security/Cryptography/DSACng.ImportExport.cs#L259-L265
[^2]: https://www.openssh.com/txt/release-7.0

* Appease mono

* test experiment

* Revert "Appease mono"

This reverts commit 881eefe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WojciechNagorski WojciechNagorski WojciechNagorski approved these changes

@scott-xu scott-xu scott-xu approved these changes

@drieseng drieseng Awaiting requested review from drieseng drieseng is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@Rob-Hague @scott-xu @WojciechNagorski