refactor: FIDO2 Credential Management Authentication, ApplicationSession #302
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fido2Session now inherits ApplicationSession in the same way as other session classes. PPUAT wont be set unless the key supports it Add possibility to self compute the authParam on EnumerateCredentialsBeginCommand, EnumerateRpsBeginCommand and GetCredentialMetadata command.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the FIDO2 Credential Management command classes to simplify authentication handling and improve code maintainability. The main purpose is to standardize how PIN/UV authentication parameters are handled across FIDO2 commands, replacing conditional token decryption with a more structured approach.
Key changes include:
- Removed
decryptAuthTokenparameters from credential management command constructors and simplified authentication token handling - Added new constructors that accept pre-computed authentication parameters and introduced static helper methods for generating authentication messages
- Updated
Fido2Sessionto inherit fromApplicationSessionand standardized logging to use the unifiedLoggerproperty
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
FidoIntegrationTestBase.cs |
Added using directive for Commands namespace and updated default device type to Any |
Fido2Tests.cs |
Enhanced integration tests with better setup, credential creation, and improved command testing patterns |
YubiKeyFeatureExtensions.cs |
Added support detection for FIDO2 CTAP2.2 standard |
YubiKeyFeature.cs |
Added FidoCtap22 enum value for CTAP2.2 feature detection |
Fido2Session.cs |
Refactored to inherit from ApplicationSession and updated constructor/disposal patterns |
Fido2Session.Pin.cs |
Updated all logging calls to use unified Logger property |
Fido2Session.MakeCredential.cs |
Updated logging to use unified Logger property |
Fido2Session.LargeBlobs.cs |
Updated logging to use unified Logger property |
Fido2Session.GetAssertion.cs |
Updated logging to use unified Logger property |
Fido2Session.CredMgmt.cs |
Major refactoring of credential management methods to use new authentication parameter handling |
Fido2Session.Config.cs |
Updated logging to use unified Logger property |
Fido2Session.BioEnrollment.cs |
Updated logging to use unified Logger property |
GetCredentialMetadataCommand.cs |
Added new constructor for pre-computed auth params and static helper method |
EnumerateRpsBeginResponse.cs |
Modernized null checks using is not null pattern |
EnumerateRpsBeginCommand.cs |
Added new constructor and static helper method for authentication message generation |
EnumerateCredentialsGetNextCommand.cs |
Added new constructor for pre-computed auth params |
EnumerateCredentialsBeginCommand.cs |
Added new constructor and static helper method for authentication message generation |
CredentialManagementCommand.cs |
Removed decryptAuthToken parameter and added new constructor for pre-computed auth params |
ApplicationSession.cs |
Enhanced XML documentation for Connection property |
Comments suppressed due to low confidence (1)
Yubico.YubiKey/src/Yubico/YubiKey/Fido2/Fido2Session.CredMgmt.cs:1
- There's trailing whitespace at the end of this line that should be removed.
// Copyright 2025 Yubico AB
Yubico.YubiKey/src/Yubico/YubiKey/Fido2/Commands/CredentialManagementCommand.cs
Show resolved
Hide resolved
Test Results: Windows 2 files 2 suites 9s ⏱️ Results for commit dcf1b86. ♻️ This comment has been updated with latest results. |
Test Results: Ubuntu 2 files 2 suites 14s ⏱️ Results for commit dcf1b86. ♻️ This comment has been updated with latest results. |
Test Results: MacOS 2 files 2 suites 11s ⏱️ Results for commit b7654ee. |
DennisDyallo
changed the title
Minimum allowed line rate is |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request refactors the FIDO2 Credential Management command classes to simplify authentication handling and improve flexibility when constructing commands. The main changes remove the
decryptAuthTokenparameter from several constructors, standardize how PIN/UV authentication parameters are handled, and add new constructors for cases where the authentication parameter is pre-computed. Additionally, logging is updated to use a consistent logger, and some documentation is improved.FIDO2 Credential Management Command Refactoring:
decryptAuthTokenparameter from constructors inCredentialManagementCommandand related command classes, simplifying how authentication tokens are handled. Now, authentication is always performed using the protocol's methods, and the SDK no longer conditionally decrypts tokens. [1] [2] [3] [4] [5]CredentialManagementCommand,EnumerateCredentialsBeginCommand,EnumerateCredentialsGetNextCommand,EnumerateRpsBeginCommand, andGetCredentialMetadataCommandto allow instantiating these commands with a pre-computed PIN/UV authentication parameter and protocol version. [1] [2] [3] [4] [5]GetAuthenticationMessageto help callers generate the correct message to use for PIN/UV authentication for each command. [1] [2] [3]Fido2Session and Logging Improvements:
Fido2Sessionpartial classes to use the unifiedLoggerproperty instead of the private_logfield, ensuring consistency in logging throughout the codebase. [1] [2] [3] [4] [5] [6] [7]GetCredentialMetadatamethod inFido2Session.CredMgmt.csto use the new authentication parameter handling: it now computes the authentication parameter before constructing the command and uses the new constructor, retrying with a new token if needed. [1] [2] [3]Documentation and Code Quality:
is not nullfor null checks and adding missing using directives. [1] [2] [3]