feat: expose attestation type and trust path after validation #98
Conversation
bdewater
force-pushed
the
attestation-type-trust-path
branch
from
702b48b
to
f4a834c
Compare
| ) | ||
| ) | ||
| attestation_type, attestation_trust_path = subject.valid?(authenticator_data, client_data_hash) | ||
| expect(attestation_type).to eq(WebAuthn::AttestationStatement::Base::ATTESTATION_TYPE_BASIC_OR_ATTCA) |
There was a problem hiding this comment.
I only see "Basic" as the returned type in the "Verification procedure" in https://www.w3.org/TR/2018/CR-webauthn-20180807/#packed-attestation. I may be missing something.
There was a problem hiding this comment.
This has been clarified in the latest editors draft. In the version you're linking, signing procedure step 2 does mention "If Basic or AttCA attestation is in use..."
There was a problem hiding this comment.
This has been clarified in the latest editors draft
Gotcha 👍
| if raw_attestation_certificates&.any? | ||
| [ATTESTATION_TYPE_BASIC_OR_ATTCA, attestation_certificate_chain] | ||
| elsif raw_ecdaa_key_id | ||
| [ATTESTATION_TYPE_ECDAA, raw_ecdaa_key_id] |
There was a problem hiding this comment.
We don't support ECDAA for now.
Can we please remove it from here, it will never execute this code.
| ATTESTATION_TYPE_SELF = "self" | ||
| ATTESTATION_TYPE_ATTCA = "attca" | ||
| ATTESTATION_TYPE_ECDAA = "ecdaa" | ||
| ATTESTATION_TYPE_BASIC_OR_ATTCA = "basic_or_attca" |
There was a problem hiding this comment.
W3C Recommendation seem to use first letter uppercase, e.g. None instead of none or AttCA instead of attca.
There was a problem hiding this comment.
Also, can we move these out of Base namespace?
| @@ -39,6 +39,8 @@ | |||
| ) | |||
|
|
|||
| expect(response.valid?(original_challenge, original_origin)).to eq(true) | |||
| expect(response.attestation_type).to eq(WebAuthn::AttestationStatement::Base::ATTESTATION_TYPE_BASIC_OR_ATTCA) | |||
| expect(response.attestation_trust_path).to be_kind_of(OpenSSL::X509::Certificate) | |||
There was a problem hiding this comment.
From what I understand W3C recommendations states it should return x5c which stands for a 1 element array, not the actual element.
https://www.w3.org/TR/2018/CR-webauthn-20180807/#fido-u2f-attestation
| @@ -39,6 +39,8 @@ | |||
| ) | |||
|
|
|||
| expect(response.valid?(original_challenge, original_origin)).to eq(true) | |||
| expect(response.attestation_type).to eq(WebAuthn::AttestationStatement::Base::ATTESTATION_TYPE_BASIC_OR_ATTCA) | |||
There was a problem hiding this comment.
I only see "Basic" as the returned type in the "Verification procedure" in https://www.w3.org/TR/2018/CR-webauthn-20180807/#fido-u2f-attestation. I may be missing something.
| @@ -55,6 +57,8 @@ | |||
| ) | |||
|
|
|||
| expect(response.valid?(original_challenge, original_origin)).to eq(true) | |||
| expect(response.attestation_type).to eq(WebAuthn::AttestationStatement::Base::ATTESTATION_TYPE_SELF) | |||
There was a problem hiding this comment.
I would try to avoid using internal constants for test expectations.
We want the test to fail if we accidentally change the constant value, right?
|
@bdewater Want me to help out with the changes? |
|
...or help you in any other way? |
bdewater
force-pushed
the
attestation-type-trust-path
branch
from
f4a834c
to
010884d
Compare
|
Hi @grzuy I've addressed your comments, please let me know if you'd like to see other changes. Just have been busy the last few weeks, but I intend to get this over the finish line 😄 |
| @@ -6,7 +6,7 @@ module WebAuthn | |||
| module AttestationStatement | |||
| class None < Base | |||
| def valid?(*_args) | |||
| true | |||
| [WebAuthn::AttestationStatement::Types::NONE, nil] | |||
There was a problem hiding this comment.
I think with this new Types namespace I find reading this a bit confusing.
I am worried that reading "attestation statement type none" would confuse users into thinking this is talking about "attestation statement formats", which is a different thing.
I'm more inclined to use WebAuthn::AttestationFormat::ATTESTATION_TYPE_NONE or just even WebAuthn::ATTESTATION_TYPE_NONE. Either works for me.
There was a problem hiding this comment.
I like WebAuthn::AttestationFormat::ATTESTATION_TYPE_NONE, I think it captures well how Attestation Statement Formats part of the spec describes the relation between the two. Thanks, I've updated the PR!
bdewater
force-pushed
the
attestation-type-trust-path
branch
from
010884d
to
ea2a138
Compare
The WebAuthn spec notes the following: > Attestation statements conveying attestations of type AttCA use the same data structure as attestation statements conveying attestations of type Basic, so the two attestation types are, in general, distinguishable only with externally provided knowledge regarding the contents of the attestation certificates conveyed in the attestation Retrieving and managing this external data from the FIDO Metadata Service is (for now) up to the implementing application if so desired.
bdewater
force-pushed
the
attestation-type-trust-path
branch
from
ea2a138
to
581d987
Compare
|
Awesome, thank you very much! Added additional commit to fix tests. |
The WebAuthn spec notes the following:
Retrieving and managing the this external data (TOC and statements) from the FIDO Metadata Service is up to the implementing application (for now? #66) if so desired.
Closes #80