Add PKCS#8, SPKI and PEM support for SLH-DSA

[PranavSenthilnathan]

Add support for PKCS#8, SPKI and PEM, both encrypted and unencrypted. Test have been refactored to simplify things a little and SlhDsa.cs now has full code coverage (only Debug.Assert is not covered).

TODO:

• Add newly approved convenience APIs
• SlhDsa.cs has some TODOs in it
• Add some more roundtrip tests for encrypted PKCS#8/PEM
• Test cleanup - tests are disorganized from refactoring so I'll likely be shuffling them within (but not moving to other files).

Contributes to #113506

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[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]

This API does not "deeply" validate encoding rules, so I am not sure with the intention of taking in the encoding rules.

That is to say: if you want to validate that an ASN.1 object is DER, this will not thrown for an indefinite length encoded item inside a definite length encoded item. This is only reading the most "outer" thing.

The actual reading will validate the rules - this was just meant to make sure there was nothing trailing after the data.

[PranavSenthilnathan]

My thought process here was that in the previous code, ReadEncodedValue would throw AsnContentException (for example, when the length in the encoding was longer that the encoding itself), so either we can catch it and wrap it in a CryptoException or ArgumentException, or change it to the TryReadEncodedValue and throw. I did the latter, but if we're already doing the work of the check (even if it is non-exhaustive), it might make sense to surface it to the user as an inner exception. We can of course just let it throw the AsnContentException up too but I think the pattern in all the APIs is to wrap it in a CryptoException

[vcsjones]

Improving the exception is fine - but accepting the encoding rules and renaming the method is what I am specifically focusing on. I don't think it does anymore more than validate there is no trailing data, and the rules don't matter for that one specific thing.

[PranavSenthilnathan]

It does validate that the length field encoded in the input is not larger than the input itself (not enough data). And in the DER case it does validate that it isn't using variable length encoding. As you mentioned, these are both only for the top level structure, but so is the trailing data check.

How about ThrowIfInvalidLength or ThrowIfUnexpectedLength?

[bartonjs]

Renaming it to something like ThrowIfInvalidLength is fine, but since all callers (that I see) specify BER, it shouldn't be a parameter.

For consistency, AsnContentException should not be directly thrown.

[vcsjones]

It does validate that the length field encoded in the input is not larger than the input itself (not enough data)

The AsnReader (or AsnValueReader) is going to catch that anyway when it attempts to actually decode the structure. Why try to pre-validate it? The only reason we have the trailing data check is is because it doesn't happen automatically during reading.

[bartonjs]

The AsnReader (or AsnValueReader) is going to catch that anyway

I believe that actually is the problem. AsnDecoder.ReadEncodedValue threw an AsnContentException, this method didn't catch it, and an undocumented exception type escaped the public method.

Using TryRead avoids that exception, vs making a ThrowIf have a try/catch-wrap.

[vcsjones]

Using TryRead avoids that exception, vs making a ThrowIf have a try/catch-wrap.

Sure. My only point is that

1. The method does not actually validate the encoding in a meaningful way
2. It doesn't matter if it is BER/DER in this case.
3. It was already throwing crypto exceptions for truncated ASN.1.

[bartonjs]

it was already throwing crypto exceptions for truncated ASN.1.

If that's the case, then, full-undo. If the edge case of the AsnContentException leaking out from the top-level item having a length that exceeds the span is true, then this is fine.

[PranavSenthilnathan]

3. It was already throwing crypto exceptions for truncated ASN.1.

It throws AsnContentException: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.cs#L539-L540

2. It doesn't matter if it is BER/DER in this case.

Yes, I can relax the encoding to always be BER, but we still are going to have to wrap because of the comment above.

[bartonjs]

Please don't mix test refactorings and adding new tests. Separate changes. Don't care which is first.

(if the moves had showed as a rename, it'd be fine, but it's hard to track "did we lose coverage of this existing thing" and "do we have coverage of this new thing" at the same time)

[bartonjs]

There's too much test refactoring going on with a change that's adding functionality. They need to be split into independently reviewable pieces.

[PranavSenthilnathan]

There's too much test refactoring going on with a change that's adding functionality. They need to be split into independently reviewable pieces.

Do you have a preference on how this is done? I can rebase a commit with the refactoring to the base of this PR branch, but I'm not sure if github will mess up the existing PR feedback. Or I can just create a new PR with the refactoring and merge it back into here once it's in main.

[PranavSenthilnathan]

Revert

[bartonjs]

Do you have a preference on how this is done? I can rebase a commit with the refactoring to the base of this PR branch, but I'm not sure if github will mess up the existing PR feedback.

The refactoring/reorganizing and the addition of new features need to be in two different PRs (two different signoffs, and end up as two different commits permanently). Based on having done this recently, I'll speculate that the easiest answer is to make a new local branch based on this branch, delete all of the new stuff, then rebase it against dotnet/main (and pre-squash so it doesn't show any work happening then being undone). After that gets merged, then you can merge it into this branch and we resume looking at it after.

The other option is to just undo all of the test refactoring and show this as just new feature and new feature tests, then either never do the refactoring, or do it as a followup.