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

(RHEL-36276) Two followups that make unlocking with PIN locked yubikey actually work #289

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

msekletar
Copy link
Member

@msekletar msekletar commented Jul 22, 2024

No description provided.

aafeijoo-suse and others added 2 commits July 22, 2024 17:23
If a user only presses ENTER when the PIN is requested (without actually typing
the PIN), an assertion is reached and no other unlock method is requested.

```
sh-5.2# systemctl status systemd-cryptsetup@cr_root
× systemd-cryptsetup@cr_root.service - Cryptography Setup for cr_root
     Loaded: loaded (/etc/crypttab; generated)
    Drop-In: /etc/systemd/system/systemd-cryptsetup@.service.d
             └─pcr-signature.conf
     Active: failed (Result: core-dump) since Thu 2024-04-25 08:44:30 UTC; 10min ago
       Docs: man:crypttab(5)
             man:systemd-cryptsetup-generator(8)
             man:systemd-cryptsetup@.service(8)
    Process: 559 ExecStartPre=/usr/bin/pcr-signature.sh (code=exited, status=0/SUCCESS)
    Process: 604 ExecStart=/usr/bin/systemd-cryptsetup attach cr_root /dev/disk/by-uuid/a8cbd937-6975-4e61-9120-ce5c03138700 none x-initrd.attach,tpm2-device=auto (code=dumped, signal=ABRT)
   Main PID: 604 (code=dumped, signal=ABRT)
        CPU: 19ms

Apr 25 08:44:29 localhost systemd[1]: Starting Cryptography Setup for cr_root...
Apr 25 08:44:30 localhost systemd-cryptsetup[604]: Assertion '!pin || pin_size > 0' failed at src/cryptsetup/cryptsetup-tokens/cryptsetup-token-systemd-tpm2.c:60, function cryptsetup_token_open_pin(). Aborting.
Apr 25 08:44:30 localhost systemd[1]: systemd-cryptsetup@cr_root.service: Main process exited, code=dumped, status=6/ABRT
Apr 25 08:44:30 localhost systemd[1]: systemd-cryptsetup@cr_root.service: Failed with result 'core-dump'.
Apr 25 08:44:30 localhost systemd[1]: Failed to start Cryptography Setup for cr_root.
```

In this case, `cryptsetup_token_open_pin()` receives an empty (non-NULL) `pin`
with `pin_size` equals to 0.

```
🔐 Please enter LUKS2 token PIN:

Breakpoint 3, cryptsetup_token_open_pin (cd=0x5555555744c0, token=0, pin=0x5555555b3cc0 "", pin_size=0, ret_password=0x7fffffffd380,
    ret_password_len=0x7fffffffd378, usrptr=0x0) at ../src/cryptsetup/cryptsetup-tokens/cryptsetup-token-systemd-tpm2.c:42
42	                void *usrptr /* plugin defined parameter passed to crypt_activate_by_token*() API */) {
(gdb) continue
Assertion '!pin || pin_size > 0' failed at src/cryptsetup/cryptsetup-tokens/cryptsetup-token-systemd-tpm2.c:60, function cryptsetup_token_open_pin(). Aborting.
```

(cherry picked from commit 5cef6b5)

Related: RHEL-36276
When enrolling a new FIDO2 token with a client PIN, this tells the authenticator to require the PIN on all uses.

It also collects a PIN before attempting to create a credential.

Works around #31443 in most (not all) scenarios.

(cherry picked from commit 12cf745)

Related: RHEL-36276
@github-actions github-actions bot added pr/needs-ci Formerly needs-ci pr/needs-review Formerly needs-review labels Jul 22, 2024
Copy link

github-actions bot commented Jul 22, 2024

Commit validation

Tracker - RHEL-36276

The following commits meet all requirements

commit upstream
9615dfd - cryptsetup-tokens: fix pin asserts systemd/systemd@5cef6b5
2e92740 - cryptenroll: Use CTAP2.1 credProtect extension systemd/systemd@12cf745

Tracker validation

Success

🟢 Tracker RHEL-36276 has set desired product: rhel-9.3.0
🟢 Tracker RHEL-36276 has set desired component: systemd
🟢 Tracker RHEL-36276 has been approved


Pull Request validation

Success

🟢 CI - All checks have passed
🟢 Review - Reviewed by a member
🟢 Approval - Changes were approved


Auto Merge

Success

🟢 Pull Request is not marked as draft and it's not blocked by dont-merge label
🟢 Pull Request meet requirements, title has correct form
🟢 Pull Request meet requirements, mergeable is true
🟢 Pull Request meet requirements, mergeable_state is clean
🟢 Pull Request has correct target branch main
🟢 Pull Request was merged

@github-actions github-actions bot removed the pr/needs-ci Formerly needs-ci label Jul 22, 2024
@jamacku jamacku added this to the RHEL-9.5.0 milestone Jul 23, 2024
Copy link
Member

@jamacku jamacku left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot removed the pr/needs-review Formerly needs-review label Jul 23, 2024
@github-actions github-actions bot merged commit 42330aa into redhat-plumbers:main Jul 23, 2024
30 checks passed
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.

4 participants