Improve error handling

[iamcarbon]

In addition to addressing #340 (and superseding #341) to address the merge conflict:

This PR:

• Improves attnCert property validation and eliminates extra dictionary lookups
• Moves more error messages to Fido2ErrorMessages
• Eliminates unneeded LINQ casts [.NET added strongly typed enumerators]
• [BREAKING] Makes AttestationVerifier{attStmt,authenticatorData,clientDataHash} private protected. If needed by consumers, we should expose over public properties.

[iamcarbon]

@abergs @aseigler Ready for review / feedback.

Please note the field visibility changes made in AttestationVerifier and thoughts on whether we should expose new public properties.

[codecov-commenter]

Codecov Report

Merging #344 (835978e) into master (9823ec0) will increase coverage by 0.07%.
The diff coverage is 91.05%.

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
+ Coverage   77.87%   77.95%   +0.07%     
==========================================
  Files          88       88              
  Lines        2495     2513      +18     
  Branches      415      421       +6     
==========================================
+ Hits         1943     1959      +16     
  Misses        439      439              
- Partials      113      115       +2     

Impacted Files	Coverage Δ
Src/Fido2/CryptoUtils.cs	88.17% <ø> (-0.13%)	⬇️
Src/Fido2/AttestationFormat/AndroidKey.cs	95.23% <77.77%> (-0.06%)	⬇️
Src/Fido2/AttestationFormat/AttestationVerifier.cs	85.00% <85.00%> (ø)
Src/Fido2/AttestationFormat/AndroidSafetyNet.cs	98.79% <100.00%> (-0.02%)	⬇️
Src/Fido2/AttestationFormat/Apple.cs	93.54% <100.00%> (ø)
Src/Fido2/AttestationFormat/AppleAppAttest.cs	70.83% <100.00%> (ø)
Src/Fido2/AttestationFormat/FidoU2f.cs	91.42% <100.00%> (ø)
Src/Fido2/AttestationFormat/None.cs	100.00% <100.00%> (ø)
Src/Fido2/AttestationFormat/Packed.cs	100.00% <100.00%> (+1.85%)	⬆️
Src/Fido2/AttestationFormat/Tpm.cs	99.64% <100.00%> (-0.01%)	⬇️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[aseigler]

@abergs @aseigler Ready for review / feedback.

Please note the field visibility changes made in AttestationVerifier and thoughts on whether we should expose new public properties.

I was looking to add something like the below to AttestationVerifer and move the similar code out of the AuthenticatorAttestationResponse.VerifyAsync():

public static AttestationVerifier Create(string fmt)
{
    return fmt switch
    {
        "none" => new None(),                           // https://www.w3.org/TR/webauthn/#none-attestation
        "tpm" => new Tpm(),                             // https://www.w3.org/TR/webauthn/#tpm-attestation
        "android-key" => new AndroidKey(),              // https://www.w3.org/TR/webauthn/#android-key-attestation
        "android-safetynet" => new AndroidSafetyNet(),  // https://www.w3.org/TR/webauthn/#android-safetynet-attestation
        "fido-u2f" => new FidoU2f(),                    // https://www.w3.org/TR/webauthn/#fido-u2f-attestation
        "packed" => new Packed(),                       // https://www.w3.org/TR/webauthn/#packed-attestation
        "apple" => new Apple(),                         // https://www.w3.org/TR/webauthn/#apple-anonymous-attestation
        "apple-appattest" => new AppleAppAttest(),      // https://developer.apple.com/documentation/devicecheck/validating_apps_that_connect_to_your_server 
        _ => throw new Fido2VerificationException(Fido2ErrorCode.UnknownAttestationType, $"Unknown attestation type. Was '{fmt}'")
    };
}

Then have the caller call Create(fmt) to get a verifier. Right now this only gets called from inside the attestation verifier, but with the passkey support, it can get called when a devicePublicKey extension object is found in an attestation or inside an assertion.

[abergs]

It's unfortunate that this performance boost is a little less readable. Never been a fan of out, but I think the trade off is OK, just unfortunate.

[abergs]

I've did a quick review of 12/13 files, which is fine.
I haven't looked at the big bulk of changes in Src/Fido2/AttestationFormat/AttestationVerifier.cs.

I like @aseigler approach.

[iamcarbon]

@aseigler I went ahead and moved the logic for creating AttestationVerifier instances up. Much nicer.

[aseigler]

@aseigler I went ahead and moved the logic for creating AttestationVerifier instances up. Much nicer.

So glad you like it. I have been staring at that area for the past....I want to say, 4 years, and keep thinking "there's got to be a better way!" but didn't really see it until the DPK work really forced the issue. I will try my best to get this merged early this weekend.

[abergs]

Indeed, really nice.