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

fido2 list-credentials: Handle missing RP name #359

Merged
merged 2 commits into from
May 10, 2023
Merged

Conversation

robin-nitrokey
Copy link
Member

@robin-nitrokey robin-nitrokey commented Mar 28, 2023

The RP name is optional when enumerating RPs using the credential management command. Only the id is required. This patch changes the fido2 list-credentials subcommand to show the id instead of the name if the name is not set.

Fixes: #352

Checklist

  • tested with Python3.9
  • run make check or make fix for the formatting check
  • signed commits
  • updated documentation (e.g. parameter description, inline doc, docs.nitrokey)
  • added labels

Test Environment and Execution

  • OS: Debian 11
  • device's model: NK3AM
  • device's firmware version: v1.3.0-rc.1

The RP name is optional when enumerating RPs using the credential
management command.  Only the id is required.  This patch changes the
fido2 list-credentials subcommand to show the id instead of the name if
the name is not set.

Fixes: #352
Comment on lines 239 to 242
if "name" in reliable_party:
local_print(f"{reliable_party['name']}: ")
else:
local_print(f"{reliable_party['id']}: ")
Copy link
Member

Choose a reason for hiding this comment

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

Could be better with:

Suggested change
if "name" in reliable_party:
local_print(f"{reliable_party['name']}: ")
else:
local_print(f"{reliable_party['id']}: ")
name = reliable_party.get('name', reliable_party['id'])
local_print(f"{name}: ")

Copy link
Member

@szszszsz szszszsz left a comment

Choose a reason for hiding this comment

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

Shows error during tests on my data. Please hold on with merging.

@szszszsz
Copy link
Member

szszszsz commented May 10, 2023

Tested on NK3C v1.3.1 and NK FIDO2 2.4.0.

~/w/pynitrokey (credential-name|✔) $ ./venv/bin/nitropy fido2 list-credentials
Command line tool to interact with Nitrokey devices 0.4.35
Please provide pin:
There are 4 registered credentials
-----------------------------------
ssh:foobar2:
- id: a30058689de72bef012f90949cd32d1bca059a788ecb264ed758b424a65844a86878b4f55fd33363095f8f1c1b2736abcb87
fa3a6b1d65d7986816bed1877e45c6e7deb70379ca30ce82401f369b82af701a0e087f91c3b52f167c2d96bf7a5a76f6476efff750
eb651ec50f014ce2a3db2fa6a3126d2cb6f97302501ffda749b4f0bf45685b960a1717e306
  user: openssh
-----------------------------------
Microsoft:
- id: a30058744fef2900c2a09dad7f376a07cce31002b8a6ecd543e7dae8423809f1c25800b709237a9c775974e254112c7a0674
9127457347f0b7abdedca1a25879d07e75c7ccb86513bc82b092f36415c62a1236190968a019661993bc2d84b488031783c7c22778
cddf265e856e8bd22418bf00601bdce8c8014cce7d1841e5a0b6d5bc3b16f702504ac9df088e0863c769a6a6b8b0aecbf6
  user: Sz Test ([email protected])
-----------------------------------
webauthn.io:
- id: a30058496fb187449eb7558b0d4641d2d0679f96e628aead2c4cde052e56cb4b9b2ea281e9bbc1408ee180a4c92d555c1ec9
3d68c4fe6bdb8fbe085c58bfea3a3e2e522f11b486e24f560f4a62014c83ecb52f514ded86a0ac3b1602503a1cd73e596bcb75201c
de0893113cf7
  user: qqq
-----------------------------------
ssh:foobar:
- id: a3005867fe4ebd03c66dbb602d1a227b96d858f82a76b6bef4f4c97e29fec15698aa4b0e7b2d67d55bb46b824fc99d727791
a3ebbf1c4997580a25f8a332da11d6c88a6ecc5dbd205efc0b70af57d3e3eb44658281a922cc5c402348a238437c3d0c3575b9397a
20768811014cafa215a1294fa8ffc748adc202509f2f516c2bc0f0d3b03f89998d01e0b4
  user: openssh
-----------------------------------
There is an estimated amount of 96 credential slots left
~/w/pynitrokey (credential-name|✔) $

@szszszsz szszszsz merged commit 32a9850 into master May 10, 2023
@szszszsz szszszsz deleted the credential-name branch May 10, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

list-credentials exception
2 participants