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

ImageCustomizer - Service and Overlay recommendations for Verity-enabled images. #10608

Merged
merged 7 commits into from
Oct 9, 2024

Conversation

liulanze
Copy link
Contributor

@liulanze liulanze commented Oct 2, 2024

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • The toolchain has been rebuilt successfully (or no changes were made to it)
  • The toolchain/worker package manifests are up-to-date
  • Any updated packages successfully build (or no packages were changed)
  • Packages depending on static components modified in this PR (Golang, *-static subpackages, etc.) have had their Release tag incremented.
  • Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • All package sources are available
  • cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • LICENSE-MAP files are up-to-date (./LICENSES-AND-NOTICES/SPECS/data/licenses.json, ./LICENSES-AND-NOTICES/SPECS/LICENSES-MAP.md, ./LICENSES-AND-NOTICES/SPECS/LICENSE-EXCEPTIONS.PHOTON)
  • All source files have up-to-date hashes in the *.signatures.json files
  • sudo make go-tidy-all and sudo make go-test-coverage pass
  • Documentation has been updated to match any changes to the build system
  • Ready to merge

Summary

What does the PR accomplish, why was it needed?
Service and Overlay recommendations for Verity-enabled images.
Clear services down errors when booting Verity enabled images.

Does this affect the toolchain?

NO

Test Methodology
  • Local build

@liulanze liulanze requested a review from a team as a code owner October 2, 2024 22:29
@@ -550,6 +546,23 @@ os:
corruptionOption: panic
```

When enabling the Verity feature, it is recommended that overlays are applied to
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t want the official recommendations for MIC+verity to include overlays. They are a lazy person’s solution to provisioning an image for verity.

Instead, the recommendations should be to create a writable persistent partition (e.g. /var) and then redirect other files to the writable partition using symlinks and the like.

For example, verity-config.yaml redirects the SSH host keys from /etc to /var.

@@ -550,6 +546,23 @@ os:
corruptionOption: panic
```

When enabling the Verity feature, it is recommended that overlays are applied to
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that over time the recommendations for verity images could end up being quite long. There are lots of different services/programs available on Azure Linux and a number of them may need special handling under verity.

In addition, there may be multiple options for a service/program with different tradeoffs. Some users may want a service/program configuration option changeable for convenience. While other users may want it immutable for security.

As such, I recommend splitting off the verity recommendations into its own doc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable to me.

@liulanze liulanze requested review from cwize1 and romoh October 3, 2024 22:09
recommended to create a writable persistent partition (e.g., /var) for any
directories that require write access. Critical files and directories, such as
SSH host keys or logs, can be redirected to the writable partition using
symlinks or similar methods. See [Verity Filesystem Layout
Copy link
Contributor

Choose a reason for hiding this comment

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

why not call out overlays here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, Chris mentioned that overlays should not be part of the official MIC+Verity recommendations, so I removed the overlays related suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

My interpretation of Chris' comment was that it shouldn't be the sole recommendation but IMO we should include it as one possible option.

Copy link
Contributor Author

@liulanze liulanze Oct 4, 2024

Choose a reason for hiding this comment

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

It makes sense to me. Agree.

Copy link
Contributor

@romoh romoh left a comment

Choose a reason for hiding this comment

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

Left some suggestions

@@ -511,10 +511,6 @@ Example: `noatime,nodiratime`
Specifies the configuration for dm-verity root integrity verification. Please
execute `sudo modprobe nbd` before building the image with verity enablement.
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're in the area, it might be goodness to remove the reference to nbd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@@ -550,6 +546,27 @@ os:
corruptionOption: panic
```

The Verity-enabled root filesystem is always mounted as read-only. Its root hash
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move all of this prose to the verity.md file. And then in this file, only say something like: There are multiple ways to configure a verity enabled image. For recommendations see, [Verity Image Recommendations](./verity.md).

@@ -0,0 +1,16 @@
# Verity Filesystem Layout Recommendations
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably title this document 'Verity Image Recommendations`.

Some configurations can be made flexible to allow changes, while others may be
set as immutable for enhanced security.

For example, if your image includes the `systemd-growfs-root.service`, which
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using prose, let's try to make the recommendations more structured. Also, including example configs would be goodness.

For example:

## systemd-growfs-root

This service attempts to resize the root filesystem, which fails since verity makes the root filesystem readonly and a fixed size.

### Solution 1: Do nothing

Since the root filesystem is readonly, the `systemd-growfs-root` service will fail.
However, the only impact will be an error in the boot logs.

### Solution 2: Disable service

Disabling the service removes the error from the boot logs.

`` `yaml
os:
  services:
    disable:
    - systemd-growfs-root
`` `

Do something similar for other services (e.g. cloud-init, ssh, auditd, docker, etc.).

security. Some configurations can be made flexible to allow changes, while
others may be set as immutable for enhanced security.

## systemd-growfs-root
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be goodness to create a dedicated section for the var partition, since so many partitions need it to be writable. That way, you can avoid sections pointing to the cloud-init section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks.

- systemd-growfs-root
```

## cloud-init-local, cloud-config, cloud-final
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Just ## cloud-init

While there are multiple systemd services included with cloud-init, they are all part of the same application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks.

- systemd-growfs-root
```

## cloud-init-local, cloud-config, cloud-final
Copy link
Contributor

Choose a reason for hiding this comment

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

cloud-init is an odd one. cloud-init has a bunch of different features that can be used to configure the system. (e.g. user accounts, networking, etc.) But a lot of those features typically require the /etc directory to be writable.

I think the general recommendation should be to disable cloud-init and not use it.

Though it is possible that some cloud-init features can be used under verity. For those, we may end up giving them dedicated sections in this doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have re-write the cloud-init part, however, the cloud-init services I faced so far (cloud-init-local, cloud-config, cloud-final), they all need write access to /etc, so I think for the section of cloud-init features that can be used under verity, the doc can be updated when users meet them.

The following example script can be added to the `postCustomization` scripts to
automatically move the SSH host keys and update the `sshd_config` file:

```bash
Copy link
Contributor

Choose a reason for hiding this comment

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

You should provide a sample config, not just the script.

Note: Scripts can now be specified inline using content instead of path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

The following example script can be added to the `postCustomization` scripts to
automatically move the SSH host keys and update the `sshd_config` file:

```bash
Copy link
Contributor

Choose a reason for hiding this comment

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

The sshd-keygen.service file also need to be updated.

Note: Additional files now support content instead of source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

security. Some configurations can be made flexible to allow changes, while
others may be set as immutable for enhanced security.

## systemd-growfs-root
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add a section for networking.

In Mariner 3.0, there are no longer any default network settings. In non-verity images, this doesn't matter too much since cloud-init will provide default networking settings. But cloud-init fails to provision the network in verity images since /etc is not writable. Hence, it is a good idea for verity images to specify network settings. The test verity config does this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, please review.

@liulanze liulanze requested a review from a team as a code owner October 9, 2024 21:48
@liulanze liulanze merged commit 9e41255 into 3.0-dev Oct 9, 2024
12 checks passed
@liulanze liulanze deleted the user/lanzeliu/mic-verity-recommendation-fs branch October 9, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants