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

oem-factory-reset: fix keygen prompt order (fixes Yubikey. Breaks Nitrokey/Librem Keys) #1063

Closed
wants to merge 1 commit into from

Conversation

icequbes1
Copy link
Contributor

@icequbes1 icequbes1 commented Nov 23, 2021

Edit: Yubikey related. Does not affect Librem key/Nitrokey and seem to be linked to OpenGPG implemented standards being different between Yubikey and Nitrokey/Librem Key.


  • gpg --version: gpg (GnuPG) 2.2.21
  • An "okay" was requested after Comment
  • Admin PIN was requested after "okay"
  • Prompts observed:

Make off-card backup of encryption key? (Y/n)
PIN:
Key is valid for? (0)
Is this correct? (y/N)
Real name:
Email address:
Comment:
Change (N)ame, (C)omment, (E)mail or (O)kay/(Q)uit?
Admin PIN:

@tlaurion
Copy link
Collaborator

tlaurion commented Nov 26, 2021

Needs physical testing. Willing to pool that into next that PR prior of merge testing. Bit more difficult since CircleCI builds are foobar for the moment.

Thanks for this!

@tlaurion
Copy link
Collaborator

tlaurion commented Nov 26, 2021

@icequbes1 icequbes1 marked this pull request as draft November 26, 2021 22:29
@icequbes1
Copy link
Contributor Author

Marking as draft because the prompts seemed to have changed after flashing a new build, and I don't know why. Please don't waste time testing until I've figured this out.

I've observed 4 variations of the prompts thus far including the one already on master. Some signs are pointing to there being two gpg-agents in the background with different --homedirs.

More investigation is needed.

@tlaurion
Copy link
Collaborator

tlaurion commented Nov 26, 2021

Normally, key-init is the most interesting source of information here.

There is one homedir used to validate ISOs, then there is the user directory used for everything else.

As per key-init, the user's public keychain added to valid keychain to validate ISOs if detached signed. So basically, user's public key is added to distro's keys to validate user's detached signed by design.

Will willingly troubleshoot this issue in the goal of having heads generate keys and copy them over the proper way to USB dongle in later step.

From normal state of heads; there is no user keychain. Heads detects that absence of user public key and prompts for USB security dongle's key generation.

This is just output from brain content. Testing this would give more output.

But you have 4 different use cases @icequbes1 ?!

@tlaurion
Copy link
Collaborator

So basically one distro homedir and one user's homedir is normal.

- gpg --version: gpg (GnuPG) 2.2.21
- Admin PIN was requested after Comment
- Confirmation of key expiration is not prompted since command-fd is
  supplied
- Prompts observed (since script supplies --command-fd):
  - cardedit.prompt      [answer: admin]
  - cardedit.prompt      [answer: generate]
  - cardedit.genkeys.backup_enc [answer: n]
  - passphrase.enter     [answer: $USER_PIN_DEF]
  - keygen.valid         [answer: 0]
  - keygen.name          [answer: $GPG_USER_NAME]
  - keygen.email         [answer: $GPG_USER_MAIL]
  - keygen.comment       [answer: $GPG_USER_COMMENT]
  - passphrase.enter     [answer: $ADMIN_PIN_DEF]
- In comparison, prompts when user does generation manually (no
  command-fd):
  - Make off-card backup of encryption key? (Y/n)
  - PIN:
  - Key is valid for? (0)
  - Is this correct? (y/N)
  - Real name:
  - Email address:
  - Comment:
  - Change (N)ame, (C)omment, (E)mail or (O)kay/(Q)uit?
  - Admin PIN:
@icequbes1 icequbes1 marked this pull request as ready for review November 27, 2021 05:02
@icequbes1
Copy link
Contributor Author

@tlaurion I've resolved the discrepancy; the --command-fd parameter as used by oem-factory-reset changes the prompts as compared to a generic gpg --edit-card invocation (when performing the manual generation step). I referenced the gnupg code (g10/cpr.c - cpr_enabled()) to determine this.

So all in all, the diff as compared to what's on master is moving the Admin PIN to the bottom, and removing the "y/N" confirmation for a key expiration value of 0.

@MrChromebox
Copy link
Contributor

will test this week, I hadn't noticed it was currently broken. Was it a gnupg version bump that changed things?

@tlaurion
Copy link
Collaborator

tlaurion commented Nov 27, 2021

@MrChromebox the gpg2 last module change was 1y ago, so no. Edit: I tested this before and it was working; it got merged after upgrading gpg2.

@MrChromebox
Copy link
Contributor

I cherry-picked this change and when performing an oem-factory-reset, it fails with the error "GPG Key automatic keygen failed" so this change isn't agreeing with my L14 at least

@icequbes1
Copy link
Contributor Author

icequbes1 commented Nov 30, 2021 via email

@MrChromebox
Copy link
Contributor

@icequbes1 has what ever worked? the oem-factory-reset works perfectly for me in Pureboot, and there aren't any deviations in it which would cause it to not work with upstream Heads AFAICT. What board and key are you using?

@icequbes1
Copy link
Contributor Author

icequbes1 commented Nov 30, 2021 via email

@icequbes1
Copy link
Contributor Author

@tlaurion I suggest closing this PR. I'll do more debugging, maybe flashing Pureboot to my T430. I just don't have any sunny day cases to work with since I'm new to heads and key generation has never worked on my T430. The issue is more complex than this PR attempts to address, so I'll come back to a new PR once a more thorough analysis is conducted.

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 1, 2021

@icequbes1: New builds roms are coming under #1015 for testing. Including the fixes initializing the display properly for t430 board owners(dGPU or not) which might be linked to your issue.

Can you launch the oem-factory-reset script from Heads recovery shell and give the output that is on screen when it fails for you?

@icequbes1
Copy link
Contributor Author

I've just flashed the latest build of #1015 from commit 8f9ccae. I get the same response on screen as @MrChromebox "GPG Key automatic keygen failed", the same error I got the first time I flashed heads a week ago and started to investigate.

There is no output on screen as to the reason for the error.

Therefore I modified oem-factory-reset to redirect standard error to a file during the keygen invocation of gpg. It is currently being thrown to /dev/null.

Currently:

gpg --comand-fd=0 --status-fd=2 --pinentry-mode=loopback --card-edit \
   > /tmp/gpg_card_edit_output 2>/dev/null

Temporary modification:

gpg --comand-fd=0 --status-fd=2 --pinentry-mode=loopback --card-edit \
   > /tmp/gpg_card_edit_output 2>/tmp/gpg_card_edit_error

I've attached the contents of gpg_card_edit_error, which makes it apparent that the answers to prompts that gpg is expecting is invalid:

[GNUPG:] GET_LINE cardedit.prompt
[GNUPG:] GOT_IT
Admin commands are allowed

[GNUPG:] GET_LINE cardedit.prompt
[GNUPG:] GOT_IT
[GNUPG:] GET_LINE cardedit.genkeys.backup_enc
[GNUPG:] GOT_IT

Please note that the factory settings of the PINs are
   PIN = '123456'     Admin PIN = '12345678'
You should change them using the command --change-pin

[GNUPG:] INQUIRE_MAXLEN 100
[GNUPG:] GET_HIDDEN passphrase.enter
[GNUPG:] GOT_IT
[GNUPG:] SC_OP_FAILURE 2
gpg: error checking the PIN: Bad PIN
[GNUPG:] SC_OP_FAILURE 2

[GNUPG:] GET_LINE cardedit.prompt
[GNUPG:] GOT_IT

Invalid command  (try "help")

[GNUPG:] GET_LINE cardedit.prompt
[GNUPG:] GOT_IT

Invalid command  (try "help")

This PR fixed this because Admin PIN was being given to gpg first when the User PIN was expected. All of the prompts that are expected by the gpg (which this PR addressed) were detailed in the PR commit 1:

- Prompts observed (since script supplies --command-fd):
  - cardedit.prompt      [answer: admin]
  - cardedit.prompt      [answer: generate]
  - cardedit.genkeys.backup_enc [answer: n]
  - passphrase.enter     [answer: $USER_PIN_DEF]
  - keygen.valid         [answer: 0]
  - keygen.name          [answer: $GPG_USER_NAME]
  - keygen.email         [answer: $GPG_USER_MAIL]
  - keygen.comment       [answer: $GPG_USER_COMMENT]
  - passphrase.enter     [answer: $ADMIN_PIN_DEF]

Assuming oem-factory-reset works as-is for others indicates the prompting appears to be different in certain situations.

gpg_card_edit_error.txt

@icequbes1
Copy link
Contributor Author

Linking this issue potentially to #764 and #860. In those, gpg2 was upgraded and looked to cause oem-factory-reset issues, potentially tty-related.

But if oem-factory-reset worked after those fixes, I'm not sure if I'm running into a regression or "it's just me".

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 1, 2021

@icequbes1 just flashed https://2534-103208611-gh.circle-artifacts.com/0/build/x230-hotp-maximized/heads-x230-hotp-maximized-v5.0.1-57-g8f9ccae.rom from #1015

Defaults worked.

Customized worked as well.

Did not save to USB the public key generated, but this is out of scope of this test.

So if testing a ROM from #1015 gives you different results, then coming from the same code base, we will be able to try to diagnose where the problem could come from.

#1015 has been rebased on master as of today.

Some facts:

So here, it seems that

  • neither the gpg2 toolchain change is responsible of the behavior.
  • neither code change is responsible for behavior.

So.... question leads to what is wrong with tty/output redirection.
Which is why I'm asking to test #1015 t430-whatever and report on what ROM was used and screen captures, otherwise this path I've been before, which normally leads to checking everything else where my gut feelings are normally not wrong.

@icequbes1 : Let me know the behavior of #1015 dowloaded rom per #1015 (comment) instructions!

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 1, 2021

There is no output on screen as to the reason for the error.

@icequbes1 : You have output on your t430 from Heads on boot? Which model with without dGPU?

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 1, 2021

Linking this issue potentially to #764 and #860. In those, gpg2 was upgraded and looked to cause oem-factory-reset issues, potentially tty-related.

But if oem-factory-reset worked after those fixes, I'm not sure if I'm running into a regression or "it's just me".

@icequbes1 : "Just me" errors currently generalizes to a bunch, which are, t430 board owners from cross-linking reports.

Going back to basics:

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 1, 2021

@icequbes1
#764 specifies to run the command, as asked prior, directly on the command line, which worked there, while it was not working inline directly from the GUI prompts. Which was fixed by #777, that is, upgrading the gpg toolstack, and nullifying possible fault on TTY which is generalized to all platforms, for which t430 would be no different.

I need more input, otherwise it seems that t430 is doomed :P
Edit: joking, but I would expect a lot more of bug reports if the oem-factory-reset script was not working for anyone (t430 excluded) else.

Once again Nitrokey, time to shine. I do not own the board, and if I did, I would make you pay to support it and the countless hours of support for the t430 you should be supporting (search for t430 issues under Heads please.)!

:)
@nitrosimon @alex-nitrokey @jans23

@icequbes1
Copy link
Contributor Author

@tlaurion My testing from #1063 (comment) is from an up-to-date #1015 build from your tlaurion:maximized_boards-coreboot-4_13 branch as of today. We are working on the same code base. For good measure I flashed the image built by CircleCI (t430), but arrived at the same result of failing key generation.

I do not have issues with the internal display on my T430 that is iGPU-only. Boot output from heads is fine. That PR is unrelated (especially since key generation failed on coreboot 4.8).

I don't see how this is specifically related to T430. The prompts from gpg are simply different than what oem-factory-reset is providing.

Running gpg --card-edit --command-fd=0 --status-fd=2 --pinentry-mode=loopback outside of heads on a Fedora 33 VM with gpg 2.2.25 prompts for values that my PR addresses.

The same prompts my PR addresses is seen when I built for BOARD=qemu-coreboot and do a passthrough of my Yubikey to qemu.

In the cases where generation works, I am curious what the standard error output looks like for the 'admin/generate' sequence.

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 2, 2021

The x230-hotp-maximized board I have tested and gave insights from, previously is running the same script sthe L14 tested by @MrChromebox, since scripts and gpg tool chain haven't changed. The coreboot config is different. Will dig more....

I will modify script tomorrow to provide output as you gave. Butt that would prove positive feedback, as opposite to yours... which won't be helpful @icequbes1

@egonona?

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 2, 2021

@icequbes1 Some more testing on the x230-hotp-maximized board rom https://2534-103208611-gh.circle-artifacts.com/0/build/x230-hotp-maximized/heads-x230-hotp-maximized-v5.0.1-57-g8f9ccae.rom

signal-2021-12-02-101530

It works as expected. This is why i'm so confused. So if everything is the same but the board used, there needs to be something relevant to the board configuration that makes the I/O different.....

The relevant files, including modified oem-factory-reset script, and related output redirection into *output and *error 1 to 4 files are included in the archive below, with output of /etc/config showing that the board rom is coming from 8f9ccae where nothing in tree was unclean.

(Note, that as you did, I modified the oem-factory-reset script to have both output and error redirected, but here in different files which corresponds to individual 4 gpg calls in the script.)

oem-factory-reset_seperate-output_x230-hotp-maximized.tar.gz

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 2, 2021

Running gpg --card-edit --command-fd=0 --status-fd=2 --pinentry-mode=loopback outside of heads on a Fedora 33 VM with gpg 2.2.25 prompts for values that my PR addresses.

@icequbes1 the prompts are probably different there, since those are not the same gpg toolchain.
As pointed out here, Heads has gpg 2.2.21 LTS suite compiled in per last merged PR of version bump to support keygen of RSA 4096bits keypairs, which will have variation of prompts as compared to gpg 2.2.25 toolstack. Each time there is a bump in gpg versions under Heads, extensive testing needs to be done on gpg prompts. But for the same version, prompts should be consistent. Which is why this is puzzling here.

Here is the diff of my oem-factory-reset script against 8f9ccae version to produce pairs of output* and error* in previous report

user@heads-tests:~/heads$ diff initrd/bin/oem-factory-reset ~/QubesIncoming/Insurgo/oem-factory-reset 
72c72
<         > /tmp/gpg_card_edit_output 2>/dev/null
---
>         > /tmp/gpg_card_edit_output1 2>/tmp/gpg_card_edit_error1
95c95
<         > /tmp/gpg_card_edit_output 2>/dev/null
---
>         > /tmp/gpg_card_edit_output2 2>/tmp/gpg_card_edit_error2
113c113
<         > /tmp/gpg_card_edit_output 2>/dev/null 
---
>         > /tmp/gpg_card_edit_output3 2>/tmp/gpg_card_edit_error3
137c137
<         > /tmp/gpg_card_edit_output 2>/dev/null 
---
>         > /tmp/gpg_card_edit_output4 2>/tmp/gpg_card_edit_error4 

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 2, 2021

@icequbes1 :As for oem-factory-reset from this PR when compared to 8f9ccae version with seperated output:

user@heads-tests:~/heads$ diff initrd/bin/oem-factory-reset ~/QubesIncoming/Insurgo/icequbes1_pr/oem-factory-reset_icequbes1_different_output 
54c54
<     whiptail $BG_COLOR_ERROR --msgbox "${msg}\n\n" $HEIGHT $WIDTH $BG_COLOR_ERROR --title "Error"
---
>     whiptail $BG_COLOR_ERROR --msgbox "${msg}\n\n" $WIDTH $HEIGHT $BG_COLOR_ERROR --title "Error"
72c72
<         > /tmp/gpg_card_edit_output 2>/dev/null
---
>         > /tmp/gpg_card_edit_output1 2>/tmp/gpg_card_edit_error1
95c95
<         > /tmp/gpg_card_edit_output 2>/dev/null
---
>         > /tmp/gpg_card_edit_output2 2>/tmp/gpg_card_edit_error2
105d104
<         echo ${ADMIN_PIN_DEF}
108d106
<         echo y
111a110
>         echo ${ADMIN_PIN_DEF}
113c112
<         > /tmp/gpg_card_edit_output 2>/dev/null 
---
>         > /tmp/gpg_card_edit_output3 2>/tmp/gpg_card_edit_error3
137c136
<         > /tmp/gpg_card_edit_output 2>/dev/null 
---
>         > /tmp/gpg_card_edit_output4 2>/tmp/gpg_card_edit_error4 
519c518
<     $HEIGHT $WIDTH --title "OEM Factory Reset Complete"
---
>     $WIDTH $HEIGHT --title "OEM Factory Reset Complete"

Fails.
Attached is the modified script and outputs.
icequbes_pr.tar.gz


My point here is: x230-hotp-maximized, L14 and others (non reporting issues) are successfully using 8f9ccae's oem-factory-reset

