Use HashSet/SearchValues when known OID list is long

[PranavSenthilnathan]

When the list of OIDs is long (SLH-DSA and Composite ML-DSA), use a more efficient lookup instead of Array.IndexOf.

[Copilot]

Pull Request Overview

This PR optimizes OID lookup performance by replacing Array.IndexOf with more efficient lookup mechanisms when dealing with long lists of known OIDs. The change introduces a new IStringLookup interface with implementations using SearchValues<string> for .NET 9+ and HashSet<string> for earlier versions, while maintaining StringLookupArray for short lists.

Key changes:

• Introduces IStringLookup interface and optimized implementations for string lookups
• Updates cryptographic classes to use the new lookup mechanism for OID validation
• Conditionally uses SearchValues or HashSet based on target framework for better performance

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
KeyFormatHelper.cs	Adds IStringLookup interface and implementations, updates method signatures to use the interface
KeyFormatHelper.Encrypted.cs	Updates method signatures to accept IStringLookup instead of string arrays
SlhDsa.cs	Replaces string array with StringLookup for 12 known OIDs
CompositeMLDsa.cs	Replaces string array with StringLookup for 18 known OIDs
ECAlgorithm.cs	Replaces string array with StringLookupArray for single OID
RSAKeyFormatHelper.cs	Replaces string array with StringLookupArray for single OID
MLKem.cs	Replaces string array with StringLookupArray for 3 OIDs
MLDsa.cs	Replaces string array with StringLookupArray for 3 OIDs
EccKeyFormatHelper.cs	Replaces string array with StringLookupArray for single OID
DSAKeyFormatHelper.cs	Replaces string array with StringLookupArray for single OID

[Copilot]

The StringLookupArray class stores the backing array but doesn't make a defensive copy. Consider making a copy of the input array to prevent external modifications that could affect lookup behavior.

Suggested change

	public bool Contains(string value) => Array.IndexOf(backingArray, value) >= 0;
	private readonly string[] _backingArray = (string[])backingArray.Clone();
	public bool Contains(string value) => Array.IndexOf(_backingArray, value) >= 0;

[dotnet-policy-service]

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

[vcsjones]

Are there any benchmarks showing this is a worthwhile change?

In the grand scheme of things, the OID lookups are done during PKCS#8 and SPKI import, so I would generally expect these to be only a small fraction of the actual key processing.

[MihaZupan]

Despite the name, SearchValues is not beneficial in this case.
SearchValues<string>.Contains(string) is currently backed by a HashSet<string>.Contains(string).

The only searches this is being used for are setOfValues.Contains(string), this is what HashSet/FrozenSet excel at.
You'd want to use SearchValues if you were looking for substrings within a text, i.e. someText.ContainsAny(setOfValues).

[PranavSenthilnathan]

Are there any benchmarks showing this is a worthwhile change?

In the grand scheme of things, the OID lookups are done during PKCS#8 and SPKI import, so I would generally expect these to be only a small fraction of the actual key processing.

I haven't run benchmarks but I expect the same.

Regardless of perf, I think we should still get rid of the list for SLH-DSA and Composite ML-DSA and use SlhDsaAlgorithm.GetAlgorithmFromOid/CompositeMLDsaAlgorithm.GetAlgorithmFromOid as the single source of truth instead of duplicating the OIDs.

[bartonjs]

This looks like the kind of change that feels like a perf win, but is actually a loss. N isn't very big here, and the affected methods run very few times in a real world program.

The comment suggesting that we stop using a duplicate list and just use an existing lookup function sounds reasonable; but is not a priority unless the change itself is trivially small.

[PranavSenthilnathan]

Closing since there's no significant perf benefit.