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

Support ECDiffieHellman on X509Certificate2 #42180

Merged
merged 31 commits into from Sep 28, 2020
Merged

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Sep 13, 2020

This adds ECDiffieHellman functionality to X509Certificate2. In addition, this also adds arbitrary SubjectPublicKeyInfo support for PublicKey.

This also makes some under-the-hood changes. Previously, we were always able to know what type of algorithm was used with a given certificate based on the certificate SPKI's OID. In the case of id-ecPublicKey, this is not true anymore. Other plumbing changes include changes to support ImportFromPem on X509Certificate2.

Work left to be done:

  • XML documentation
  • Windows PFX tests

Closes #413

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

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.

@ghost
Copy link

ghost commented Sep 13, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @jeffhandley
See info in area-owners.md if you want to be subscribed.

@vcsjones vcsjones marked this pull request as draft September 13, 2020 15:44
@vcsjones
Copy link
Member Author

Hm. CNG is behaving differently between Win10 and Win8.1/7. I suppose this is related to CNG's strong opinion that EC keys are either ECDH or ECDSA.

Windows 8.1 and below needs to be explicitly told that the public key it
is handling is for key agreement purposes.
@vcsjones
Copy link
Member Author

@bartonjs I think I fixed the Windows 7 / 8.1 key import problem. Does the change in 2d4fda5 make sense?

@bartonjs
Copy link
Member

Does the change in 2d4fda5 make sense?

Depends on your definition of "make sense" 😄. Apparently the private keys default to ECDH but the public keys were defaulting to ECDSA, and whatever lookup it did in the CryptFindOID universe returned multiple hits, ECDSA first... and that flag says "if something can't encrypt, I don't want it".. and while ECDH can't encrypt, it's purpose is eventually encryption, so it's marked as "encryption". Does any of that make sense? Not especially 😄.

Does the code read cleanly? Sure. Does it make tests pass? Apparently 😄.

PublicKey should at least be able to create a SPKI for any algorithm.
Verify we can sign arbitrary SPKIs.
Add a test that we can still round-trip keys that contain invalid
parameters but are well structured.
@vcsjones
Copy link
Member Author

@bartonjs thanks as always for the thoughtful feedback. I think I have addressed all of the points you have made so far.

@vcsjones
Copy link
Member Author

Guessing macOS is going to blow up with a bad DSA at usage time, not import time. No idea what what it is actually doing with an 8-bit DSA key.

@bartonjs
Copy link
Member

Guessing macOS is going to blow up with a bad DSA at usage time, not import time. No idea what what it is actually doing with an 8-bit DSA key.

Well, p is 0, so that's a 0-bit key, no? 😄.

I'd probably delete one of the integers out of the DSA-Params struct so it doesn't decode correctly, making the whole "bad DSA" SPKI

30 1B 30 13 06 07 2A 86 48 CE 38 04 01 30 08 02 
01 00 02 03 00 FF FF 03 04 00 02 01 03 

Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again: Thanks, @vcsjones!

@bartonjs bartonjs merged commit cd6ff01 into dotnet:master Sep 28, 2020
@vcsjones vcsjones deleted the 413-impl branch September 28, 2020 15:49
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API Enhancement: ECDiffieHellman X509 certificate management gaps
3 participants