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

[API Proposal]: PemEncoding.WriteString #65144

Closed
vcsjones opened this issue Feb 10, 2022 · 3 comments · Fixed by #69877
Closed

[API Proposal]: PemEncoding.WriteString #65144

vcsjones opened this issue Feb 10, 2022 · 3 comments · Fixed by #69877
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@vcsjones
Copy link
Member

vcsjones commented Feb 10, 2022

Background and motivation

For a reason I no longer recall, PemEncoding.Write returns char[]. I think nearly all of the time, folks that choose to use the allocating version (as apposed to TryWrite) want a string, not a char[]. That means

  1. You end up wrapping nearly every single PemEncoding.Write(...) to be new string(PemEncoding.Write(...)).
  2. Doing 1 means you pay the allocation twice for the PEM. Once for the char array, and once again for the string.
  3. If you want to do it as one allocation and you only have a ReadOnlySpan<byte> of data, getting a ReadOnlySpan<T> into the callback of string.Create currently requires unsafe code, so this starts becoming a little more than trivial. So developers don't have an easy way to do just the allocation once.

When implementing the PEM exports, this pretty much meant every time I would have preferred to use Write meant I actually had to use TryWrite in combination with GetEncodedSize and string.Create.

API Proposal

Since we can't change the return type...

namespace System.Security.Cryptography {
    public static class PemEncoding {
        public static string WriteString(ReadOnlySpan<char> label, ReadOnlySpan<byte> data);
    }
}

API Usage

Using ExportCertificatePem as an example:

public string ExportCertificatePem()
{
int pemSize = PemEncoding.GetEncodedSize(PemLabels.X509Certificate.Length, RawDataMemory.Length);
return string.Create(pemSize, this, static (destination, cert) => {
if (!cert.TryExportCertificatePem(destination, out int charsWritten) ||
charsWritten != destination.Length)
{
Debug.Fail("Pre-allocated buffer was not the correct size.");
throw new CryptographicException();
}
});
}

Would become:

public string ExportCertificatePem()
{
    return PemEncoding.WriteString(PemLabels.X509Certificate, RawDataMemory.Span);
}

Alternative Designs

No response

Risks

No response

@vcsjones vcsjones added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 10, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Feb 10, 2022
@ghost
Copy link

ghost commented Feb 10, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

For a reason I no longer recall, PemEncoding.Write returns char[]. I think nearly all of the time, folks that choose to use the allocating version (as apposed to TryWrite) want a string, not a char[]. That means

  1. You end up wrapping nearly every single PemEncoding.Write(...) to be new string(PemEncoding.Write(...)).
  2. Doing 1 means you pay the allocation twice for the PEM. Once for the char array, and once again for the string.
  3. If you want to do it as one allocation and you only have a ReadOnlySpan<char> of data, getting a ReadOnlySpan<T> into the callback of string.Create currently requires unsafe code, so this starts becoming a little more than trivial. So developers don't have an easy way to do just the allocation once.

When implementing the PEM exports, this pretty much meant every time I would have preferred to use Write meant I actually had to use TryWrite in combination with GetEncodedSize and string.Create.

API Proposal

Since we can't change the return type...

namespace System.Security.Cryptography {
    public static class PemEncoding {
        public string WriteString(ReadOnlySpan<char> label, ReadOnlySpan<byte> data);
    }
}

API Usage

Using ExportCertificatePem as an example:

public string ExportCertificatePem()
{
int pemSize = PemEncoding.GetEncodedSize(PemLabels.X509Certificate.Length, RawDataMemory.Length);
return string.Create(pemSize, this, static (destination, cert) => {
if (!cert.TryExportCertificatePem(destination, out int charsWritten) ||
charsWritten != destination.Length)
{
Debug.Fail("Pre-allocated buffer was not the correct size.");
throw new CryptographicException();
}
});
}

Would become:

public string ExportCertificatePem()
{
    return PemEncoding.WriteString(PemLabels.X509Certificate, RawDataMemory.Span);
}

Alternative Designs

No response

Risks

No response

Author: vcsjones
Assignees: -
Labels:

api-suggestion, area-System.Security, untriaged

Milestone: -

@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Feb 10, 2022
@bartonjs bartonjs added this to the Future milestone Feb 10, 2022
@bartonjs bartonjs modified the milestones: Future, 7.0.0 May 4, 2022
@vcsjones vcsjones self-assigned this May 25, 2022
@vcsjones
Copy link
Member Author

Branch with implementation main...vcsjones:writestring-impl

@bartonjs
Copy link
Member

Looks good as proposed

namespace System.Security.Cryptography {
    public static class PemEncoding {
        public static string WriteString(ReadOnlySpan<char> label, ReadOnlySpan<byte> data);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 26, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 26, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 28, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants