dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.76k forks source link

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

Open 0xced opened 3 weeks ago

0xced commented 3 weeks ago

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.

dotnet-policy-service[bot] commented 3 weeks ago

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

vcsjones commented 3 weeks ago

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 commented 3 weeks ago

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)

dotnet-policy-service[bot] commented 2 weeks ago

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

dotnet-policy-service[bot] commented 2 days ago

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.