So there seems to be a problem on the t430 for unknown reasons. Tagging t430 owners to at least replicate issue (I can't):
t430 (xx30): @Thrilleratplay @alexmaloteaux @nickfrostatx @lsafd @bwachter @shamen123 @eganonoa(no dGPU) @alex-nitrokey @nitrosimon @jans23

Any insight here? Can you, board owners, confirm you replicate currently reported issue?
Latest produced roms from CI (valid for 29 days) for latest commit 8f9ccae of #1015:

External flash of t430-hotp-maximized:
https://2535-103208611-gh.circle-artifacts.com/0/build/t430-hotp-maximized/heads-t430-hotp-maximized-v5.0.1-57-g8f9ccae-bottom.rom
https://2535-103208611-gh.circle-artifacts.com/0/build/t430-hotp-maximized/heads-t430-hotp-maximized-v5.0.1-57-g8f9ccae-top.rom

Internal flash from past t430-hotp-maximized flashed board:
https://2535-103208611-gh.circle-artifacts.com/0/build/t430-hotp-maximized/heads-t430-hotp-maximized-v5.0.1-57-g8f9ccae.rom

Tested working with x230-hotp-maximized....

@icequbes1
Copy link
Contributor Author

icequbes1 commented Dec 2, 2021 via email

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 2, 2021

@icequbes1 : rechecking!

Based on @tlaurion's gpg_card_edit_error3 file, I see two things: 1. @tlaurion's smartcard is OpenPGP version 3.4 1. My smartcard is OpenPGP version 2.0 1. The removal of the "y" answer that I fixed in my PR is a valid issue, as @tlaurion's output is providing "y" to the "name" prompt to which gpg responds name must be at least 5 characters. Fortunately the next prompt answer is the actual name so this error goes unnoticed and still works.

Extract of gpg_card_edit_error3's #1063 (comment) provided logs:

[GNUPG:] GET_LINE keygen.name
[GNUPG:] GOT_IT
Name must be at least 5 characters long
[GNUPG:] GET_LINE keygen.name
[GNUPG:] GOT_IT

So diff would be:

diff --git a/initrd/bin/oem-factory-reset b/initrd/bin/oem-factory-reset
index e39eef68..342cfe1e 100755
--- a/initrd/bin/oem-factory-reset
+++ b/initrd/bin/oem-factory-reset
@@ -105,7 +105,6 @@ gpg_key_reset()
         echo ${ADMIN_PIN_DEF}
         echo ${USER_PIN_DEF}
         echo 0
-        echo y
         echo ${GPG_USER_NAME} 
         echo ${GPG_USER_MAIL}
         echo ${GPG_USER_COMMENT}

@icequbes1 : nice catch!
Will test and report back local build.

  1. Therefore the only difference is when the Admin PIN is prompted. For @tlaurion it's at the very beginning. For me, it's at the very end. Based on this, I think this may be due to the smartcard itself (Yubikey vs ZeitControl) or the OpenPGP version of the smart cards. Unfortunately the Yubikey does not have upgradable firmware. If someone can do a back-to-back test of a Yubikey vs a smartcard known to work, this may be the answer.

@icequbes1 : I was not aware there was problems nor differences from OpenGPG version.
https://osresearch.net/Prerequisites#usb-security-dongles-aka-security-token-aka-smartcard was reported by users to work before, so confused here again since last changes are from last gpg version bump.

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 2, 2021

Will test and report back local build.

diff --git a/initrd/bin/oem-factory-reset b/initrd/bin/oem-factory-reset
index e39eef68..342cfe1e 100755
--- a/initrd/bin/oem-factory-reset
+++ b/initrd/bin/oem-factory-reset
@@ -105,7 +105,6 @@ gpg_key_reset()
         echo ${ADMIN_PIN_DEF}
         echo ${USER_PIN_DEF}
         echo 0
-        echo y
         echo ${GPG_USER_NAME} 
         echo ${GPG_USER_MAIL}
         echo ${GPG_USER_COMMENT}

Works.
@icequbes1 :I remember having had to bump my head around those problems while status-fd and command-fd were good enough at the time of writing Insurgo's reownership wizard. But maybe it is time to invest into making all OpenGPG keycards work.

That would mean digging a bit more into https://www.gnupg.org/documentation/manuals/gnupg/GPG-Esoteric-Options.html
options, and have a question/answer dialogue with gpg instead of using a fixed scheme to answer. Your report, here, shows that there might not be so many Yubikey users? It also shows that #692 should probably contain testers for other less used gpg cards owners....

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 2, 2021

Food for thought:

@icequbes1
Copy link
Contributor Author

icequbes1 commented Dec 3, 2021 via email

@daringer
Copy link
Collaborator

daringer commented Dec 3, 2021

From our side I can report that we cannot reproduce this issue with Nitrokeys. Works perfectly fine for current #1015 state

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 3, 2021

@icequbes1 I modified OP accordingly.

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 3, 2021

As a side note this means that USB security dongle's firmwqre changes in the future might cause problems even thought deployed GPG software suite deployed under Heads doesn't change. The more I think about it, the more #771 should in the long term replace oem-factory-reset current implementation.

Unfortunately for us:

user@Insurgo:~$ gpg --card-edit --batch
gpg: can't do this in batch mode

@tlaurion
Copy link
Collaborator

tlaurion commented Dec 3, 2021

@icequbes1

Well well well. The long promised gpg-card additional tool will be released under gnupg 2.3 (unreleased today)


EDIT: confused. 2.3.3 is released https://www.gnupg.org/download/
Their news are not up to date....

Though released:

  • Not available under debian-10 (gpg 2.2.12)
  • Not available under fedora-33 (gpg 2.2.25)
  • Not available under debian-11 (gpg 2.2.27

when available and more tested by communities (users on OSes...) it might be the time for an overhaul of oem-factory-reset using gpg-card frontend directly, where we won't have to deal with different implementations for different opengpg card specifications under gpg --card-edit anymore.

Release notes
Documentation is already available: https://gnupg.org/documentation/manuals/gnupg/gpg_002dcard.html

@tlaurion tlaurion changed the title oem-factory-reset: fix keygen prompt order oem-factory-reset: fix keygen prompt order for Yubikey Dec 3, 2021
@tlaurion tlaurion changed the title oem-factory-reset: fix keygen prompt order for Yubikey oem-factory-reset: fix keygen prompt order (fixes Yubikey. Breaks Nitrokey/Librem Keys) Dec 4, 2021
@icequbes1
Copy link
Contributor Author

I'm closing this PR so that it can be rejected in favor of opening a proper issue. The PR was premature.

I've done some additional debugging and determined why this issue is specific to the Yubikey. An issue will be more appropriate to explain and figure out a course of action.

Continuted at Issue #1076.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants