Skip to content

Document Pkcs12Info #2914

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

Merged
merged 8 commits into from
Aug 6, 2019
Merged

Document Pkcs12Info #2914

merged 8 commits into from
Aug 6, 2019

Conversation

bartonjs
Copy link
Member

@bartonjs bartonjs commented Aug 2, 2019

]]></format>
</remarks>
<exception cref="T:System.InvalidOperationException">The <see cref="P:System.Security.Cryptography.Pkcs12.Pkcs12Info.IntegrityMode"/> value is not <see cref="F:System.Security.Cryptography.Pkcs12.Pkcs12IntegrityMode.Password"/>.</exception>
<exception cref="T:System.Security.Cryptography.CryptographicException">The hash algorithm option specified by the PKCS#12 PFX contents could not be identified or is not supported by this platform.</exception>
</Docs>
</Member>
<Member MemberName="VerifyMac">
Copy link
Member Author

Choose a reason for hiding this comment

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

@mairaw or @rpetrusha : This file doesn't have a MethodGroup entry for this overloaded member (and I feel like I've seen other new API in that state). I don't know if that means a) we're not bothering with them, b) there's an error in the ingestor which results in it not producing them c) there's an error in the validator that makes it not report them as missing d) other e) some multiple combination of a-d.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean the MemberGroup element to add summary, remarks, etc. that apply to all overloads?
If so, the reflection tool doesn't add them automatically and we're not required to document them.

When we want to avoid repeating the same text, we manually add them.

If I misunderstood, please let me know.

Choose a reason for hiding this comment

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

It seems, though, that the <summary> element of a member group is ignored if there aren't some <AssemblyInfo> elements present.

@mairaw
Copy link
Contributor

mairaw commented Aug 2, 2019

Made some small changes. Please take a look.

@mairaw mairaw added 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles labels Aug 2, 2019
@mairaw mairaw added this to the August 2019 milestone Aug 2, 2019
@bartonjs
Copy link
Member Author

bartonjs commented Aug 2, 2019

Changes look good to me. As does the explanation about the MemberGroup element.

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

This looks really good, @bartonjs. I've left some minor suggestions and added a <MemberGroup> section that you can add a <remarks> section to (hopefully I didn't create some badly formatted XML). A number of links are broken in the build, though I can't see any reason why. I've suggested adding a space between the closing quotation mark and the end tag in crefs in the event that that fixes the problem, though I rather doubt that it will.

@bartonjs
Copy link
Member Author

bartonjs commented Aug 3, 2019

Ah, being removed from the text for a couple hours I see I wrote "Pkcs12" too many times... it's "S.S.C.Pkcs.Pkcs12Info", not "S.S.C.Pkcs12.Pkcs12Info". I'll remove the extraneous 12s

Copy link
Member Author

@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.

Removing all of the erroneous 12s and adding a MemberGroup remark via the suggestions interface.

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

I've left some additional comments and one suggestion for you to consider, @bartonjs.

Therefore, there is no guarantee that a password which results in a `true` return from this method will succeed on a call to <xref:System.Security.Cryptography.Pkcs.Pkcs12SafeContents.Decrypt%2A>.

In the PKCS#12 specification, a distinction is made between a `null` password and an "empty" password, and that difference is reflected in the return value of this method.
If the `null` string returns `true`, then <xref:System.String.Empty> returns `false`, and vice versa.

Choose a reason for hiding this comment

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

Suggested change
If the `null` string returns `true`, then <xref:System.String.Empty> returns `false`, and vice versa.
If the `null` string returns `true`, then <xref:System.String.Empty?displayProperty=nameWithType> returns `false`, and vice versa.


## Remarks

It's not possible to distingush the error due to the password being incorrect from the error due to the contents having been altered.

Choose a reason for hiding this comment

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

Since these two paragraphs now appear in the member group, they can be removed from here.


## Remarks

It's not possible to distinguish the error due to the password being incorrect from the error due to the contents having been altered.

Choose a reason for hiding this comment

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

Since these two paragraphs now appear in the member group, they can be removed from here.

@rpetrusha rpetrusha merged commit 72af7a5 into dotnet:master Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants