Skip to content

LUKS activation over D-Bus#250

Merged
mvidner merged 5 commits intomasterfrom
luks-activation-over-dbus
Sep 12, 2022
Merged

LUKS activation over D-Bus#250
mvidner merged 5 commits intomasterfrom
luks-activation-over-dbus

Conversation

@mvidner
Copy link
Contributor

@mvidner mvidner commented Sep 6, 2022

Problem

#249 says: After moving questions to a separate service (see #248), it is not possible to unlock LUKS devices anymore. The problem is there is no way to ask a LUKS question over D-Bus, so those questions are always handled like generic ones.

Solution

Teach the Questions service about NewLuksActivation and add the appropriate client code.

Testing

Added new unit tests

Tested manually:

  1. set up a toy LUKS partition in the testing VM
IMG=/dev/sda3 # it is the swap partition. make sure to use a scratch one too in your testing!
swapoff $IMG
cryptsetup --force-password luksFormat $IMG  # force accepting a simple password
  1. now run the installer and click through

  2. Once the installer successfully unlocks the partition, we need to close it manually to be able to rerun the test: cryptsetup close cr-auto-1

See also man cryptsetup

Screenshots

d-installer-luks-question

@coveralls
Copy link

coveralls commented Sep 6, 2022

Pull Request Test Coverage Report for Build 3022757723

  • 19 of 21 (90.48%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-3.3%) to 73.023%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/lib/dinstaller/dbus/clients/questions_manager.rb 7 9 77.78%
Totals Coverage Status
Change from base Build 2933827024: -3.3%
Covered Lines: 1708
Relevant Lines: 2339

💛 - Coveralls

@mvidner
Copy link
Contributor Author

mvidner commented Sep 6, 2022

@imobachgs on IRC you said that "questions_manager looks weird when used over D-Bus". Can you please explain a bit?

@imobachgs
Copy link
Contributor

@imobachgs on IRC you said that "questions_manager looks weird when used over D-Bus". Can you please explain a bit?

Sure. My concerns are visible in the CanAskQuestion module. As a client, you pass a question object to the add method (Question class) and, if you are using D-Bus, you get an DBus::Question instance (asked_question).

We need to make sure that Question and DBus::Question implement a common interface (that should be enough). Or we could work with IDs, so add returns some kind of ID and wait receive such an ID. In that way, as a client, you do not worry which kind of question you are working with.

@mvidner
Copy link
Contributor Author

mvidner commented Sep 6, 2022

make sure that Question and DBus::Question implement a common interface (that should be enough).

OK. I did cheat in #231 and only implemented a bare minimum interface in DI::DBus::Clients::Question (as opposed to DI::Question)
https://github.com/yast/d-installer/pull/231/files#diff-0b3ec193f1637a485a5350ffced5f13432d20b7166fb8855e781e7020f000c7cR47

Do you really mean DI::DBus::Question instead?

@imobachgs
Copy link
Contributor

make sure that Question and DBus::Question implement a common interface (that should be enough).

OK. I did cheat in #231 and only implemented a bare minimum interface in DI::DBus::Clients::Question (as opposed to DI::Question) https://github.com/yast/d-installer/pull/231/files#diff-0b3ec193f1637a485a5350ffced5f13432d20b7166fb8855e781e7020f000c7cR47

Do you really mean DI::DBus::Question instead?

No, you are right, I meant DBus::Clients::Question.

@mvidner mvidner force-pushed the luks-activation-over-dbus branch from c727f78 to 6bc1494 Compare September 8, 2022 13:28
It is living in a separate service now
and the calls here just raise
undefined method 'language' for DInstaller::DBus::Clients::Language

Fixes up 4261d91
@mvidner mvidner force-pushed the luks-activation-over-dbus branch from 6bc1494 to c6f4f9b Compare September 9, 2022 13:28
@mvidner mvidner marked this pull request as ready for review September 9, 2022 13:59
@manager_dbus ||= DInstaller::DBus::Manager.new(manager, logger)
end

def language_dbus
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@mvidner mvidner merged commit 61d7e00 into master Sep 12, 2022
@mvidner mvidner deleted the luks-activation-over-dbus branch September 12, 2022 07:30
@imobachgs imobachgs mentioned this pull request Nov 16, 2022
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.

3 participants