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

Fix Plymouth passphrase prompt in initramfs script #9202

Merged
merged 2 commits into from
Aug 27, 2019

Conversation

belperite
Copy link
Contributor

Motivation and Context

In issue #9193 I found that entering the ZFS encryption passphrase under Plymouth wasn't working because in the ZFS initrd script, Plymouth was calling zfs via --command, which wasn't passing through the filesystem argument to zfs load-key properly (it was passing through the single quotes around the filesystem name intended to handle spaces literally, which zfs load-key couldn't understand).

Description

I have updated the script to have Plymouth and systemd pipe the key to zfs load-key instead, which removes the obstacle of Plymouth passing through arguments which contain spaces via --command.

How Has This Been Tested?

I have tested this on my own desktop machine, updating the initramfs and rebooting upon each change to ensure the script continues working in a real environment.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

Entering the ZFS encryption passphrase under Plymouth wasn't working
because in the ZFS initrd script, Plymouth was calling zfs via
"--command", which wasn't passing through the filesystem argument to
zfs load-key properly (it was passing through the single quotes around
the filesystem name intended to handle spaces literally,
which zfs load-key couldn't understand).

I have changed the script to have Plymouth pipe the key to zfs instead.

Signed-off-by: Richard Allen <belperite@gmail.com>
@behlendorf
Copy link
Contributor

@ghfields @rlaager would you mind reviewing this.

@rlaager
Copy link
Member

rlaager commented Aug 22, 2019

This looks generally correct. Why is the loadkey_stdin function required, as opposed to just piping to ${ZFS} load-key "${ENCRYPTIONROOT}" directly?

@codecov
Copy link

codecov bot commented Aug 22, 2019

Codecov Report

Merging #9202 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9202      +/-   ##
==========================================
+ Coverage   79.22%   79.27%   +0.05%     
==========================================
  Files         400      400              
  Lines      122001   122001              
==========================================
+ Hits        96656    96718      +62     
+ Misses      25345    25283      -62
Flag Coverage Δ
#kernel 79.77% <ø> (+0.02%) ⬆️
#user 67.29% <ø> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9ebdfd...88f5469. Read the comment docs.

@ghfields
Copy link
Contributor

I agree with @rlaager . It would be preferred if we can get by without another function.

Additionally, the single quote was added during code review. It was also added to dracut in #8114 . Is it functioning properly over there?

Stepping back, could this have to do with which shell used? Not an argument for not fixing, but an explanation why only some people are experiencing it (see #8306 (comment)). I also have a coworker reporting that he believes this broke on his system after updates. He just removed the single quotes since he didn't have to worry about spaces.

@belperite
Copy link
Contributor Author

@rlaager The function was just to try and keep in with the idea of the original DECRYPT_CMD - having the command defined in one place whilst being used in multiple places. Am happy to change to direct piping though if that's what is preferred?

@ghfields The shell I use is the default one in Buster at installation time. I didn't have time to test Dracut sadly - I just needed my desktop up and running ASAP for work. However, see below:

contrib/dracut/90zfs/mount-zfs.sh.in:

                        ask_for_password \
                                --tries 5 \
                                --prompt "Encrypted ZFS password for ${ENCRYPTIONROOT}: " \
                                --cmd "zfs load-key '${ENCRYPTIONROOT}'"

Unfortunately, it looks like ask_for_password in Dracut (dracut/modules.d/90crypt/crypt-lib.sh) is a wrapper that calls Plymouth like so:

        if [ -x /bin/plymouth ] && /bin/plymouth --ping; then
            /bin/plymouth ask-for-password \
                --prompt "$ply_prompt" --number-of-tries=$ply_tries \
                --command="$ply_cmd"

If broken, fixing this for Dracut may have wider implications?

As per community suggestion, have changed zfs load-key to direct piping.

Signed-off-by: Richard Allen <belperite@gmail.com>
@belperite
Copy link
Contributor Author

@rlaager @ghfields have changed to direct pipe for zfs load-key in initramfs as per your suggestion. Dracut may need a different approach.

Copy link
Member

@rlaager rlaager left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ghfields ghfields left a comment

Choose a reason for hiding this comment

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

Approved. Like the more uniform look.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Aug 27, 2019
@behlendorf behlendorf merged commit f335b8f into openzfs:master Aug 27, 2019
@behlendorf behlendorf mentioned this pull request Sep 5, 2019
12 tasks
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 17, 2019
Entering the ZFS encryption passphrase under Plymouth wasn't working
because in the ZFS initrd script, Plymouth was calling zfs via
"--command", which wasn't passing through the filesystem argument to
zfs load-key properly (it was passing through the single quotes around
the filesystem name intended to handle spaces literally,
which zfs load-key couldn't understand).

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Garrett Fields <ghfields@gmail.com>
Signed-off-by: Richard Allen <belperite@gmail.com>
Issue openzfs#9193
Closes openzfs#9202
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 18, 2019
Entering the ZFS encryption passphrase under Plymouth wasn't working
because in the ZFS initrd script, Plymouth was calling zfs via
"--command", which wasn't passing through the filesystem argument to
zfs load-key properly (it was passing through the single quotes around
the filesystem name intended to handle spaces literally,
which zfs load-key couldn't understand).

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Garrett Fields <ghfields@gmail.com>
Signed-off-by: Richard Allen <belperite@gmail.com>
Issue openzfs#9193
Closes openzfs#9202
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 18, 2019
Entering the ZFS encryption passphrase under Plymouth wasn't working
because in the ZFS initrd script, Plymouth was calling zfs via
"--command", which wasn't passing through the filesystem argument to
zfs load-key properly (it was passing through the single quotes around
the filesystem name intended to handle spaces literally,
which zfs load-key couldn't understand).

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Garrett Fields <ghfields@gmail.com>
Signed-off-by: Richard Allen <belperite@gmail.com>
Issue openzfs#9193
Closes openzfs#9202
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 18, 2019
Entering the ZFS encryption passphrase under Plymouth wasn't working
because in the ZFS initrd script, Plymouth was calling zfs via
"--command", which wasn't passing through the filesystem argument to
zfs load-key properly (it was passing through the single quotes around
the filesystem name intended to handle spaces literally,
which zfs load-key couldn't understand).

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Garrett Fields <ghfields@gmail.com>
Signed-off-by: Richard Allen <belperite@gmail.com>
Issue openzfs#9193
Closes openzfs#9202
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 19, 2019
Entering the ZFS encryption passphrase under Plymouth wasn't working
because in the ZFS initrd script, Plymouth was calling zfs via
"--command", which wasn't passing through the filesystem argument to
zfs load-key properly (it was passing through the single quotes around
the filesystem name intended to handle spaces literally,
which zfs load-key couldn't understand).

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Garrett Fields <ghfields@gmail.com>
Signed-off-by: Richard Allen <belperite@gmail.com>
Issue openzfs#9193
Closes openzfs#9202
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 23, 2019
Entering the ZFS encryption passphrase under Plymouth wasn't working
because in the ZFS initrd script, Plymouth was calling zfs via
"--command", which wasn't passing through the filesystem argument to
zfs load-key properly (it was passing through the single quotes around
the filesystem name intended to handle spaces literally,
which zfs load-key couldn't understand).

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Garrett Fields <ghfields@gmail.com>
Signed-off-by: Richard Allen <belperite@gmail.com>
Issue openzfs#9193
Closes openzfs#9202
tonyhutter pushed a commit that referenced this pull request Sep 26, 2019
Entering the ZFS encryption passphrase under Plymouth wasn't working
because in the ZFS initrd script, Plymouth was calling zfs via
"--command", which wasn't passing through the filesystem argument to
zfs load-key properly (it was passing through the single quotes around
the filesystem name intended to handle spaces literally,
which zfs load-key couldn't understand).

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Garrett Fields <ghfields@gmail.com>
Signed-off-by: Richard Allen <belperite@gmail.com>
Issue #9193
Closes #9202
paper42 added a commit to paper42/zfs that referenced this pull request May 28, 2021
plymouth --command splits the command on spaces which means
that zfs-load-key was getting the filesystem name enclosed
in single quotes (since 13c59bb) and failing. This commit
fixes it by piping the password directly to the command
similar to how it's done in other scripts (initramfs,
dracut without plymouth).

Signed-off-by: Michal Vasilek <michal@vasilek.cz>
Related-to: openzfs#9193
Related-to: openzfs#9202
behlendorf pushed a commit that referenced this pull request Jun 26, 2021
plymouth --command splits the command on spaces which means
that zfs-load-key was getting the filesystem name enclosed
in single quotes (since 13c59bb) and failing. This commit
fixes it by piping the password directly to the command
similar to how it's done in other scripts (initramfs,
dracut without plymouth).

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Michal Vasilek <michal@vasilek.cz>
Related-to: #9193
Related-to: #9202
Closes #12147
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 29, 2021
plymouth --command splits the command on spaces which means
that zfs-load-key was getting the filesystem name enclosed
in single quotes (since 13c59bb) and failing. This commit
fixes it by piping the password directly to the command
similar to how it's done in other scripts (initramfs,
dracut without plymouth).

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Michal Vasilek <michal@vasilek.cz>
Related-to: openzfs#9193
Related-to: openzfs#9202
Closes openzfs#12147
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants