duo-labs / py_webauthn

Pythonic WebAuthn 🐍
https://duo-labs.github.io/py_webauthn
BSD 3-Clause "New" or "Revised" License
856 stars 171 forks source link

Use attestation 'none' as default ? #65

Closed andreasrs closed 3 years ago

andreasrs commented 4 years ago

The flask_demo results in the client asking the user for permission to view information about their keys. I gusss this is because internally; py_webauthn uses direct as the default attestation:

https://github.com/duo-labs/py_webauthn/blob/306e73dd4dafcdf2bb52d48c31a3774c4eff8740/webauthn/webauthn.py#L91

The spec seems to indicate that none is the default: https://www.w3.org/TR/webauthn/#dom-attestationconveyancepreference-none What is the reasoning behind choosing direct as the default in this library?

The demo itself does not seem to enforce/require attestation when direct is used. The registration is successful even when the client blocks the request to gather key data. Is this the correct behaviour ? If we want to use direct as default, maybe the demo should be updated to require this and then do something with this data?

nickmooney commented 4 years ago

Hey @andreasrs, thanks for the question! Currently we are requesting a direct attestation and verifying that attestation if it's present. If you click the "Block" button, the user agent (i.e. Chrome) will replace the attestation blob with a "none" attestation.

The behavior in the demo is such that "none" attestation is acceptable, which is why things succeed. See the following line: https://github.com/duo-labs/py_webauthn/blob/master/webauthn/webauthn.py#L573

To me this feels like good catchall behavior: you can validate that attestations succeed when they are provided, but it won't break the demo if you click Block. Do you have feelings about a default behavior you would prefer to see?

andreasrs commented 4 years ago

Disclaimer: I'm a newbie to webauthn, and I am trying to learn.

@nickmooney Thanks for your reply. I just find the behaviour in the demo a bit confusing, why specify "direct" when "none" is an end-result we find to be perfectly acceptible?


My own personal expectation was for the registration to fail when the block button is pressed. Because we requested a direct attestation which was not provided.

I would not have been puzzled if:

The behaviour I expected to happen occurs if following change is made:

none_attestation_permitted = False

if attestation is in fact direct. In the demo it is always set to True. So we always allow registration as you say.

This is more of a curiosity I struggle to find clear answers to. I don't really have the basis to say that the demo handles this wrong. It's just not what I would personally intuitively expect to happen.

To me this feels like good catchall behavior: you can validate that attestations succeed when they are provided, but it won't break the demo if you click Block. Do you have feelings about a default behavior you would prefer to see?

To me personally, "good catchall behaviour" is only good in a demo if it is what one would realistically do in a real-world scenario. And that part is not completely clear to me. What is the general opinion on how services choose to do this? The spec is pretty vague around that, at least the parts I have looked into. But I would assume that most real-life services that want direct attestation enforce this, as well perhaps look into making sure that the authenticator is of the correct type, etc? Assuming that is true, I think it would add a lot of value to the demo to both implement and add some documentation around that.

Thanks for your input and time :)

andreasrs commented 4 years ago

Example: https://github.com/andreasrs/py_webauthn/commit/e836f3c34a4513a66d7ce6301d456064dbf2c539

Allows attestation to be switched easily in demo, and registration with none is only allowed if attestation is set to this by the server.

nickmooney commented 4 years ago

That's a great point re real-world use cases.

In the real world, relying parties should (imo) avoid requesting attestation unless they are using it for enforcement. This is because attestation data is potentially identifying: it can be used to identify a particular credential as created by a particular batch of authenticators (though not by an an individual authenticator itself).

I'm assigning this to myself and will take a look at your modifications within the week.

andreasrs commented 4 years ago

@nickmooney did you get to take a look? Let me know if you want some modifications or a PR opened.

andreasrs commented 2 years ago

@MasterKale what was the final verdict on this one when closing it?

MasterKale commented 2 years ago

@MasterKale what was the final verdict on this one when closing it?

The latest version of the library defaults to "none" attestation:

https://github.com/duo-labs/py_webauthn/blob/master/webauthn/registration/generate_registration_options.py#L54