-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add the ability to load a PKCS10 into CertificateRequest #73023
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones Issue DetailsFixes #29547.
|
src/libraries/System.Security.Cryptography/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
...ty.Cryptography/src/System/Security/Cryptography/X509Certificates/CertificateRequest.Load.cs
Outdated
Show resolved
Hide resolved
...ty.Cryptography/src/System/Security/Cryptography/X509Certificates/CertificateRequest.Load.cs
Outdated
Show resolved
Hide resolved
...ty.Cryptography/src/System/Security/Cryptography/X509Certificates/CertificateRequest.Load.cs
Outdated
Show resolved
Hide resolved
...ty.Cryptography/src/System/Security/Cryptography/X509Certificates/CertificateRequest.Load.cs
Outdated
Show resolved
Hide resolved
...ecurity.Cryptography/src/System/Security/Cryptography/X509Certificates/CertificateRequest.cs
Show resolved
Hide resolved
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.
A non-blocking suggestion, otherwise looks fine to me.
int saltSize = digestValueLength.GetValueOrDefault(); | ||
|
||
if (!digestValueLength.HasValue) | ||
{ | ||
saltSize = Helpers.HashOidToByteLength(HashAlgorithm.Algorithm); | ||
} |
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.
int saltSize = digestValueLength.GetValueOrDefault(); | |
if (!digestValueLength.HasValue) | |
{ | |
saltSize = Helpers.HashOidToByteLength(HashAlgorithm.Algorithm); | |
} | |
int saltSize = digestValueLength ?? Helpers.HashOidToByteLength(HashAlgorithm.Algorithm); |
@bartonjs actually, one other question: some of these are "big exponent" tests. Are they going to fail on Android like you observed at #72906 (comment)? |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
I was going to say "no", because the PSS tests all just use a new ephemeral RSA key... but it does look like there might be at least one, so let's see. |
Hmmm.... I can't actually find any big-exponent Verify tests that run. I thought we had big-exponent tests in the base algorithms library, but we don't... In the CRLBuilder tests, the Pkcs1 path didn't bother asking if the signature checked out, because the signature algorithm is deterministic, and if the signature generator can generate the right signature then clearly it can verify it... right? RSAAndroid uses RsaPaddingProcessor instead of OS routines for sign and verify, but we know those implementations are "correct" because macOS also uses them (at least, for PSS, which we've already seen as not working). This makes it seem like Android's RsaSignPrimitive is working correctly for big exponents, but RsaVerificationPrimitive isn't... which seems... odd. I was concerned that we broke it with moving off of RsaPaddingProcessor recently, but that was only for OAEP encryption, the current source for RSAAndroid shows it's still using RsaPaddingProcessor for RSA sign and RSA verify (both PSS and PKCS1). As far as this PR, it's one of:
Hmm. |
My 2 cents: We should disable the tests for Android and create an issue to investigate post 7 since there seems to be some ambiguity around "RsaSignPrimitive is working correctly for big exponents, but RsaVerificationPrimitive isn't" That leaves one of two things. Either we figure out that something isn't quite right with verification, we fix it, and then remove the ignores. Or, if everything is working on our side, we can write tests against the primitives to codify that Android throws the correct Or some variation on this idea. |
Anecdotally, I was investigating getting Android off RSAPaddingProcessor for PKCS1 / PSS like we did with OAEP... but... it seems that Android has no capability to sign pre-hashed data. It always wants the message and does the hash + sign in one step, unless you fall back to raw RSA and do it yourself. |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes #29547.