pypi / warehouse

The Python Package Index
https://pypi.org
Apache License 2.0
3.6k stars 963 forks source link

TFA with Fido2 privacy intrution by requiring attestation #10087

Closed laphilipa closed 3 years ago

laphilipa commented 3 years ago

Describe the bug Can't register TFA with a FIDO key without device attestation, making this feature unusable for privacy conscious developers.

Expected behavior Respect users privacy, when registering FIDO keys by not requiring attestation. Most sites work without attestation pip should work without as well.

To Reproduce

Firefox fido2 pip

My Platform macOS 11.6 Firefox 92.0 (64-Bit) Yubikey blue USB-A with NFC

ewdurbin commented 3 years ago

While we do not store the attestation information, we do currently attempt to verify it in our provisioning.

We also currently allow self attestation, so I'm not sure precisely what requiring it at all gains us. It seems that attestation is most crucial in environments that have strict requirements on hardware/provenance or to prevent MITM attacks... But I'm not sure what the correct approach is. @woodruffw do you have an opinion here?

The change necessary (excluding tests) to avoid this popover/warning is as follows:

diff --git a/warehouse/utils/webauthn.py b/warehouse/utils/webauthn.py
index 8507d434..be5d12a1 100644
--- a/warehouse/utils/webauthn.py
+++ b/warehouse/utils/webauthn.py
@@ -87,6 +87,7 @@ def get_credential_options(user, *, challenge, rp_name, rp_id):
         user.username,
         user.name or user.username,
         None,
+        attestation=None,
         user_verification="discouraged",
     )

@@ -119,7 +120,7 @@ def verify_registration_response(response, challenge, *, rp_id, origin):
     # for the individual challenge.
     encoded_challenge = _webauthn_b64encode(challenge.encode()).decode()
     response = pywebauthn.WebAuthnRegistrationResponse(
-        rp_id, origin, response, encoded_challenge, self_attestation_permitted=True
+        rp_id, origin, response, encoded_challenge, self_attestation_permitted=True, none_attestation_permitted=True
     )
     try:
         return response.verify()
skorokithakis commented 3 years ago

Just to chime in, I agree that attestation isn't required here, since the provenance of the hardware authenticator itself shouldn't matter (ie PyPI shouldn't need to know that a certain key was made by a specific company). Here's some additional explanation.

woodruffw commented 3 years ago

Yep, we shouldn't need attestation at all (it is indeed for provenance, and we already have other mechanisms like WebAuthn's counter for MITM/skip attacks). The only "downside" is that we aren't able to associate signatures to physical/trustworhy devices, but that's not something we're doing anyways and is outside of PyPI's threat model.

IIRC, we have self-attestation enabled because WebKit/Safari's implementation of attestation with Touch ID originally only provided self-attested signatures. I'm not sure if that's changed, but WebKit does support some kind of anonymized direct attestation that looks similar (identical?) to the standardized anonymous attestation CA technique.

ewdurbin commented 3 years ago

Thanks @woodruffw and @skorokithakis! It makes sense that we move to a "No Attestation" model and keep tabs on the client library, if at some point they support additional attestations including Anonymization format (currently there are only these supported formats) we could consider those.

I'll open a PR shortly.

laphilipa commented 3 years ago

Thank you very much.