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 dracut not enabling network in initramfs #49

Closed
wants to merge 13 commits into from

Conversation

Freakygnome
Copy link

RHEL 8 dracut is no longer adding rd.neednet=1 to initramfs by default.
Updated to match Red Hat's recommendation.
Refer Section 10.9 of the RHEL 8 security hardening manual.

RHEL 8 dracut is no longer adding rd.neednet=1 to initramfs by default.
Updated to match Red Hat's recommendation.
Refer Section 10.9 of the RHEL 8 security hardening manual.
@sergio-correia
Copy link
Member

I think it would be better to use a dracut configuration file for this, so it would be more "permanent". Passing the extra configuration directly in the dracut invocation will work for this particular call performed by the role, but the next time the initramfs is updated -- e.g. during a kernel update --, dracut will not be executed with these extra parameters .

@Freakygnome
Copy link
Author

Good point would adding a step to the main-clevis.yml to check and modify the dracut config be suitable?

@@ -4,9 +4,9 @@
name: "{{ __nbde_client_packages }}"
state: present

- name: Update clevis dracut config to enable networking if Tang binding is found
- name: Create/Update nbde_client dracut config to enable networking if Tang binding is found
lineinfile:
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't recommend the use of lineinfile - https://github.com/redhat-cop/automation-good-practices/tree/main/coding_style
I would suggest using the file module, creating a role files/ subdirectory, and adding nbde_client.conf to the files/ subdirectory. If you think there will be other parameters to add to this config in the future, and the values of those configs will come from role variables, or be conditional, etc. then I suggest creating a template and using the templates/ subdirectory.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the tip I'll go with templates since this may also assist with #22

@Freakygnome Freakygnome marked this pull request as draft June 8, 2021 15:20
@Freakygnome Freakygnome requested a review from richm June 8, 2021 15:31
@richm
Copy link
Contributor

richm commented Jun 8, 2021

[citest commit:a3f9f5d653cdbc9467eb3f89d91f30c00efe5081]

@richm
Copy link
Contributor

richm commented Jun 8, 2021

I fixed the black errors - if you rebase on top of the latest, those will go away. However, the yamllint and ansible-lint issues need to be fixed.

@richm
Copy link
Contributor

richm commented Jul 6, 2021

ping - can you rebase?

@richm
Copy link
Contributor

richm commented Jul 14, 2021

[citest commit:1a0300a592d9eac9a06d5bc772a494b0d78a7d91]

@richm
Copy link
Contributor

richm commented Jul 21, 2021

ping - ready for review?

@Freakygnome
Copy link
Author

Not yet hoping to get this ready in the next few days

remove trailing space
@@ -0,0 +1 @@
hostonly_cmdline=yes
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again, it may make sense to have kernel_cmdline="rd.neednet=1" here instead of hostonly_cmdline=yes, to have network enabled in the initrd unconditionally.

@Freakygnome: are you planning on updating this PR and marking it as ready to review?

Copy link
Author

Choose a reason for hiding this comment

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

Yes would details on the template need to be added to the readme?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes would details on the template need to be added to the readme?

No - the README is only for documenting the public api, not implementation details.

- name: Generate nbde_client dracut config
template:
src: nbde_client.conf
dest: /etc/dracut.conf.d/nbde_client.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

what ownership/permissions should this file have?

owner: root
mode: u=rw

?

Copy link
Member

Choose a reason for hiding this comment

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

That would work; u=rw,g=r,o=r would be okay as well.

richm and others added 2 commits July 23, 2021 09:59
Using `u=rw` with no `g` or `o` - prefer less access by default unless more access is needed
@Freakygnome Freakygnome marked this pull request as ready for review July 26, 2021 08:07
@richm
Copy link
Contributor

richm commented Jul 27, 2021

[citest commit:912106cb5c79b770c0beab7e0e71066b4f777c37]

remove trailing space
use curly braces instead of parentheses
@richm
Copy link
Contributor

richm commented Jul 28, 2021

@ukulekek the statuses are stuck in pending - will this be fixed by using tft-bot?

@richm
Copy link
Contributor

richm commented Jul 29, 2021

[citest commit:912106cb5c79b770c0beab7e0e71066b4f777c37]

@richm
Copy link
Contributor

richm commented Jul 29, 2021

[citest commit:c84a41bf2e92158f94d9082244e87f52794c1e78]

