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

update /etc/crypttab status properly (bsc#1118977) #1087

Merged
merged 6 commits into from
Apr 23, 2020
Merged

update /etc/crypttab status properly (bsc#1118977) #1087

merged 6 commits into from
Apr 23, 2020

Conversation

wfeldt
Copy link
Member

@wfeldt wfeldt commented Apr 22, 2020

Problem

New encrypted volumes are added to /etc/crypttab (even when not mounted).

Analysis

There are two issues:

  1. The default value for in_etc is true for newly created encrytpted volumes. Since the #update_etc_status method ensures the status can't be set to false when it was originally true, this unavoidably creates an initial crypttab entry.
    To avoid this, set the in_etc flag to false explicitly for freshly created encrypted volumes.

  2. The #update_etc_status method is called when you switch between mounting/not mounting in the fstab dialog.
    But when encryption had just been enabled, the encrypted device does not yet exist in the device tree. It's created only after going through the password dialog.
    This means #update_etc_status has to be called also after creating an encryption device to get a consistent state.

Comments

  • This applies equally to /etc/mdadm.conf - currently it's not possible to avoid an entry there - even if the RAID is not mounted. The use-cases are a bit different compared to encryption, so this might be the desired state.

  • I've found the logic aroud etc_status_autoset very hard to follow and have rewritten that part to something I might still understand in a few years from now...

@coveralls
Copy link

coveralls commented Apr 22, 2020

Coverage Status

Coverage decreased (-0.0001%) to 97.64% when pulling 0f995d6 on sw_01 into be67c9e on master.

@ancorgs
Copy link
Contributor

ancorgs commented Apr 22, 2020

etc_status_autoset is there to ensure we don't remove devices from /etc/crypttab, /etc/mdadm and friends if those devices were already there at the time of probing.

We only want to revert the decision of listing the devices in those files if adding them was our decision, not if they were there for other circumstances.

@ancorgs
Copy link
Contributor

ancorgs commented Apr 22, 2020

That's why we added the test that is currently broken: to ensure we don't remove stuff that was originally in /etc/crypttab and /etc/mdadm (ie. found to be there already during probing)

@wfeldt
Copy link
Member Author

wfeldt commented Apr 23, 2020

That's why we added the test that is currently broken: to ensure we don't remove stuff

The test is wrong: it only ensures in_etc is always true. And that's wrong.

And that's also the problem with the existing code: since a new encryption has in_etc set to true the code ensures it's never false - so a new encryption is always in crypttab.

@ancorgs
Copy link
Contributor

ancorgs commented Apr 23, 2020

Code-wise, LGTM. I guess unit tests, changelog and all that will come later.

The #update_etc_status method is called when you switch between mounting/not
mounting in the fstab dialog.

But when encryption had just been enabled, the luks device does not yet
exist in the device tree. It's created only after going through the password
dialog.

So the #update_etc_status method has to be called also after creating an
encryption device.
It does the same as the old code but (IMHO) in a more brain-saving way.
Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

LGTM

@wfeldt wfeldt merged commit 9eb182a into master Apr 23, 2020
@wfeldt wfeldt deleted the sw_01 branch April 23, 2020 15:10
@yast-bot
Copy link

✔️ Public Jenkins job #297 successfully finished
✔️ Created OBS submit request #796586

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