conformance 1.7.20 4 #531
conformance 1.7.20 4 #531
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #531 +/- ##
==========================================
+ Coverage 73.95% 74.62% +0.66%
==========================================
Files 98 102 +4
Lines 2638 2719 +81
Branches 446 464 +18
==========================================
+ Hits 1951 2029 +78
+ Misses 586 582 -4
- Partials 101 108 +7 ☔ View full report in Codecov by Sentry. |
| if (trustPath.Length == 1 && trustPath[0].Subject.Equals(trustPath[0].Issuer, StringComparison.Ordinal)) | ||
|
|
||
| // We have the same singular root cert in trustpath and it is in attestationRootCertificates | ||
| if (trustPath.Length == 1) |
There was a problem hiding this comment.
@aseigler Is it really ok to not equality compare the path?
Without this we get:
I could throw in a || conformanceTesting-related check
There was a problem hiding this comment.
The check to see if the subject and the issuer are the same is based on logic described in https://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.
There was a problem hiding this comment.
@abergs I saw that we removed the check to make sure the trustPath's root subject is equal to the issuer.
What does the conformance test use for the Subject and Issuer (where they don't match). Is there a case, in production, where these values would also not match? Are they completely different values, or just encoded differently?
There was a problem hiding this comment.
@iamcarbon Not sure - I haven't looked into it.
| if (trustPath.Length > 1 && attestationRootCertificates.Any(c => string.Equals(c.Thumbprint, trustPath[^1].Thumbprint, StringComparison.Ordinal))) | ||
| { | ||
| throw new Fido2VerificationException(Fido2ErrorMessages.InvalidCertificateChain); | ||
| } |
There was a problem hiding this comment.
@aseigler Would appreciate your eyes here too, but to me it just looks like we have the addition of above if-block that is more strict than without it.
There was a problem hiding this comment.
@abergs Are we able to reference the rule in https://datatracker.ietf.org/doc/html/rfc5280 that this new condition enforces?
| Assert.False(CryptoUtils.ValidateTrustChain(trustPath, trustPath)); | ||
| Assert.False(CryptoUtils.ValidateTrustChain(attestationRootCertificates, attestationRootCertificates)); | ||
| Assert.True(CryptoUtils.ValidateTrustChain(trustPath, trustPath)); | ||
| Assert.True(CryptoUtils.ValidateTrustChain(attestationRootCertificates, attestationRootCertificates)); |
|
@aseigler See mentions. If you don't have time too look at them at the moment I might move those changes to a secondary PR wand wait for your review, while still landing some of these fixes. |
abergs
force-pushed
the
conformance-1.7.20-4
branch
from
1de524b
to
745bcdb
Compare
TrustAnchor.cs : 32 Server-ServerAuthenticatorAttestationResponse-Resp-5 Test server processing "packed" FULL attestation F-10 Send ServerAuthenticatorAttestationResponse with FULL "packed" attestation, with attStmt.x5c containing full chain, and check that server returns an error https://datatracker.ietf.org/doc/html/rfc5280#section-6.1 AuthenticatorAttestationRawResponse.cs : 18 Server-ServerAuthenticatorAttestationResponse-Resp-1 Test server processing ServerAuthenticatorAttestationResponse structure F-4 Send ServerAuthenticatorAttestationResponse that is missing "type" field and check that server returns an error CredentialCreateOptions.cs : 96 Server-ServerAuthenticatorAttestationResponse-Resp-4 Test server support of the authentication algorithms P-8 Send a valid ServerAuthenticatorAttestationResponse with SELF "packed" attestation, for "ALG_SIGN_RSASSA_PKCSV15_SHA1_RAW" aka "RS1" algorithm, and check that server succeeds Server-ServerAuthenticatorAttestationResponse-Resp-9 Test server processing "tpm" attestation P-2 Send a valid ServerAuthenticatorAttestationResponse with "tpm" attestation for SHA-1, and check that server succeeds CredentialCreateOptions.cs : 210 Server-ServerPublicKeyCredentialCreationOptions-Req-1 Test server generating ServerPublicKeyCredentialCreationOptionsRequest P-1 Get ServerPublicKeyCredentialCreationOptionsResponse, and check that: (a) response MUST contain ... AuthenticationExtensionsClientInputs.cs : 23 public string AppID { private get; set; } Server-ServerPublicKeyCredentialGetOptionsResponse-Req-1 Test server generating ServerPublicKeyCredentialGetOptionsResponse P-1 Get ServerPublicKeyCredentialGetOptionsResponse, and check that: (a) response MUST contain ... AuthenticationExtensionsClientInputs.cs : 44 public bool? UserVerificationMethod { private get; set; } Server-ServerPublicKeyCredentialGetOptionsResponse-Req-1 Test server generating ServerPublicKeyCredentialGetOptionsResponse P-1 Get ServerPublicKeyCredentialGetOptionsResponse, and check that: (a) response MUST contain ... AuthenticatorAssertionResponse.cs : 128 Server-ServerAuthenticatorAssertionResponse-Resp-3 P4,P6,P7 CryptoUtils.cs 64 (trustpath length 1 with exact match in attestation root certs) Server-ServerAuthenticatorAttestationResponse-Resp-5 Test server processing "packed" FULL attestation P-3 Send a valid ServerAuthenticatorAttestationResponse with FULL "packed" attestation that contains batch certificate, that is simply self referenced in the metadata, and check that server succeeds CryptoUtils.cs 105 - X509RevocationMode.Online makes conformance sad Server-ServerAuthenticatorAttestationResponse-Resp-9 Test server processing "tpm" attestation P-1 Send a valid ServerAuthenticatorAttestationResponse with "tpm" attestation for SHA-256, and check that server succeeds‣ P-2 Send a valid ServerAuthenticatorAttestationResponse with "tpm" attestation for SHA-1, and check that server succeeds‣ P-3 Send a valid ServerAuthenticatorAttestationResponse with "tpm" attestation pubArea.nameAlg is not matching algorithm used for generate attested.name, and check that server succeeds TestController.cs tojson -> serialize serialization error
Json serialization fix. (Object type vs ToJson())
Back to 100% conformance. TokenBinding logic readded. AppId: prevent serialization in a nicer way. UV flags are verified differently for conformance testing, otherwise as described in the RFC.
fix azure pipeline's whitespace error + removing unused using
Improve trustanchor test coverage based on codecov report
abergs
force-pushed
the
conformance-1.7.20-4
branch
from
745bcdb
to
11121e6
Compare
| namespace Fido2NetLib | ||
| { |
There was a problem hiding this comment.
Consider using file scoped namespace
| @@ -49,7 +49,7 @@ public static HashAlgorithmName HashAlgFromCOSEAlg(COSE.Algorithm alg) | |||
| }; | |||
| } | |||
|
|
|||
| public static bool ValidateTrustChain(X509Certificate2[] trustPath, X509Certificate2[] attestationRootCertificates) | |||
| public static bool ValidateTrustChain(X509Certificate2[] trustPath, X509Certificate2[] attestationRootCertificates, bool conformance = false) | |||
There was a problem hiding this comment.
Should we introduce a new enum, to control any quirks or tool / specification specific validation behaviors?
e.g.
enum FidoValidationMode
{
WebAuthNLevel2,
WebAuthNLevel3,
FidoConformance2024,
FidoConformance2025,
Default = WebAuthNLevel3
}
```csharp
public static bool ValidateTrustChain(X509Certificate2[] trustPath, X509Certificate2[] attestationRootCertificates, FidoValidationMode validationMode = FidoValidationMode.Default)
{
..
}| /// <param name="cancellationToken">The <see cref="CancellationToken"/> used to propagate notifications that the operation should be canceled.</param> | ||
| /// <returns></returns> | ||
| public async Task<RegisteredPublicKeyCredential> MakeNewCredentialAsync( | ||
| AuthenticatorAttestationRawResponse attestationResponse, | ||
| CredentialCreateOptions originalOptions, | ||
| IsCredentialIdUniqueToUserAsyncDelegate isCredentialIdUniqueToUser, | ||
| byte[]? requestTokenBindingId = null, |
There was a problem hiding this comment.
Should this be moved to CredentialCreateOptions with an [EditorBrowsable(EditorBrowsableState.Never)] attribute + used for Fido2 2024 conformance testing only.
There was a problem hiding this comment.
That's an interesting alternative to get it less visible!
| IReadOnlyList<byte[]> storedDevicePublicKeys, | ||
| uint storedSignatureCounter, | ||
| IsUserHandleOwnerOfCredentialIdAsync isUserHandleOwnerOfCredentialIdCallback, | ||
| byte[]? requestTokenBindingId = null, |
There was a problem hiding this comment.
Should we use an object for the parameters, so we can continue to add new options in the future without breaking this method?
public class MakeAssertionRequest
{
public required IReadOnlyList<byte[]> StoredDevicePublicKeys { get; init; }
public required uint StoredSignatureCounter { get; init; }
// for Fido2 2024 conformance testing only
[EditorBrowsable(EditorBrowsableState.Never)]
public byte[]? RequestTokenBindingId { get; init; }
}
| Task<RegisteredPublicKeyCredential> MakeNewCredentialAsync( | ||
| AuthenticatorAttestationRawResponse attestationResponse, | ||
| CredentialCreateOptions originalOptions, | ||
| IsCredentialIdUniqueToUserAsyncDelegate isCredentialIdUniqueToUser, | ||
| byte[]? requestTokenBindingId = null, | ||
| CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
Consider making a new object for the parameters so we can continue to add or remove parameters in the future without a binary breaking change.
public class MakeNewCredentialRequest
{
public required AuthenticatorAttestationRawResponse AttestationResponse { get; init; }
public required CredentialCreateOptions OriginalOptions { get; init; }
// for Fido2 2024 conformance testing only
[EditorBrowsable(EditorBrowsableState.Never)]
public byte[]? RequestTokenBindingId { get; init; }
}|
|
||
| public static class TrustAnchor | ||
| { | ||
| public static void Verify(MetadataBLOBPayloadEntry? metadataEntry, X509Certificate2[] trustPath, bool conformance) |
There was a problem hiding this comment.
consider:
public static void Verify(MetadataBLOBPayloadEntry? metadataEntry, X509Certificate2[] trustPath, FidoValidationMode validationMode = FidoValidationMode.Default)
This is based on #456 and results in a 100% pass on the v1.7.20-4 tool.