Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Supports enumerating Ed25519 and P521 credentials within FIDO #186

Merged
merged 31 commits into from
Feb 5, 2025

Conversation

DennisDyallo
Copy link
Collaborator

@DennisDyallo DennisDyallo commented Jan 11, 2025

This pull request includes adds support for Ed25519 FIDO credentials and updates to the EcdsaVerify class within the Yubico.YubiKey.Cryptography namespace to enhance its flexibility and maintainability. The changes primarily focus on extending the supported elliptic curves, refactoring the code for better readability, and improving error handling.

Will also fix this: virot/powershellYK#86

Key updates include:

Support for Additional Curves

  • CoseEdDsaPublicKey adds support for enumerating Ed25519 credentials
  • Extended support to include P-521 curve, in addition to existing P-256 and P-384. Updated relevant methods and comments to reflect this change. [1] [2]

Refactoring and Code Simplification

  • Introduced GetOidByAlgorithm and GetOidByLength methods to streamline the process of obtaining OIDs based on algorithm identifiers and encoded point lengths, respectively. [1] [2]
  • Removed hardcoded constants for key sizes and OIDs, replacing them with dynamic calculations and lookups from KeyDefinitions. [1] [2]

Error Handling Improvements

  • Enhanced error handling by adding more specific exception messages and conditions, ensuring that unsupported algorithms are correctly identified and reported. [1] [2]

Documentation Updates

  • Updated method documentation to accurately describe the extended curve support and the changes in signature verification processes. [1] [2] [3]

Code Cleanup

  • Removed redundant comments and unnecessary private constants to clean up the codebase and improve readability. [1] [2] [3] [4]

These changes collectively enhance the functionality and maintainability of the EcdsaVerify class, ensuring it can handle a wider range of elliptic curve signatures with improved clarity and robustness.

@DennisDyallo DennisDyallo marked this pull request as draft January 11, 2025 14:58
@DennisDyallo DennisDyallo added the enhancement New feature or request label Jan 11, 2025
Copy link

github-actions bot commented Jan 11, 2025

Test Results: Windows

    2 files      2 suites   5s ⏱️
3 768 tests 3 768 ✅ 0 💤 0 ❌
3 770 runs  3 770 ✅ 0 💤 0 ❌

Results for commit 9d37340.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 11, 2025

Test Results: Ubuntu

    2 files      2 suites   9s ⏱️
3 760 tests 3 760 ✅ 0 💤 0 ❌
3 762 runs  3 762 ✅ 0 💤 0 ❌

Results for commit 9d37340.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 11, 2025

Test Results: MacOS

    2 files      2 suites   5s ⏱️
3 760 tests 3 760 ✅ 0 💤 0 ❌
3 762 runs  3 762 ✅ 0 💤 0 ❌

Results for commit 9d37340.

♻️ This comment has been updated with latest results.

@DennisDyallo DennisDyallo added this to the April release milestone Jan 27, 2025
@DennisDyallo DennisDyallo changed the title Work in progress to include Ed25519, P384 and P521 into FIDO Feature: Allow to enumerate Ed25519 and P521 within FIDO Jan 28, 2025
@DennisDyallo DennisDyallo changed the title Feature: Allow to enumerate Ed25519 and P521 within FIDO Feature: Supports enumerating Ed25519 and P521 credentials within FIDO Jan 28, 2025
Copy link
Collaborator Author

@DennisDyallo DennisDyallo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor things, also add tests on CoseEdDsaPublicKey

Copy link
Collaborator Author

@DennisDyallo DennisDyallo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just have one question regarding the Cbor output. See above comment

@DennisDyallo DennisDyallo marked this pull request as ready for review January 29, 2025 14:13
…nstead of hardcoded constant; improve clarity in CoseEdDsaPublicKeyTests
Copy link

github-actions bot commented Feb 5, 2025

Code Coverage

Package Line Rate Branch Rate Complexity Health
Yubico.Core 44% 32% 4317
Yubico.YubiKey 50% 46% 19773
Summary 49% (33631 / 69240) 44% (8308 / 19050) 24090

Minimum allowed line rate is 40%

@DennisDyallo DennisDyallo merged commit 2854939 into develop Feb 5, 2025
12 checks passed
@DennisDyallo DennisDyallo deleted the feature/new-fido-curves branch February 5, 2025 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants