-
-
Notifications
You must be signed in to change notification settings - Fork 180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIDO Conformance Tools v1.7.15 fixes #456
Conversation
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
@@ -20,7 +20,12 @@ public sealed class AuthenticationExtensionsClientInputs | |||
/// </summary> | |||
[JsonPropertyName("appid")] | |||
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | |||
public string AppID { get; set; } | |||
public string AppID { private get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you put the getter private and add a method to retrieve the value of the app id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these private getters are used to prevent serialization in response. AppId and Uwm were not welcome by some asserts in the test tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here @aseigler -- Not sure about this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The attribute [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
is there to avoid the prop to be serialized if unset. The private getter should not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the conformance test tool, AppID should not be part of the response at all. There is a specific testcase for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Fido guys are only slowly following the RFC, the token binding is a good example for this. I had to put it back to pass their tests.
In some places they don't even want to follow the RFC,
For example: googyi@84c9909
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a private branch with a bunch of tweaks that you are running this on? I don't see how you are possibly legitimately passing these without heavy changes and special workarounds. One of the fundamental issues with the way the tool works is the tests are a simple pass/fail -- which sounds simple on the surface, but for the failure case you actually have to be failing for the right reason, which you can only verify (in my experience) by menu -> open inspector and watching the responses to make sure they all line up with the appropriate test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think everything is public in my fork and you should use the "net-standard-conversion" branch.
This is my latest commit in that branch:
Just check that you don't have this [Obsolete("Use property ResidentKey.")] stuff in the code.
I have ported the state of this .net core pull request and I readded your tokenbinding implementation.
And there was a lot work with asn stuff, serialization and cert verification, etc, but your old v2 version helped a lot in this.
"which you can only verify (in my experience) by menu -> open inspector"
I agree. So far I have not seen any testcase where it failed because of the wrong reason.
These tests have no big value, unless you want to get their certification. Then you have to do what they want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That branch definitely looks like it is passing everything legitimately. Agree with your assessment in the value. I wouldn't mind trying to get the implementation certified if some organization wanted to sponsor it. I think we broke some of the FIDO tests implementing new WebAuthn features and moving away from the third party ASN.1 and CBOR libraries we were using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have pushed a few changes to this branch to reach 100%.
I don't remember any ASN.1 or CBOR related thing that broke anything.
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.
@dotnet-policy-service agree company="LastPass" |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #456 +/- ##
==========================================
+ Coverage 74.19% 74.82% +0.62%
==========================================
Files 100 104 +4
Lines 2709 2792 +83
Branches 444 463 +19
==========================================
+ Hits 2010 2089 +79
+ Misses 595 591 -4
- Partials 104 112 +8 ☔ View full report in Codecov by Sentry. |
fix azure pipeline's whitespace error + removing unused using
Improve trustanchor test coverage based on codecov report
Thank you for this PR - Closing this in favour of now merged #531 |
Thanks! I am happy that finally my work is useful for this project. |
passes: 128 failures: 19
->
passes:164 failures: 0
changes / fixed testcases
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