Skip to content
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

cred: support for enterprise attestation #824

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

cred: support for enterprise attestation #824

wants to merge 17 commits into from

Conversation

LDVG
Copy link
Contributor

@LDVG LDVG commented Oct 2, 2024

In short, the following user visible changes are applied:

  • fido_cred_set_entattest() is added for the client to be able to request enterprise attestation;
  • fido_cred_entattest() is added to query whether the authenticator performed enterprise attestation; and
  • fido2-cred -M -a is added to request enterprise attestation (dito examples/cred).

Internally,

  • fido_dev_make_cred_tx() learned how to encode the enterprise attestation parameter;
  • fido_dev_make_cred_rx() learned how to decode the response; and
  • likewise for the winhello.c equivalents.

Finally, the fuzzer and manual pages are updated accordingly.

Further resources:

LDVG added 11 commits October 2, 2024 10:57
Can be used to enable enterprise attestation.
Returns true if an enterprise attestation was returned for this
credential, false otherwise.
Use excl_cred's first byte as an input to fido_cred_set_entattest();
like what fido_cred_set_prot() is doing with user_id. This saves adding
additional members to the parameter struct.
To request enterprise attestation.
To request enterprise attestation.
@martelletto
Copy link
Contributor

we seem to be missing MAN_ALIAS bits for the new functions, otherwise LGTM. for my own education, what does fido_cred_entattest() signify? what is the application expected to do with that information?

@LDVG
Copy link
Contributor Author

LDVG commented Oct 7, 2024

we seem to be missing MAN_ALIAS bits for the new functions, otherwise LGTM

Thanks!

for my own education, what does fido_cred_entattest() signify? what is the application expected to do with that information?

Here's my understanding: It is for the to indicate whether the authenticator used EA. The client may, for example, replace the attestation with an empty ("none") attestation statement if the authenticator did not return an EA and instead downgraded to regular attestation, I believe Chrome does this(?). If this boolean was not present in the response, the client would need knowledge of how to parse and discern EA, i.e. what constitutes individually identifiable information.

@martelletto
Copy link
Contributor

understood; thank you. is there wire data we could use for regression tests?

@LDVG
Copy link
Contributor Author

LDVG commented Oct 7, 2024

understood; thank you. is there wire data we could use for regression tests?

We could definitely record some. I'll make sure to do so before marking this PR as ready. Thanks!

To be able to specify multiple source files to add_regress_test().
This makes use of mocked i/o to ensure that our parsers work correctly.
To do this, new wiredata was recorded and the contents of the response
were extracted so that we can re-use the same expected data throughout
these tests.
@LDVG LDVG marked this pull request as ready for review October 16, 2024 07:15
@LDVG
Copy link
Contributor Author

LDVG commented Oct 16, 2024

I've added regression tests where we can assert that we parse the response to fido_dev_make_cred() correctly, this should now mostly be good to go (cc @kongeo).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants