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

[API Proposal]: System.Security.Cryptography.Oid should have a ToString() implementation #109366

Open
0xced opened this issue Oct 30, 2024 · 5 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity
Milestone

Comments

@0xced
Copy link
Contributor

0xced commented Oct 30, 2024

Background and motivation

The ToString() method of the Oid class currently always prints System.Security.Cryptography.Oid which is pretty much useless.

A nicer implementation would display the oid value and the friendly name if available.

API Proposal

namespace System.Security.Cryptography;

public sealed class Oid
{
    public override string ToString()
    {
        return Value?.Length > 0 && FriendlyName?.Length > 0 ? $"{Value} ({FriendlyName})" : Value ?? "";
    }
}

API Usage

var dsaOid = new Oid("1.3.14.3.2.12");
Console.WriteLine(dsaOid.ToString()); // prints "1.3.14.3.2.12 (DSA)"

var dummyOid = new Oid("1.2.3");
Console.WriteLine(dummyOid.ToString()); // prints "1.2.3"

var emptyOid = new Oid();
Console.WriteLine(emptyOid.ToString()); // prints "" (empty string)

Alternative Designs

Not applicable.

Risks

Probably very low.

@0xced 0xced added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 30, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Oct 30, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

@vcsjones
Copy link
Member

There are some edge cases here that probably need to be considered.

First, it is possible to have an Oid that has a friendly name, but not a value. For example:

using System;
using System.Security.Cryptography;

#pragma warning disable CA1416 // Validate platform compatibility

using ECDiffieHellmanCng ecdh = new(ECCurve.CreateFromFriendlyName("Curve25519"));
ECParameters p = ecdh.ExportParameters(false);
Console.WriteLine(p.Curve.Oid.FriendlyName);
Console.WriteLine(p.Curve.Oid.Value);

On Windows, Curve25519 is a named-only curve and has no Value. Or more simply, just doing Oid oid = new(null, "Potato");. But the Curve25519 highlights that there is an actual real-world case for this happening.

We could do something like this:

public override string ToString()
{
    return (Value, FriendlyName) switch
    {
        (string v, string f) => $"{v} ({f})",
        (string v, null) => v,
        (null, string f) => f,
        (null, null) => string.Empty
    };
}

But it has a certain amount of ambiguity to the output that I don't love.

Second, and this might be fixed now, is that touching FriendlyName on Windows can have surprisingly bad perf characteristics on first-use, which would apply to ToString.

@bartonjs bartonjs added this to the Future milestone Oct 30, 2024
@bartonjs
Copy link
Member

Given that there are 4 possible output formats (as demonstrated by @vcsjones)... what are you hoping to do with this information?

Basically, any runtime ToString() of this type is going to be problematic. A calling application/library should be looking at the constituent parts and handling null in a way that makes sense to their context. If you just want it for the debugger, then it might be a reasonable Debugger-Display value (while .ToString() is the default, the debugger-display value can be different)

@bartonjs bartonjs added needs-author-action An issue or pull request that requires more info or actions from the author. and removed untriaged New issue has not been triaged by the area owner labels Oct 30, 2024
Copy link
Contributor

This issue has been marked needs-author-action and may be missing some important information.

Copy link
Contributor

This issue has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity
Projects
None yet
Development

No branches or pull requests

3 participants