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.

Sign up for GitHub

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

Jump to bottom

Replace crypttab with lineinfile #228

Merged
merged 3 commits into from
Oct 3, 2021
Merged

Replace crypttab with lineinfile #228

merged 3 commits into from
Oct 3, 2021

Conversation

spetrosi
Copy link
Contributor

Crypttab is not available in ansible-core, hence need to replace it

@spetrosi
Replace crypttab with lineinfile
3ccd598 
Crypttab is not available in ansible-core, hence need to replace it
spetrosi
spetrosi commented Sep 17, 2021
View reviewed changes
tasks/main-blivet.yml
name: "{{ entry.name }}"
backing_device: "{{ entry.backing_device }}"
password: "{{ entry.password }}"
lineinfile:
Copy link
Contributor Author

@spetrosi spetrosi Sep 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @dwlehman, do you know if we can use the cryptsetup command for this? I didn't find a way to set password via it, and the crypttab module simply handles the config file, so I ended up using lineinfile, but maybe you can help.
The current solution with the lineinfile module works in tests_luks* consistently.
Thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

as far as I can tell - the cryptsetup command is used for modifying the runtime configuration, not the persistent configuration - it doesn't modify /etc/crypttab - the crypttab module only modifies the persistent configuration, it does not modify the runtime configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are the changes I made good to go then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes I made good to go then?

I think so - let's see if we can get some better test results.

[citest]

@richm
Copy link
Contributor

richm commented Sep 22, 2021

[citest bad]

@richm
Copy link
Contributor

richm commented Sep 22, 2021

[citest pending]

@richm
Copy link
Contributor

richm commented Sep 22, 2021

[citest bad]

@richm
Copy link
Contributor

richm commented Sep 23, 2021

@spetrosi looks like the tests_luks.yml test is failing, and it looks like it has something to do with crypttab:

TASK [linux-system-roles.storage : Mask the systemd cryptsetup services] *******
task path: /tmp/tmpq1v69liz/tasks/main-blivet.yml:50

TASK [linux-system-roles.storage : manage the pools and volumes to match the specified state] ***
task path: /tmp/tmpq1v69liz/tasks/main-blivet.yml:56
fatal: [/cache/rhel-8-y.qcow2]: FAILED! => {"actions": [], "changed": false, "crypts": [], "leaves": [], "mounts": [], "msg": "encrypted volume 'foo' missing key/password", "packages": [], "pools": [], "volumes": []}

This is rhel 8.5 and rhel 9

@spetrosi spetrosi force-pushed the replace-crypttab branch 2 times, most recently from 6efc053 to 8161493 Compare September 23, 2021 11:34
richm
richm reviewed Sep 23, 2021
View reviewed changes
tasks/main-blivet.yml Outdated Show resolved Hide resolved
@spetrosi
Add create: true to lineinfile to create the file if it doesn't exist
acc112e 
Use lower case title for consistency

Set mode properly
richm
richm approved these changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@richm richm left a comment

Choose a reason for hiding this comment

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

lgtm

@richm
Copy link
Contributor

richm commented Sep 23, 2021

[citest]

@richm
Copy link
Contributor

richm commented Sep 24, 2021

There is definitely a bug with the new implementation.
I used a fedora 34 control node and a rhel-8-y managed host
Running the tests_luks.yml test on master branch works fine, but it breaks with this fix. I'm not sure what the crypttab module is doing that makes it work.

@spetrosi
Copy link
Contributor Author

There is definitely a bug with the new implementation. I used a fedora 34 control node and a rhel-8-y managed host Running the tests_luks.yml test on master branch works fine, but it breaks with this fix. I'm not sure what the crypttab module is doing that makes it work.

The issue was that the crypttab module allowed removing entries from /etc/crypttab without providing a password. I made lineinfile ignore the password when state: absent

@richm
Copy link
Contributor

richm commented Sep 29, 2021

[citest pending]

@richm
Copy link
Contributor

richm commented Sep 29, 2021

[citest bad]

nhosoi
nhosoi approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@nhosoi nhosoi left a comment

Choose a reason for hiding this comment

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

lgtm

richm
richm reviewed Sep 30, 2021
View reviewed changes
tasks/main-blivet.yml Outdated
state: "{{ entry.state }}"
create: true
mode: "{{ __storage_crypttab.stat.mode | d('0600') }}"
mode: "{{ __storage_crypttab.stat.mode | d('0622') }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an unusual mode - -rw--w--w- - the file has to be writable by all but not readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry my bad, it should be -rw-r--r--, changed to 0644 now.

Copy link
Contributor

Choose a reason for hiding this comment

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

does the file have to be world-readable? It has a password in it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On master, the role does 0600. I mistakenly used 0644 because that's what lineinfile does by default. The file indeed stores an unencrypted password in it, so 0600 is the way to go. Sorry for the confusion and thank you for the thorough review!

@spetrosi
Copy link
Contributor Author

spetrosi commented Oct 1, 2021

[citest]

@richm
Copy link
Contributor

richm commented Oct 2, 2021

[citest bad]

@richm richm merged commit 5fc6bd3 into linux-system-roles:master Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richm richm richm approved these changes

@nhosoi nhosoi nhosoi approved these changes

@dwlehman dwlehman Awaiting requested review from dwlehman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@spetrosi @richm @nhosoi