Obsolete X509Certificate2.PrivateKey and PublicKey.Key. #54562

vcsjones · 2021-06-22T17:03:17Z

dotnet-issue-labeler · 2021-06-22T17:03:21Z

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 · 2021-06-22T17:03:23Z

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

Issue Details

Closes #47977.

Author:	vcsjones
Assignees:	-
Labels:	`area-System.Security`, `new-api-needs-documentation`
Milestone:	-

ghost · 2021-06-22T17:06:34Z

Tagging @dotnet/compat for awareness of the breaking change.

vcsjones · 2021-06-22T20:43:06Z

Test failures appear unrelated.

Once this is a 👍 from a code review perspective, I think it would be good to run outer loop on this to make sure the S.S.C.Pkcs tests I changed are still passing.

bartonjs · 2021-06-22T20:54:47Z

src/libraries/Common/src/System/Obsoletions.cs

@@ -89,5 +89,8 @@ internal static class Obsoletions

        internal const string X509CertificateImmutableMessage = "X509Certificate and X509Certificate2 are immutable. Use the appropriate constructor to create a new certificate.";
        internal const string X509CertificateImmutableDiagId = "SYSLIB0026";
+
+        internal const string KeyPropertiesMessage = "Key and PrivateKey are obsolete. Use the appropriate method to get the public key, or create a new instance with a different private key.";


The message feels a little... off.

Suggested change

internal const string KeyPropertiesMessage = "Key and PrivateKey are obsolete. Use the appropriate method to get the public key, or create a new instance with a different private key.";

internal const string KeyPropertiesMessage = "PublicKey.Key and X509Certificate2.PrivateKey are obsolete. Use the appropriate method to get the key (such as GetRSAPublicKey or GetRSAPrivateKey), or use the CopyWithPrivateKey method to create a new instance with a private key.";

use the CopyWithPrivateKey method to create a new instance with a private key.

Well, PublicKey has no such method which is why mine was probably too-vague. I'm starting to wonder if they would be better off as two separate diagnostics.

Or maybe I am thinking too hard about this.

Yeah, I was looking for precedent of using the same ID with different messages, and don't see that.

Numbers are cheap, splitting them so the message is more actionable makes sense to me. @terrajobst ?

I split the diagnostics and think that looks better. I'm happy to revert that last commit and combine them if we come up with how to message both with a single diagnostic message.

The way to think about this is like this:

Having two messages for the same diagnostic is fine, but I'd be careful that you don't end up with a grab bag for the ID where someone would want to frequently suppress one but not the other. Now, of course there is a trade off. The entire point of having a diagnostic ID is to group multiple different APIs under the same ID. You can always construct a case where someone would want to suppress it in the context of one API but not the other, but for me the imperative word is frequently. People can always suppress specific occurrences by using localized #pragma suppressions, so rare cases where this is needed always have an escape hatch. Having different IDs makes sense when for the entire project people would often want to suppress one ID but not the other. If these cases are exceedingly rare, I'd opt for using the same diagnostic ID.

While diagnostic IDs are cheap, try to imagine a VS UI that lists them all and have a user decide which one to turn off/turn. The more granular, the more cognitive load.

In other words, neither approach in the extreme is "correct", so it's something that needs domain expertise to dial.

And finally, we don't have to get it a 100% correct. Yes, we should avoid changing diagnostic IDs but if we ended up bucketizing two different cases under the same ID, I don't see a reason why, based on customer feedback, we couldn't split it later to make people's life better.

Does this make sense?

@terrajobst that is very insightful, thank you. I'm not sure that feedback pushes me over the edge in to re-combining them. Those APIs feel disparate that two separate codes make some sense.

@bartonjs et al. I am happy to change though if anyone thinks otherwise.

bartonjs · 2021-06-22T22:41:36Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-06-22T22:41:50Z

Azure Pipelines successfully started running 1 pipeline(s).

vcsjones · 2021-06-23T02:34:50Z

None of the build failures look related, but I am having a hard time parsing the results of the Android build error.

vcsjones · 2021-06-26T22:58:10Z

Merged in main and Android is building, and the previous run's outer loop didn't seem to indicate that any of the changed tests are breaking.

bartonjs · 2021-06-28T16:58:16Z

Failures look unrelated. Merging. Thanks, @vcsjones.

