Skip to content

Obsolete X509Certificate2.PrivateKey and PublicKey.Key. #54562

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.

Sign up for GitHub

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
bartonjs merged 4 commits into from
Jun 28, 2021
Merged

Obsolete X509Certificate2.PrivateKey and PublicKey.Key. #54562

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
953e699
Obsolete X509Certificate2.PrivateKey and PublicKey.Key.
vcsjones Jun 22, 2021
7a5a8ea
Fix test compilation
vcsjones Jun 22, 2021
5df8b95
Split the diagnostics to make the message easier to action.
vcsjones Jun 22, 2021
1e3348a
Merge remote-tracking branch 'ms/main' into 47977-obsolete-key-props
vcsjones Jun 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/project/list-of-diagnostics.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ The PR that reveals the implementation of the `<IncludeInternalObsoleteAttribute
| __`SYSLIB0024`__ | Creating and unloading AppDomains is not supported and throws an exception. |
| __`SYSLIB0025`__ | SuppressIldasmAttribute has no effect in .NET 6.0+. |
| __`SYSLIB0026`__ | X509Certificate and X509Certificate2 are immutable. Use the appropriate constructor to create a new certificate. |
| __`SYSLIB0027`__ | Key and PrivateKey are obsolete. Use the appropriate method to get the public key, or create a new instance with a different private key. |

## Analyzer Warnings

Expand Down
3 changes: 3 additions & 0 deletions src/libraries/Common/src/System/Obsoletions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.";
Copy link
Member

@bartonjs bartonjs Jun 22, 2021

Choose a reason for hiding this comment

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

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.";

Copy link
Member Author

@vcsjones vcsjones Jun 22, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@vcsjones vcsjones Jun 22, 2021

Choose a reason for hiding this comment

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

Or maybe I am thinking too hard about this.

Copy link
Member

@bartonjs bartonjs Jun 22, 2021

Choose a reason for hiding this comment

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

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 ?

Copy link
Member Author

@vcsjones vcsjones Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@terrajobst terrajobst Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@vcsjones vcsjones Jun 25, 2021

Choose a reason for hiding this comment

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

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

internal const string KeyPropertiesDiagId = "SYSLIB0027";
}
}
2 changes: 1 addition & 1 deletion src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ internal static (X509Certificate2 certificate, X509Certificate2Collection) Gener
intermedPub3.Dispose();
CertificateAuthority intermediateAuthority3 = new CertificateAuthority(intermedCert3, null, null, null);

RSA eeKey = (RSA)endEntity.PrivateKey;
RSA eeKey = endEntity.GetRSAPrivateKey();
endEntity = intermediateAuthority3.CreateEndEntity(
$"CN=\"A SSL Test\", O=\"testName\"",
eeKey,
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Security.Cryptography.Pkcs/tests/EnvelopedCms/DecryptTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,7 +841,7 @@ internal void VerifySimpleDecrypt(byte[] encodedMessage, CertLoader certLoader,
using (X509Certificate2 pubCert = certLoader.GetCertificate())
{
RecipientInfo recipient = ecms.RecipientInfos.Cast<RecipientInfo>().Where((r) => r.RecipientIdentifier.MatchesCertificate(cert)).Single();
ecms.Decrypt(recipient, cert.PrivateKey);
ecms.Decrypt(recipient, cert.GetRSAPrivateKey());
}
}
else
Expand Down
2 changes: 2 additions & 0 deletions ...curity.Cryptography.X509Certificates/ref/System.Security.Cryptography.X509Certificates.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public PublicKey(System.Security.Cryptography.AsymmetricAlgorithm key) { }
public PublicKey(System.Security.Cryptography.Oid oid, System.Security.Cryptography.AsnEncodedData parameters, System.Security.Cryptography.AsnEncodedData keyValue) { }
public System.Security.Cryptography.AsnEncodedData EncodedKeyValue { get { throw null; } }
public System.Security.Cryptography.AsnEncodedData EncodedParameters { get { throw null; } }
[System.ObsoleteAttribute("Key and PrivateKey are obsolete. Use the appropriate method to get the public key, or create a new instance with a different private key.", DiagnosticId = "SYSLIB0027", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
public System.Security.Cryptography.AsymmetricAlgorithm Key { get { throw null; } }
public System.Security.Cryptography.Oid Oid { get { throw null; } }
public static System.Security.Cryptography.X509Certificates.PublicKey CreateFromSubjectPublicKeyInfo(System.ReadOnlySpan<byte> source, out int bytesRead) { throw null; }
Expand Down Expand Up @@ -253,6 +254,7 @@ public X509Certificate2(string fileName, string? password, System.Security.Crypt
public System.Security.Cryptography.X509Certificates.X500DistinguishedName IssuerName { get { throw null; } }
public System.DateTime NotAfter { get { throw null; } }
public System.DateTime NotBefore { get { throw null; } }
[System.ObsoleteAttribute("Key and PrivateKey are obsolete. Use the appropriate method to get the public key, or create a new instance with a different private key.", DiagnosticId = "SYSLIB0027", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
public System.Security.Cryptography.AsymmetricAlgorithm? PrivateKey { get { throw null; } set { } }
public System.Security.Cryptography.X509Certificates.PublicKey PublicKey { get { throw null; } }
public byte[] RawData { get { throw null; } }
Expand Down
2 changes: 2 additions & 0 deletions ...ptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/PublicKey.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Buffers;
using System.Formats.Asn1;
using System.Runtime.InteropServices;
Expand Down Expand Up @@ -61,6 +62,7 @@ public PublicKey(AsymmetricAlgorithm key)

public AsnEncodedData EncodedParameters { get; private set; }

[Obsolete(Obsoletions.KeyPropertiesMessage, DiagnosticId = Obsoletions.KeyPropertiesDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
public AsymmetricAlgorithm Key
{
get
Expand Down
1 change: 1 addition & 0 deletions ...hy.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Certificate2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ public bool HasPrivateKey
}
}

[Obsolete(Obsoletions.KeyPropertiesMessage, DiagnosticId = Obsoletions.KeyPropertiesDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
public AsymmetricAlgorithm? PrivateKey
{
get
Expand Down
2 changes: 1 addition & 1 deletion ...ography.X509Certificates/tests/System.Security.Cryptography.X509Certificates.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<DefineConstants>$(DefineConstants);HAVE_THUMBPRINT_OVERLOADS</DefineConstants>
<DefineConstants Condition="'$(TargetsUnix)' == 'true'">$(DefineConstants);Unix</DefineConstants>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<NoWarn>$(NoWarn);SYSLIB0026</NoWarn>
<NoWarn>$(NoWarn);SYSLIB0026;SYSLIB0027</NoWarn>
<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)-Android;$(NetCoreAppCurrent)-Browser;$(NetCoreAppCurrent)-OSX;$(NetCoreAppCurrent)-iOS;$(NetCoreAppCurrent)-tvOS</TargetFrameworks>
</PropertyGroup>
<PropertyGroup>
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,7 @@ public void DigestValue_CRLF()

X509Certificate2 cert = new X509Certificate2(_pkcs12, "mono");
SignedXml signedXml = new SignedXml(doc);
signedXml.SigningKey = cert.PrivateKey;
signedXml.SigningKey = cert.GetRSAPrivateKey();
signedXml.SignedInfo.CanonicalizationMethod = SignedXml.XmlDsigExcC14NTransformUrl;
signedXml.SignedInfo.SignatureMethod = SignedXml.XmlDsigRSASHA1Url;

Expand Down Expand Up @@ -725,7 +725,7 @@ public void DigestValue_LF()

X509Certificate2 cert = new X509Certificate2(_pkcs12, "mono");
SignedXml signedXml = new SignedXml(doc);
signedXml.SigningKey = cert.PrivateKey;
signedXml.SigningKey = cert.GetRSAPrivateKey();
signedXml.SignedInfo.SignatureMethod = SignedXml.XmlDsigRSASHA1Url;
signedXml.SignedInfo.CanonicalizationMethod = SignedXml.XmlDsigExcC14NTransformUrl;

Expand Down Expand Up @@ -970,7 +970,7 @@ static XmlDocument CreateSignedXml(X509Certificate2 cert, string canonicalizatio
XmlDocument doc = CreateSomeXml(lineFeed);

SignedXml signedXml = new SignedXml(doc);
signedXml.SigningKey = cert.PrivateKey;
signedXml.SigningKey = cert.GetRSAPrivateKey();
signedXml.SignedInfo.CanonicalizationMethod = canonicalizationMethod;
signedXml.SignedInfo.SignatureMethod = SignedXml.XmlDsigRSASHA1Url;

Expand Down