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

Consider deprecating or annotating a few S.S.C.X509Certificates members #47977

Closed
vcsjones opened this issue Feb 7, 2021 · 8 comments · Fixed by #54562
Closed

Consider deprecating or annotating a few S.S.C.X509Certificates members #47977

vcsjones opened this issue Feb 7, 2021 · 8 comments · Fixed by #54562
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 7, 2021

There are a few APIs in System.Security.Cryptography.X509Certificates that might be worth obsoleting, or some variation of {Un}SupportedOSPlatformAttribute on them.

  • X509Certificate2.FriendlyName - Consider marking this [SupportedOSPlatformAttribute("windows")]. This API returns an empty string on non-Windows, and throws when being set on non-Windows.
  • X509Certificate2.Archived - Consider marking this [SupportedOSPlatformAttribute("windows")]. This API returns false on non-Windows, and throws when being set on non-Windows.
  • X509Certificate.Import - Consider deprecating or obsoleting this. All overloads for this API are going to throw.
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Feb 7, 2021
@ghost
Copy link

ghost commented Feb 7, 2021

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

Issue Details

There are a few APIs in System.Security.Cryptography.X509Certificates that it might be worth obsoleting, or some variation of {Un}SupportedOSPlatformAttribute on them.

  • X509Certificate2.FriendlyName - Consider marking this [SupportedOSPlatformAttribute("windows")]. This API returns an empty string on non-Windows, and throws when being set on non-Windows.
  • X509Certificate2.Archived - Consider marking this [SupportedOSPlatformAttribute("windows")]. This API returns false on non-Windows, and throws when being set on non-Windows.
  • X509Certificate.Import - Consider deprecating or obsoleting this. All overloads for this API are going to throw.
Author: vcsjones
Assignees: -
Labels:

area-System.Security, untriaged

Milestone: -

@vcsjones
Copy link
Member Author

vcsjones commented Feb 18, 2021

I guess to "proposalify" this:

public class X509Certificate2
{
+   [SupportedOSPlatform("windows")]
    public bool Archived { get; set; }

+   [SupportedOSPlatform("windows")]
    public string FriendlyName { get; set; }

+   [Obsolete("This is no longer the recommended way of retrieving the private key. Use the appropriate GetPrivateKey or CopyWithPrivateKey method instead.")]
    public AsymmetricAlgorithm PrivateKey { get; set; }

+   [Obsolete("X509Certificate2 is immutable on this platform. Use a different constructor overload instead.")]
    public X509Certificate2();

+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(byte[] rawData);
+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(byte[] rawData, SecureString? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(byte[] rawData, string? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(string fileName);
+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(string fileName, System.Security.SecureString? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(string fileName, string? password, X509KeyStorageFlags keyStorageFlags);
}

public class X509Certificate
{
+   [Obsolete("X509Certificate is immutable on this platform. Use a different constructor overload instead.")]
    public X509Certificate();

+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor instead.")]
    public void Import(byte[] rawData);
+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor instead.")]
    public void Import(byte[] rawData, SecureString? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor instead.")]
    public void Import(byte[] rawData, string? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor instead.")]
    public void Import(string fileName);
+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor instead.")]
    public void Import(string fileName, System.Security.SecureString? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor instead.")]
    public void Import(string fileName, string? password, X509KeyStorageFlags keyStorageFlags);
}

public class PublicKey
{
+   [Obsolete("The key property is no longer recommended for use. Use the appropriate GetPublicKey method instead.")]
    public AsymmetricAlgorithm Key { get; }
}

@bartonjs
Copy link
Member

Have we put Obsolete on X509Certificate2.PrivateKey and PublicKey.Key yet? Though to fully obsolete PublicKey we would probably want to put GetRSAPublicKey/etc on it. (Or just feel we got far enough there by adding SPKI export)

@vcsjones
Copy link
Member Author

vcsjones commented Feb 19, 2021

Have we put Obsolete on

Not yet.

X509Certificate2.PrivateKey

That one makes sense to me since there are good APIs to success it, so I added it.

PublicKey.Key

I'm less convinced there is a good replacement for that. The SPKI APIs work, but feel a little clunky

using X509Certificate2 cert = new X509Certificate2();
using RSA rsa = RSA.Create();
byte[] spki = cert.PublicKey.ExportSubjectPublicKeyInfo();
rsa.ImportSubjectPublicKeyInfo(spki, out _);

Perhaps we can open a separate proposal for that (opened #48510)


Another one that strikes me as a bit weird is the parameterless constructor for X509Certificate{2}. Since Import doesn't work does it have any valid use cases? Should it get deprecated too?

@bartonjs
Copy link
Member

the parameterless constructor(s) for X509Certificate{2}... Should [they] get deprecated too?

Yeah, it just produces a certificate in the same state as one gets in after Dispose(), so it's probably not all that useful. Obsolete away!

@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Feb 26, 2021
@bartonjs bartonjs added this to the 6.0.0 milestone Feb 26, 2021
@vcsjones
Copy link
Member Author

@bartonjs Since #53127 is merged now, I added the PublicKey.Key property to the proposal for deprecation.

@bartonjs
Copy link
Member

How accidentally convenient that I skipped over this one to get the new methods in.

Thanks 😄.

@bartonjs
Copy link
Member

bartonjs commented May 25, 2021

Video

  • The obsoletions should use the next available SYSLIB number
  • The SupportedOSPlatform attributes were moved from the property to just the property setter (so the getters are still allowed on all platforms, since they don't throw).
public class X509Certificate2
{
    public bool Archived {
       get;
+   [SupportedOSPlatform("windows")]
       set;
 }

    public string FriendlyName
     {
         get;
+   [SupportedOSPlatform("windows")]
         set;
     }

+   [Obsolete("This is no longer the recommended way of retrieving the private key. Use the appropriate GetPrivateKey or CopyWithPrivateKey method instead.")]
    public AsymmetricAlgorithm PrivateKey { get; set; }

+   [Obsolete("X509Certificate2 is immutable on this platform. Use a different constructor overload instead.")]
    public X509Certificate2();

+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(byte[] rawData);
+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(byte[] rawData, SecureString? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(byte[] rawData, string? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(string fileName);
+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(string fileName, System.Security.SecureString? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate2 is immutable on this platform. Use the equivalent constructor instead.")]
    public override void Import(string fileName, string? password, X509KeyStorageFlags keyStorageFlags);
}

public class X509Certificate
{
+   [Obsolete("X509Certificate is immutable on this platform. Use a different constructor overload instead.")]
    public X509Certificate();

+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor on X509Certificate2 instead.")]
    public void Import(byte[] rawData);
+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor on X509Certificate2 instead.")]
    public void Import(byte[] rawData, SecureString? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor on X509Certificate2 instead.")]
    public void Import(byte[] rawData, string? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor on X509Certificate2 instead.")]
    public void Import(string fileName);
+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor on X509Certificate2 instead.")]
    public void Import(string fileName, System.Security.SecureString? password, X509KeyStorageFlags keyStorageFlags);
+   [Obsolete("X509Certificate is immutable on this platform. Use the equivalent constructor on X509Certificate2 instead.")]
    public void Import(string fileName, string? password, X509KeyStorageFlags keyStorageFlags);
}

public class PublicKey
{
+   [Obsolete("The key property is no longer recommended for use. Use the appropriate GetPublicKey method instead.")]
    public AsymmetricAlgorithm Key { get; }
}

@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 25, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 22, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 28, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 28, 2021
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