feat: expose attestation type and trust path after validation #98

bdewater · 2018-10-21T15:56:15Z

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 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

grzuy

Great work! 👏
Thank you.

Left a couple of comments.

grzuy · 2018-10-26T01:57:03Z

spec/webauthn/attestation_statement/packed_spec.rb

-      )
-    )
+    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)


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.

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..."

This has been clarified in the latest editors draft

Gotcha 👍

grzuy · 2018-10-26T02:00:18Z

lib/webauthn/attestation_statement/packed.rb

+        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]


We don't support ECDAA for now.
Can we please remove it from here, it will never execute this code.

grzuy · 2018-10-26T02:07:43Z

lib/webauthn/attestation_statement/base.rb

+      ATTESTATION_TYPE_SELF = "self"
+      ATTESTATION_TYPE_ATTCA = "attca"
+      ATTESTATION_TYPE_ECDAA = "ecdaa"
+      ATTESTATION_TYPE_BASIC_OR_ATTCA = "basic_or_attca"


W3C Recommendation seem to use first letter uppercase, e.g. None instead of none or AttCA instead of attca.

Also, can we move these out of Base namespace?

grzuy · 2018-10-26T02:11:41Z

spec/webauthn/authenticator_attestation_response_spec.rb

@@ -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)


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

grzuy · 2018-10-26T02:18:47Z

spec/webauthn/authenticator_attestation_response_spec.rb

@@ -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)


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.

grzuy · 2018-10-26T02:26:25Z

spec/webauthn/authenticator_attestation_response_spec.rb

@@ -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)


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?

grzuy · 2018-11-01T23:30:31Z

@bdewater Want me to help out with the changes?

grzuy · 2018-11-01T23:40:35Z

...or help you in any other way?

bdewater · 2018-11-04T14:55:00Z

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 😄

grzuy

Thank you for your updates!

grzuy · 2018-11-06T03:10:37Z

lib/webauthn/attestation_statement/none.rb

@@ -6,7 +6,7 @@ module WebAuthn
  module AttestationStatement
    class None < Base
      def valid?(*_args)
-        true
+        [WebAuthn::AttestationStatement::Types::NONE, nil]


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.

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!

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.

…tement/types

grzuy · 2018-11-08T04:12:10Z

Awesome, thank you very much!

Added additional commit to fix tests.
Merging now.

bdewater force-pushed the attestation-type-trust-path branch from 702b48b to f4a834c Compare October 21, 2018 17:03

grzuy reviewed Oct 26, 2018

View reviewed changes

bdewater force-pushed the attestation-type-trust-path branch from f4a834c to 010884d Compare November 4, 2018 14:53

grzuy reviewed Nov 6, 2018

View reviewed changes

bdewater force-pushed the attestation-type-trust-path branch from 010884d to ea2a138 Compare November 7, 2018 14:33

bdewater force-pushed the attestation-type-trust-path branch from ea2a138 to 581d987 Compare November 7, 2018 15:59

fix: fix LoadError: cannot load such file -- webauthn/attestation_sta…

ad82954

…tement/types

grzuy merged commit 34a13fb into cedarcode:master Nov 8, 2018

bdewater deleted the attestation-type-trust-path branch November 8, 2018 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose attestation type and trust path after validation #98

feat: expose attestation type and trust path after validation #98

bdewater commented Oct 21, 2018 •

edited

Loading

grzuy left a comment

grzuy Oct 26, 2018

bdewater Oct 29, 2018

grzuy Oct 30, 2018

grzuy Oct 26, 2018

grzuy Oct 26, 2018

grzuy Oct 26, 2018

grzuy Oct 26, 2018

grzuy Oct 26, 2018

grzuy Oct 26, 2018

grzuy commented Nov 1, 2018

grzuy commented Nov 1, 2018

bdewater commented Nov 4, 2018

grzuy left a comment

grzuy Nov 6, 2018 •

edited

Loading

bdewater Nov 7, 2018

grzuy commented Nov 8, 2018

feat: expose attestation type and trust path after validation #98

feat: expose attestation type and trust path after validation #98

Conversation

bdewater commented Oct 21, 2018 • edited Loading

grzuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzuy commented Nov 1, 2018

grzuy commented Nov 1, 2018

bdewater commented Nov 4, 2018

grzuy left a comment

Choose a reason for hiding this comment

grzuy Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzuy commented Nov 8, 2018

bdewater commented Oct 21, 2018 •

edited

Loading

grzuy Nov 6, 2018 •

edited

Loading