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

Implement PEM exports for RSA PKCS#1 and ECPrivateKey #61487

Closed
wants to merge 6 commits into from

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Nov 11, 2021

Closes #51630.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 11, 2021
@dotnet-issue-labeler
Copy link

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.

@vcsjones
Copy link
Member Author

vcsjones commented Nov 11, 2021

@bartonjs this is all in S.S.C.Algorithms. I'm not sure if you are in the middle of moving that down in to the new S.S.Cryptography assembly. If you have that in-flight, it's probably easier for me to rebase on that work than for this to get merged and you rebase on top of that. If that is the case, please feel free to hold this PR until that's complete and I'll respond to the things moving around.

@ghost
Copy link

ghost commented Nov 11, 2021

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

Issue Details

Closes out #51630.

Author: vcsjones
Assignees: -
Labels:

area-System.Security, new-api-needs-documentation, community-contribution

Milestone: -

@bartonjs
Copy link
Member

I'm not sure if you are in the middle of moving that down in to the new S.S.Cryptography assembly.

Yep, have it all merged in locally, except Browser.

If that is the case, please feel free to hold this PR until that's complete

OK, I'll go ahead and mark this as no-merge. It should be actionable late-this or early-next week.

@bartonjs bartonjs added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 11, 2021
@bartonjs
Copy link
Member

The code LGTM, just needs to be rebased on a future commit :)

The diminished DP/DQ values are exported different on Windows 7. Use
a key that will behave the same on all platforms.
The way RSA exports keys private keys is not consistent between platforms
and brittle. Change the tests to use well-known data for PEM exports so
the tests are stable.
@vcsjones
Copy link
Member Author

Since #61516 changed some of the APIs here, I am going to close this PR and re-open it after that PR is complete.

@vcsjones vcsjones closed this Nov 16, 2021
@vcsjones vcsjones deleted the 51630-rsa-ecc-pem branch November 22, 2021 23:10
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

APIs for exporting certificates and keys to PEM
2 participants