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 X509Certificate2.RawDataMemory #57835

Merged
merged 4 commits into from
Aug 24, 2021

Conversation

vcsjones
Copy link
Member

Closes #57448

@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 ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 20, 2021
@ghost
Copy link

ghost commented Aug 20, 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 #57448

Author: vcsjones
Assignees: -
Labels:

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

Milestone: -

@vcsjones
Copy link
Member Author

I didn't go with the custom MemoryManager trick. For one I would probably have a difficult time figuring out how to do it correctly, and for two because it still feels a bit unnecessary to me. MemoryMarshal is an "I know what I am doing" type, and even if we did do it, it would not address the pinning possibility.

@GrabYourPitchforks
Copy link
Member

Should we doc that the returned ROM<byte> has an independent lifetime from the underlying certificate? ROM<byte> is supposed to be an abstraction, but we're returning a property whose correct usage relies on the buffer having an independent lifetime, and people will absolutely depend on this behavior. Might be worthwhile to formalize this expectation in docs.

@vcsjones
Copy link
Member Author

@GrabYourPitchforks

Should we doc that the returned ROM<byte> has an independent lifetime from the underlying certificate?

What would documentation like that look like? I... think... given the API shape it's a reasonable expectation (otherwise it'd be a CopyRawDataTo(..) or something like that.

Would codifying in a test be more reasonable, or is that not strong enough? (I'm starting to think the test should exist regardless... let me add that).

@GrabYourPitchforks
Copy link
Member

Would codifying in a test be more reasonable, or is that not strong enough?

That's probably good enough honestly. Get the ROM<byte>, dispose the cert, then validate the buffer you previously got is still filled with delicious X.509 goodness.

{
ReadOnlyMemory<byte> first = cert.RawDataMemory;
ReadOnlyMemory<byte> second = cert.RawDataMemory;
Assert.True(first.Span == second.Span, "RawDataMemory returned different values.");
Copy link
Member

Choose a reason for hiding this comment

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

It's quite rare to see someone use == with spans and have it actually be correct ;-)

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

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

Thanks also for the unit test update!

@GrabYourPitchforks GrabYourPitchforks merged commit 893b437 into dotnet:main Aug 24, 2021
@vcsjones vcsjones deleted the cert-rawdatamemory branch August 24, 2021 12:16
@ghost ghost locked as resolved and limited conversation to collaborators Sep 23, 2021
@bartonjs bartonjs added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 19, 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.

[API Proposal]: X509Certificate2.RawDataMemory
4 participants