…bugger2 * origin/main: (78 commits) Fix unreached during dump. (dotnet#54861) Fix lowering usage of an unset LSRA field. (dotnet#54731) Fix setting breakpoints on AVX 256 instructions and other 32 byte immediate instructions (dotnet#54786) Add perf_slow yaml (dotnet#54853) Faster type load for scenarios made more common by generic math (dotnet#54588) Make sure we consider buffer length when marshalling back Unicode ByValTStr fields (dotnet#54695) Add YieldProcessor implementation for arm (dotnet#54829) Remove ActiveIssue for dotnet#50968 (dotnet#54831) Enable System.Text.Json tests for Wasm AOT (dotnet#54833) Remove ActiveIssue for dotnet#51723 (dotnet#54830) Fix load exception on generic covariant return type (dotnet#54790) Obsolete X509Certificate2.PrivateKey and PublicKey.Key. (dotnet#54562) First round of converting System.Drawing.Common to COMWrappers (dotnet#54636) Fix alloc-dealloc mismatches (dotnet#54701) Add one-shot ECB methods [Mono] MSBuild Task housekeeping (dotnet#54485) Move iOS/tvOS simulator AOT imports in the Mono workload (dotnet#54821) Remove unnecessary char[] allocation from Uri.GetRelativeSerializationString (dotnet#54799) Reduce overhead of Enumerable.Chunk (dotnet#54782) Fix EnumMemberRefs always returning NULL (dotnet#54805) ...

Obsolete X509Certificate2.PrivateKey and PublicKey.Key.

953e699

dotnet-issue-labeler bot added area-System.Security new-api-needs-documentation labels Jun 22, 2021

vcsjones added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jun 22, 2021

ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jun 22, 2021

Fix test compilation

7a5a8ea

vcsjones marked this pull request as ready for review June 22, 2021 20:41

bartonjs reviewed Jun 22, 2021

View reviewed changes

Split the diagnostics to make the message easier to action.

5df8b95

bartonjs approved these changes Jun 22, 2021

View reviewed changes

stephentoub approved these changes Jun 23, 2021

View reviewed changes

Merge remote-tracking branch 'ms/main' into 47977-obsolete-key-props

1e3348a

bartonjs merged commit af18e93 into dotnet:main Jun 28, 2021

vcsjones deleted the 47977-obsolete-key-props branch June 28, 2021 17:03

karelz mentioned this pull request Jul 19, 2021

Test failure: EventSource_UnsuccessfulRequest_LogsStartFailedStop #55906

Closed

ghost locked as resolved and limited conversation to collaborators Jul 28, 2021

ericstj added this to the 6.0.0 milestone Sep 30, 2021

ericstj assigned bartonjs Sep 30, 2021

bartonjs removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 30, 2021

bartonjs removed their assignment Sep 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Obsolete X509Certificate2.PrivateKey and PublicKey.Key. #54562

Obsolete X509Certificate2.PrivateKey and PublicKey.Key. #54562

vcsjones commented Jun 22, 2021

dotnet-issue-labeler bot commented Jun 22, 2021

ghost commented Jun 22, 2021

ghost commented Jun 22, 2021

vcsjones commented Jun 22, 2021 •

edited

Loading

bartonjs Jun 22, 2021

vcsjones Jun 22, 2021

vcsjones Jun 22, 2021

bartonjs Jun 22, 2021

vcsjones Jun 22, 2021 •

edited

Loading

terrajobst Jun 24, 2021 •

edited

Loading

vcsjones Jun 25, 2021

bartonjs commented Jun 22, 2021

azure-pipelines bot commented Jun 22, 2021

vcsjones commented Jun 23, 2021

vcsjones commented Jun 26, 2021

bartonjs commented Jun 28, 2021

	internal const string KeyPropertiesMessage = "Key and PrivateKey are obsolete. Use the appropriate method to get the public key, or create a new instance with a different private key.";
	internal const string KeyPropertiesMessage = "PublicKey.Key and X509Certificate2.PrivateKey are obsolete. Use the appropriate method to get the key (such as GetRSAPublicKey or GetRSAPrivateKey), or use the CopyWithPrivateKey method to create a new instance with a private key.";

Obsolete X509Certificate2.PrivateKey and PublicKey.Key. #54562

Obsolete X509Certificate2.PrivateKey and PublicKey.Key. #54562

Conversation

vcsjones commented Jun 22, 2021

dotnet-issue-labeler bot commented Jun 22, 2021

ghost commented Jun 22, 2021

ghost commented Jun 22, 2021

vcsjones commented Jun 22, 2021 • edited Loading

bartonjs Jun 22, 2021

Choose a reason for hiding this comment

vcsjones Jun 22, 2021

Choose a reason for hiding this comment

vcsjones Jun 22, 2021

Choose a reason for hiding this comment

bartonjs Jun 22, 2021

Choose a reason for hiding this comment

vcsjones Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

terrajobst Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

vcsjones Jun 25, 2021

Choose a reason for hiding this comment

bartonjs commented Jun 22, 2021

azure-pipelines bot commented Jun 22, 2021

vcsjones commented Jun 23, 2021

vcsjones commented Jun 26, 2021

bartonjs commented Jun 28, 2021

vcsjones commented Jun 22, 2021 •

edited

Loading

vcsjones Jun 22, 2021 •

edited

Loading

terrajobst Jun 24, 2021 •

edited

Loading