U2F facets support

[baloo]

This pull request implement support for U2F facets

see https://fidoalliance.org/specs/fido-u2f-v1.2-ps-20170411/fido-appid-and-facets-v1.2-ps-20170411.html

[baloo]

This pull request currently implements a test to show the problem I currently face implementing support for u2f facets.

I have a hard time coming a good API to solve the issue, any tips, ideas or advise is more than welcome!

Here is the output of the test:

    def authenticate_complete(self, state, credentials, credential_id,
                              client_data, auth_data, signature):
        """Verify the correctness of the assertion data received from
        the client.

        :param state: The state data returned by the corresponding
            `register_begin`.
        :param credentials: The list of previously registered credentials.
        :param credential_id: The credential id from the client response.
        :param client_data: The client data.
        :param auth_data: The authenticator data.
        :param signature: The signature provided by the client."""
        if client_data.get('type') != WEBAUTHN_TYPE.GET_ASSERTION:
            raise ValueError('Incorrect type in ClientData.')
        if not self._verify(client_data.get('origin')):
>           raise ValueError('Invalid origin in ClientData.')
E           ValueError: Invalid origin in ClientData.

[baloo]

diff --git a/fido2/rpid.py b/fido2/rpid.py
index d6fefcce15311..95bfc6f77d718 100644
--- a/fido2/rpid.py
+++ b/fido2/rpid.py
@@ -74,7 +74,7 @@ def verify_rp_id(rp_id, origin):
     return False

-def verify_app_id(app_id, origin):
+def verify_app_id(app_id, rp_id, origin):
     """Checks if a FIDO U2F App ID is usable for a given origin.

     :param app_id: The App ID to validate.
@@ -86,4 +86,4 @@ def verify_app_id(app_id, origin):
     url = urlparse(app_id)
     if url.scheme != 'https':
         return False
-    return verify_rp_id(url.hostname, origin)
+    return verify_rp_id(url.hostname, origin) or verify_rp_id(rp_id, origin)
diff --git a/fido2/server.py b/fido2/server.py
index d0990b9517e92..2345e474f7d84 100644
--- a/fido2/server.py
+++ b/fido2/server.py
@@ -310,7 +310,7 @@ class U2FFido2Server(Fido2Server):

     def __init__(self, app_id, rp, *args, **kwargs):
         super(U2FFido2Server, self).__init__(rp, *args, **kwargs)
-        kwargs['verify_origin'] = lambda o: verify_app_id(app_id, o)
+        kwargs['verify_origin'] = lambda o: verify_app_id(app_id, rp.ident, o)
         kwargs['attestation_types'] = [FidoU2FAttestation()]
         self._app_id = app_id
         self._app_id_server = Fido2Server(RelyingParty(app_id), *args, **kwargs)

That would fix my issue, but I'm not sure about it.

[dainnilsson]

The proper way to do the origin verification for a https://.../facets.json App ID would be to verify it in accordance to the content of that file. I suppose we could add support for parsing the JSON and handling that file format so you could provide it, but I'm not sure the complexity of that is worth doing. One problem I see in the current approach is that it isn't really possible to customize the verify_origin function to the inner _app_id_server instance. Allowing this should make it possible for you to supply a simple function tailored to how you want to validate origins, would that suffice for you? I'm thinking something like:

diff --git a/fido2/server.py b/fido2/server.py
index b8be6ef..e86e514 100644
--- a/fido2/server.py
+++ b/fido2/server.py
@@ -305,12 +305,17 @@ class U2FFido2Server(Fido2Server):
     See https://www.w3.org/TR/webauthn/#sctn-appid-extension for details.

     :param app_id: The appId which was used for U2F registration.
+    :param verify_u2f_origin: (optional) Alternative function to validate an
+        origin for U2F credentials..
     For other parameters, see Fido2Server.
     """

     def __init__(self, app_id, rp, *args, **kwargs):
         super(U2FFido2Server, self).__init__(rp, *args, **kwargs)
-        kwargs['verify_origin'] = lambda o: verify_app_id(app_id, o)
+        kwargs['verify_origin'] = kwargs.get(
+            'verify_u2f_origin',
+            lambda o: verify_app_id(app_id, o)
+        )
         kwargs['attestation_types'] = [FidoU2FAttestation()]
         self._app_id = app_id
         self._app_id_server = Fido2Server(RelyingParty(app_id), *args, **kwargs)

What do you think?

[baloo]

@dainnilsson Thanks for the reply! Sorry for the delay of this reply, I was out on holidays.

I didn't want to implement the full featured json parsing either, the complexity of it looked a bit too hazardous to me. I think the custom function would work. Do you want me to amend this PR to include your patch?

[dainnilsson]

Now it's my turn to be sorry for the delay! Yes, please add the patch to the RP and I'll merge it.

[baloo]

@dainnilsson I had to rewrite your patch slightly, but I think it works great!

I provided examples in the tests.

[baloo]

I've been trying all morning to get that error fixed on travis. It does not reproduce on my computer. I've tried a couple configuration of virtualenv, I've tried ubuntu 16.04 in docker, with the same codebase as travis (curl https://storage.googleapis.com/travis-ci-language-archives/python/binaries/ubuntu/16.04/x86_64/pypy2.7-6.0.tar.bz2 thingy) can't reproduce either.

@dainnilsson if you have any idea, I'm all ears :/

[baloo]

what's even more interesting is:

======================================================================

ERROR: test_u2f_facets (test.test_server.TestU2FFido2Server)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/build/Yubico/python-fido2/test/test_server.py", line 121, in test_u2f_facets

    client_data, authenticator_data, signature)

  File "/home/travis/build/Yubico/python-fido2/fido2/server.py", line 353, in authenticate_complete

    return self._app_id_server.authenticate_complete(*args, **kwargs)

  File "/home/travis/build/Yubico/python-fido2/fido2/server.py", line 280, in authenticate_complete

    raise ValueError('User Present flag not set.')

ValueError: User Present flag not set.

well:

$ git grep 'User Present'
fido2/ctap2.py:        """Return true if the User Present flag is set.
fido2/ctap2.py:        :return: True if User Present is set, False otherwise.

well, this does not look like my code... I don't understand.

[baloo]

meh, I ended up with an old version of master because git fetch did not act the way I expected it to do (fetching from my fork instead of origin). Can reproduce now.

[dainnilsson]

Looks good, thanks!

Please use the already existing fido2.utils.bytes2int instead of int_from_bytes in test/utils.py and we should be ready to merge.

[baloo]

sure, missed that one.

[baloo]

@dainnilsson any chance you could push a new version of the lib once this is merged? This would ease things up for me to deploy this. Thanks :)

[baloo]

Thank you so much!

[dainnilsson]

Thank you for the contribution! I'll try to get a release out pretty soon as well.

[dainnilsson]

Release (0.7.1) is out!