# Settings used configuring dracut
nbde_client_dracut_settings:
- kernel_cmdline="rd.neednet=1"
- ip="dhcp"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to pass ip="dhcp" explicitly, as it may cause problems for people setting up network manually.

Copy link
Author

Choose a reason for hiding this comment

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

Should this be added as an example but commented out or just removed I don't believe at actually required.
In which case should the rd.neednet be set to on by default?

Copy link
Member

@sergio-correia sergio-correia Aug 23, 2021

Choose a reason for hiding this comment

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

Set rd.neednet=1 by default, since the role requires networking in the initramfs, but remove ip=dhcp as it may cause issues for configurations that already pass ip= to e.g. specify a static IP.

Removed ip config line as this may cause issues with existing
configurationsthat already use the.
@sergio-correia
Copy link
Member

Looks good to me, thanks. @richm, could you take another look as well, please?

@@ -19,4 +19,8 @@ nbde_client_provider: clevis
# - http://server2.example.com
nbde_client_bindings: []

# Settings used configuring dracut
nbde_client_dracut_settings:
- kernel_cmdline="rd.neednet=1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that this role needs to reboot the machine in order for the changes to take effect? If so, we have a few roles that have to do something similar. See https://github.com/linux-system-roles/kernel_settings#role-variables kernel_settings_reboot_ok and https://github.com/linux-system-roles/kernel_settings#variables-exported-by-the-role kernel_settings_reboot_required

Copy link
Member

Choose a reason for hiding this comment

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

This role sets up the machine so that next time it reboots, it will be able to use the configured tang servers for auto unlocking of its LUKS devices. To do that, it will need networking enabled in the initramfs during early boot, so that is what this change is doing.

I think the case here is different than the one you quoted, in terms of needing a reboot in order for the changes to take effect, although in practice the machine will have to be rebooted eventually if we want to see auto-unlocking on boot in action.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah - right - this is only for when the machine is actually rebooted and needs to unlock LUKS at boot time

lgtm

@richm
Copy link
Contributor

richm commented Oct 11, 2021

[citest commit:108e28f6981107bd28ab116b2390955d9e2a9464]

@richm
Copy link
Contributor

richm commented Oct 11, 2021

[citest commit:f70235e757120fa54c9109eb9b848594de30c9d2]

@richm
Copy link
Contributor

richm commented Oct 11, 2021

[citest pending]

2 similar comments
@richm
Copy link
Contributor

richm commented Oct 11, 2021

[citest pending]

@richm
Copy link
Contributor

richm commented Oct 12, 2021

[citest pending]

@@ -19,4 +19,8 @@ nbde_client_provider: clevis
# - http://server2.example.com
nbde_client_bindings: []

# Settings used configuring dracut
nbde_client_dracut_settings:
- kernel_cmdline="rd.neednet=1"
Copy link
Contributor

Choose a reason for hiding this comment

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

The presence of nbde_client_dracut_settings with a value means that this is a public variable which is intended to be set to a possibly different value by a user. Is this desirable? Or is the intention that kernel_cmdline="rd.neednet=1" is always set? Is it always needed? Is there any situation where the user would need to change this value, or prevent it from being added? Because an alternative is to just hardcode this value in the nbde_client.conf below.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is intended that the user can set this value, then it should be documented in the README.md, and there should be a test which sets this value to a non-default value, and verifies that the value was set.

@richm
Copy link
Contributor

richm commented Oct 20, 2021

[citest pending]

1 similar comment
@richm
Copy link
Contributor

richm commented Nov 3, 2021

[citest pending]

@richm
Copy link
Contributor

richm commented Jan 10, 2022

how does this PR relate to #58 ?

@richm
Copy link
Contributor

richm commented Jan 10, 2022

[citest pending]

@richm
Copy link
Contributor

richm commented Jan 10, 2022

[citest bad]

@sergio-correia
Copy link
Member

how does this PR relate to #58 ?

how does this PR relate to #58 ?

That other PR would be a superset of this, as it also enables network in the initramfs. This one could be closed, if the other one gets merged.

@richm
Copy link
Contributor

richm commented Feb 1, 2022

how does this PR relate to #58 ?

how does this PR relate to #58 ?

That other PR would be a superset of this, as it also enables network in the initramfs. This one could be closed, if the other one gets merged.

ok - closing

@richm richm closed this Feb 1, 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.

None yet

3 participants