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

[Breaking] Drop support for RSA PKCS#1 PEM keys in favor of the OpenSSH format. #272

Merged
merged 2 commits into from
Dec 12, 2024

Conversation

tmds
Copy link
Owner

@tmds tmds commented Dec 12, 2024

As described in the README, this library embraces OpenSSH format when possible.

This drops support for the RSA PKCS#1 PEM ('RSA PRIVATE KEY') format which was supported because the library author was using this format for a key.

If you are using a key in this format, it can be converted by running:

ssh-keygen -p -f <key_file>

The OpenSSH format has been supported in OpenSSH since 2014 and became the default format used by 'ssh-keygen' in 2018.

@tmds
Copy link
Owner Author

tmds commented Dec 12, 2024

@jborean93 fyi.

…SH format.

As described in the README, this library embraces OpenSSH format when possible.

This drops support for the RSA PKCS#1 PEM ('RSA PRIVATE KEY') format which
was supported because the library author was using this format for a key.

If you are using a key in this format, it can be converted by running:

   ssh-keygen -p -f <key_file>

The OpenSSH format has been supported in OpenSSH since 2014 and became
the default format used by 'ssh-keygen' in 2018.
@tmds tmds force-pushed the drop_rsa_pem_key_support branch from 31b5c8f to 11a646f Compare December 12, 2024 10:07
@tmds tmds merged commit 76e391e into main Dec 12, 2024
2 checks passed
@yokrysty
Copy link
Contributor

well this change broke everything for me
i used the command to convert the key and now i get this error:
The specified RSA parameters are not valid. Exponent and Modulus are required. If D is present, it must have the same length as Modulus. If D is present, P, Q, DP, DQ, and InverseQ are required and must have half the length of Modulus, rounded up, otherwise they must be omitted.
i also tried to use puttygen -> Conversions -> Export OpenSSH Key (force new file format) with the same result
tried more tools on windows and linux without luck
i did some debugging and ended up finding that the InverseQ is not the same after the conversion
image
now since the old format is not supported anymore i cannot use the library

so you have an ideea what is going on?

@tmds
Copy link
Owner Author

tmds commented Dec 12, 2024

Sorry for breaking your app @yokrysty.

I assume the converted key is working well with other tools, like ssh.

From the debug info you provided, I think we're not meeting the length requirements that the .NET class is expecting:

If D is present, it must have the same length as Modulus. If D is present, P, Q, DP, DQ, and InverseQ are required and must have half the length of Modulus, rounded up, otherwise they must be omitted.

This code needs to be updated for that:

byte[] modulus = reader.ReadMPIntAsByteArray(isUnsigned: true);
byte[] exponent = reader.ReadMPIntAsByteArray(isUnsigned: true);
BigInteger d = reader.ReadMPInt();
byte[] inverseQ = reader.ReadMPIntAsByteArray(isUnsigned: true);
BigInteger p = reader.ReadMPInt();
BigInteger q = reader.ReadMPInt();
BigInteger dp = d % (p - BigInteger.One);
BigInteger dq = d % (q - BigInteger.One);
RSAParameters parameters = new()
{
Modulus = modulus,
Exponent = exponent,
D = d.ToByteArray(isUnsigned: true, isBigEndian: true),
InverseQ = inverseQ,
P = p.ToByteArray(isUnsigned: true, isBigEndian: true),
Q = q.ToByteArray(isUnsigned: true, isBigEndian: true),
DP = dp.ToByteArray(isUnsigned: true, isBigEndian: true),
DQ = dq.ToByteArray(isUnsigned: true, isBigEndian: true)
};

With your key only the InverseQ is not the expected value. Can you try updating the above code by setting the minLength on ReadMPIntAsByteArray when we read inverseQ so we meet the length requirement, and see if that makes things work?

@yokrysty
Copy link
Contributor

yes it works with:
byte[] inverseQ = reader.ReadMPIntAsByteArray(isUnsigned: true, minLength: modulus.Length / 2);

@tmds
Copy link
Owner Author

tmds commented Dec 12, 2024

Great! I'll look at fixing this and include it in a patch release in the coming day(s).

@yokrysty
Copy link
Contributor

yokrysty commented Dec 12, 2024

forgot to mention that the app is running on Windows .NET 8 inside IIS
i see that on Windows RSA.Create() is using new RSABCrypt() which seems a wrapper around some Windows native dll
image

@tmds
Copy link
Owner Author

tmds commented Dec 13, 2024

@yokrysty the fix is in 0.9.1 which I've just uploaded to nuget.org.

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

Successfully merging this pull request may close these issues.

2 participants