fix(auth): replace pyu2f reauth fallback with fido2#17391
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces the pyu2f dependency with fido2 for security key reauthentication, updating the challenge implementation, credentials documentation, setup dependencies, and associated tests. Feedback from the review highlights that fido2 version 2.0.0 does not exist on PyPI, so the dependency range in setup.py should target 1.x instead. Additionally, it is recommended to handle potential OSError exceptions when listing devices or communicating with the security key to prevent unexpected crashes and improve error handling.
| devices = list(CtapHidDevice.list_devices()) | ||
| if not devices: | ||
| sys.stderr.write("No security key found.\n") | ||
| return None |
There was a problem hiding this comment.
Avoid broad except Exception: blocks that silently return None. Instead, catch specific exceptions like OSError and log the exception using logger.warning to aid in debugging and prevent masking underlying issues.
| devices = list(CtapHidDevice.list_devices()) | |
| if not devices: | |
| sys.stderr.write("No security key found.\n") | |
| return None | |
| try: | |
| devices = list(CtapHidDevice.list_devices()) | |
| except OSError as caught_exc: | |
| logger.warning("Failed to list security keys: %s", caught_exc) | |
| return None | |
| if not devices: | |
| sys.stderr.write("No security key found.\n") | |
| return None |
References
- Avoid broad
except Exception:blocks that silently returnNone. Instead, log the exception (e.g., usinglogger.warning) to aid in debugging and prevent masking underlying issues.
There was a problem hiding this comment.
I added handling for OSError around device enumeration so OS or HID permission failures do not bubble out of the reauth flow.
Also added a test for this path.
| try: | ||
| signature = Ctap1(device).authenticate( | ||
| client_param, app_param, key_handle | ||
| ) | ||
| except ApduError as caught_exc: | ||
| if caught_exc.code == APDU.WRONG_DATA: | ||
| continue | ||
| if caught_exc.code == APDU.USE_NOT_SATISFIED: | ||
| sys.stderr.write( | ||
| "Timed out while waiting for security key touch.\n" | ||
| ) | ||
| return None | ||
| raise | ||
| except CtapError as caught_exc: | ||
| if caught_exc.code in ( | ||
| CtapError.ERR.TIMEOUT, | ||
| CtapError.ERR.ACTION_TIMEOUT, | ||
| CtapError.ERR.KEEPALIVE_CANCEL, | ||
| CtapError.ERR.OPERATION_DENIED, | ||
| ): | ||
| sys.stderr.write( | ||
| "Timed out while waiting for security key touch.\n" | ||
| ) | ||
| return None | ||
| raise |
There was a problem hiding this comment.
When communicating with a security key, Ctap1(device).authenticate(...) can raise an OSError (or platform-specific subclasses like PermissionError or FileNotFoundError) if the user lacks sufficient permissions (e.g., missing udev rules on Linux) or if the device is disconnected. Adding an except OSError block ensures these common communication issues are handled gracefully by writing to stderr and continuing to the next device, rather than crashing the application.
try:
signature = Ctap1(device).authenticate(
client_param, app_param, key_handle
)
except ApduError as caught_exc:
if caught_exc.code == APDU.WRONG_DATA:
continue
if caught_exc.code == APDU.USE_NOT_SATISFIED:
sys.stderr.write(
"Timed out while waiting for security key touch.\n"
)
return None
raise
except CtapError as caught_exc:
if caught_exc.code in (
CtapError.ERR.TIMEOUT,
CtapError.ERR.ACTION_TIMEOUT,
CtapError.ERR.KEEPALIVE_CANCEL,
CtapError.ERR.OPERATION_DENIED,
):
sys.stderr.write(
"Timed out while waiting for security key touch.\n"
)
return None
raise
except OSError as caught_exc:
sys.stderr.write(
"Failed to communicate with security key: {}.\n".format(
caught_exc
)
)
continueThere was a problem hiding this comment.
Handled this too. If one security key raises OSError while authenticating, the fallback now writes the error and tries the next detected device.
Added coverage for the bad-device then good-device case.
| return self._obtain_challenge_input_webauthn(metadata, webauthn_handler) | ||
| except Exception: | ||
| # Attempt pyu2f if exception in webauthn flow | ||
| # Attempt fido2 if exception in webauthn flow. |
There was a problem hiding this comment.
Thanks for proposing this! Migrating away from pyu2f to fido2 is absolutely the right direction long-term, especially since pyu2f is officially deprecated. I'm concerned about the risk of breaking users. How do you feel about keeping both pyu2f and fido2, with a plan to remove pyu2f at a later date?
@_helpers.copy_docstring(ReauthChallenge)
def obtain_challenge_input(self, metadata):
# 1. Try the primary Webauthn Handler first
try:
factory = webauthn_handler_factory.WebauthnHandlerFactory()
webauthn_handler = factory.get_handler()
if webauthn_handler is not None:
sys.stderr.write("Please insert and touch your security key\n")
return self._obtain_challenge_input_webauthn(metadata, webauthn_handler)
except Exception:
pass
# 2. Try fido2 (Modern fallback)
try:
import fido2
return self._obtain_challenge_input_fido2(metadata)
except ImportError:
pass
# 3. Try pyu2f (Legacy fallback with deprecation warning)
try:
import pyu2f
import warnings
warnings.warn(
"Support for pyu2f is deprecated and will be removed in a future release. "
"Please switch to fido2 by installing `fido2` or `google-auth[reauth]`.",
category=DeprecationWarning,
stacklevel=2
)
return self._obtain_challenge_input_pyu2f(metadata)
except ImportError:
# If both libraries are completely missing, guide them to install the modern one
raise exceptions.ReauthFailError(
"fido2 dependency is required to use the Security Key reauth feature. "
"It can be installed via `pip install fido2`."
)
This is just an example. We can use the _get_fido2_classes function that you have
|
|
||
| return self._obtain_challenge_input_fido2(metadata) | ||
|
|
||
| def _get_fido2_classes(self): |
There was a problem hiding this comment.
Please add type hints and a doc string to this function. Also applies to _obtain_challenge_input_fido2
Fix details :
Replaces the deprecated
pyu2fsecurity-key fallback in google-auth reauth withfido2, while keeping the WebAuthn handler path preferred when available.The fallback keeps the existing U2F challenge response shape and covers the same main outcomes: valid assertion, ineligible key, no key, timeout, and missing optional dependency.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #17381