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

Obsolete CryptoConfig.EncodeOID #55579

Closed
vcsjones opened this issue Jul 13, 2021 · 4 comments · Fixed by #55592
Closed

Obsolete CryptoConfig.EncodeOID #55579

vcsjones opened this issue Jul 13, 2021 · 4 comments · Fixed by #55592
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@vcsjones
Copy link
Member

vcsjones commented Jul 13, 2021

Background and Motivation

Inspired by #54087. We have CryptoConfig.EncodeOID to encoding an OID to ASN.1 DER. However it has a number of quirks in the way that it parses strings.

  • It does not handle the first two arcs correctly (e.g. 41.41.41.41)
  • "Supports" negative arcs.
  • Doesn't handle large integers.
  • etc

Now that there is System.Formats.Asn1, we have better tools for developers to use. I don't see any place within the Libraries that require developers to give us those bytes from EncodeOID, so there does not seem to be a case to keep using it, unless you are unintentionally giving it invalid OIDs.

Proposed API

namespace System.Security.Cryptography {
    public partial class CryptoConfig {
+       [Obsolete("EncodeOID is obsolete. Use the ASN.1 functionality provided in System.Formats.Asn1.")]
        public static byte[] EncodeOID(string str);
    }
}
@vcsjones vcsjones added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 13, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Jul 13, 2021
@ghost
Copy link

ghost commented Jul 13, 2021

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

Issue Details

Background and Motivation

Inspired by #54087. We have CryptoConfig.EncodeOID to encoding an OID to ASN.1 DER. However it has a number of quirks in the way that it parses strings.

  • It does not handle the first two arcs correctly (e.g. 41.41.41.41)
  • "Supports" negative arcs.
  • Doesn't handle large integers.
  • etc

Now that there is System.Formats.Asn1, we have better tools for developers to use. I don't see any place within the Libraries that require developers to give us those bytes from EncodeOID, so there does not seem to be a case to keep using it, unless you are unintentionally giving it invalid OIDs.

Proposed API

namespace System.Security.Cryptography {
    public partial class CryptoConfig {
+       [Obsolete("EncodeOID is obsolete. Use the ASN.1 functionality provided in to System.Formats.Asn1 package.")]
        public static byte[] EncodeOID(string str);
    }
}

<table>
  <tr>
    <th align="left">Author:</th>
    <td>vcsjones</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>-</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`api-suggestion`, `area-System.Security`, `untriaged`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>-</td>
  </tr>
</table>
</details>

@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 13, 2021
@bartonjs bartonjs added this to the 6.0.0 milestone Jul 13, 2021
@bartonjs bartonjs removed the untriaged New issue has not been triaged by the area owner label Jul 13, 2021
@terrajobst
Copy link
Contributor

  • Looks good as proposed
  • Just dispense another diagnostic ID :-)
namespace System.Security.Cryptography
{
    public partial class CryptoConfig
    {
        [Obsolete("EncodeOID is obsolete. Use the ASN.1 functionality provided in System.Formats.Asn1.")]
        public static byte[] EncodeOID(string str);
    }
}

@terrajobst terrajobst 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 Jul 13, 2021
@vcsjones vcsjones self-assigned this Jul 13, 2021
@bartonjs
Copy link
Member

@vcsjones From the self assignment I assume you saw I set this as 6.0 and are starting on it now. 😄

@vcsjones
Copy link
Member Author

vcsjones commented Jul 13, 2021

From the self assignment I assume you saw I set this as 6.0 and are starting on it now. 😄

Almost done.

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

3 participants