Conformance fix: Remove trustpath subject-issuer comparison

[abergs]

Merging this would make the library pass all conformance tests (for the 1.7.20 4 tool)

This was originally written by @googyi (in #456), but due to lack of review in #531 we couldn't merge it.

@googyi Could you help explain this change?

Notworthy comment from @aseigler:

The check to see if the subject and the issuer are the same is based on logic described in datatracker.ietf.org/doc/html/rfc5280#section-3.2. I thought we had already covered this before, where the batch certificate referenced in the metadata is not a root...I'll have to review this more thoroughly.

These are the values that are submitted during Conformance testing:

Subject:
L=Wakefield, S=MY, C=US, OU=Authenticator Attestation, O=FIDO Alliance, [email protected], CN=FIDO2 BATCH KEY prime256v1

Issuer:
L=Wakefield, S=MY, C=US, OU=CWG, O=FIDO Alliance, [email protected], CN=FIDO2 TEST ROOT

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.40%. Comparing base (ea6cd8f) to head (ef232f8).
Report is 3 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
+ Coverage   76.96%   78.40%   +1.44%     
==========================================
  Files         100      100              
  Lines        2613     2797     +184     
  Branches      441      507      +66     
==========================================
+ Hits         2011     2193     +182     
+ Misses        496      493       -3     
- Partials      106      111       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[abergs]

I've tried my best to iron this out, although I confess I'm not experienced in certs/trust path validation etc.

Looking at the comments (that are pasted from the linked MDS spec) - it seems the code is a bit too restrictive currently?

The relevant part from the MDS spec (also commented in the code already):

attestationRootCertificates
Each element of this array represents a PKIX [RFC5280] X.509 certificate that is a valid trust anchor for this authenticator model.
Multiple certificates might be used for different batches of the same model.
The array does not represent a certificate chain, but only the trust anchor of that chain.
A trust anchor can be a root certificate, an intermediate CA certificate or even the attestation certificate itself.

Since this comment appears to be from the FIDO metadata specification, specifically around MetadataStatement.attestationRootCertificates, I believe it explicitly states that the trust anchor doesn't have to be self-signed (subject = issuer) - it can be an intermediate CA certificate or the attestation certificate itself, which would typically not have matching subject and issuer fields.

The thumbprints of the trustpath.0 and rootCert does line up, so it's only the first if that is questionably too restrictive.

Again, here's the code with what I think is an invalid comment:

        // Let's check the simplest case first.  If subject and issuer are the same, and the attestation cert is in the list, that's all the validation we need

        // We have the same singular root cert in trustpath and it is in attestationRootCertificates
        if (trustPath.Length == 1 && trustPath[0].Subject.Equals(trustPath[0].Issuer, StringComparison.Ordinal))
        {
            foreach (X509Certificate2 cert in attestationRootCertificates)
            {
                if (cert.Thumbprint.Equals(trustPath[0].Thumbprint, StringComparison.Ordinal))
                {
                    return true;
                }
            }
        }

[abergs]

@aseigler Does my analysis above make sense?