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

Add PEM exports to certificates #59674

Merged
merged 6 commits into from
Sep 29, 2021
Merged

Conversation

vcsjones
Copy link
Member

This implements PEM export on X509Certificate2 and X509Certificate2Collection.

Contributes to #51630.

This implements PEM export on X509Certificate2 and X509Certificate2Collection.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 27, 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.

@ghost
Copy link

ghost commented Sep 27, 2021

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

Issue Details

This implements PEM export on X509Certificate2 and X509Certificate2Collection.

Contributes to #51630.

Author: vcsjones
Assignees: -
Labels:

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

Milestone: -

public static void TryExportCertificatePems_Empty()
{
X509Certificate2Collection cc = new X509Certificate2Collection();
AssertPemExport(cc, string.Empty);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Mis-aligned

Suggested change
AssertPemExport(cc, string.Empty);
AssertPemExport(cc, string.Empty);

string pkcs7Pem = collection.ExportPkcs7Pem();
AssertPem(collection, pkcs7Pem);

Span<char> pkcs7Buffer = new char[pkcs7Pem.Length];
Copy link
Member

Choose a reason for hiding this comment

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

Should we give this one the full Goldilocks? (One too short (done), one too long (missing), one just right (done)).

@@ -1640,6 +1690,71 @@ private static void TestExportStore(X509ContentType ct)
}
}

private static void AssertPemExport(X509Certificate2Collection collection, string expectedContents)
{
Span<char> buffer = new char[expectedContents.Length];
Copy link
Member

Choose a reason for hiding this comment

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

Do we think "bigger than needed" is an interesting case here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh, interesting enough to make you leave a remark, so, let's say yes and add a test case for it.

"MAoGCCqGSM49BAMCA0cAMEQCIHafyKHQhv+03DaOJpuotD+jNu0Nc9pUI9OA8pUY\n" +
"3+qJAiBsqKjtc8LuGtUoqGvxLLQJwJ2QNY/qyEGtaImlqTYg5w==\n" +
"-----END CERTIFICATE-----";
using ImportedCollection ic = Cert.ImportFromPem(SinglePem);
Copy link
Member

Choose a reason for hiding this comment

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

While the repository, collectively, is OK with braceless usings in test code, I'll make one plea to keep the braces :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Your house, your rules 😄.

pemCount++;
}

Assert.Equal(1, pemCount);
Copy link
Member

Choose a reason for hiding this comment

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

This test is what inspired me to ask the tighter export elsewhere. Seems like we can simplify the code, and increase the amount of stuff that the test covers, if we just take an existing PEM import it, normalize the newlines to LF, and export+Assert.

[Theory]
[InlineData(true)]
[InlineData(false)]
public static void ExportCertificatePem_ContainsOnlyCertPem(bool yoda)
{
    const string Input = ...;

    string output;

    using (X509Certificate2 cert = new X509Certificate2(Encoding.ASCII.GetBytes(Input)))
    {
        if (yoda)
        {
            output = cert.ExportCertificatePem();
        }
        else
        {
            Span<char> destination = stackalloc char[Input.Length + 5];

            Assert.True(cert.TryExportCertificatePem(destination, out int charsWritten));
            Assert.Equal(Input.Length, charsWritten);

            output = destination.Slice(0, charsWritten).ToString();
        }
     }

     Assert.Equal(Input, output);
}

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could add the "did I start at the right place?" and "did I end at the right place?" checks to what's already here.

Copy link
Member Author

@vcsjones vcsjones Sep 29, 2021

Choose a reason for hiding this comment

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

Hm, I see. Perhaps "ContainsOnlyCertPem" was misleading in name, the test was ensuring that (if by some miracle) a certificate's private key would not be included, or at least a form of documentation "Yes, the 'pem export' does not include CERTIFICATE nor RSA/EC PRIVATE KEY".

But I see what you are suggesting as well. I suppose by "exact string" match, that would pretty much have the same meaning.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can just get rid of these tests if we tighten the others.

@bartonjs
Copy link
Member

Code generally looks good, just a couple of comment requests.

Tests generally look good, but since I didn't write the code I have the easy job of just asking for more :)

@bartonjs bartonjs merged commit 750965d into dotnet:main Sep 29, 2021
@vcsjones vcsjones deleted the x509-pem-export-51630 branch September 29, 2021 16:15
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
@bartonjs bartonjs added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 18, 2022
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 needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants