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

manifest.yaml: Add content_manifest folder to add <nvr>.json for build info #670

Closed
wants to merge 1 commit into from
Closed

manifest.yaml: Add content_manifest folder to add <nvr>.json for build info #670

wants to merge 1 commit into from

Conversation

gursewak1997
Copy link
Contributor

Add content_manifests dir to have json files for build_info

@openshift-ci openshift-ci bot requested review from lucab and peberanek November 15, 2021 19:57
@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 15, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 15, 2021

Hi @gursewak1997. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@gursewak1997 gursewak1997 changed the title manifest.yaml: Add content_manifest and <nvr>.json manifest.yaml: Add content_manifest folder to add <nvr>.json for build info Nov 15, 2021
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 15, 2021
@miabbott
Copy link
Member

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 15, 2021
@gursewak1997
Copy link
Contributor Author

/retest

manifest.yaml Outdated
Comment on lines 172 to 175
OSTREE_VERSION=$(grep -oP '(?<=^OSTREE_VERSION=).+' /etc/os-release | tr -d "'")
ARCH=`jq .basearch /usr/share/rpm-ostree/treefile.json | tr -d '"'`
RHEL_VERSION=$(grep -oP '(?<=^RHEL_VERSION=).+' /etc/os-release | tr -d '"')
OS_NAME=$(grep -oP '(?<=^ID=).+' /etc/os-release | tr -d '"')
Copy link
Member

Choose a reason for hiding this comment

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

You can source /etc/os-release and use OSTREE_VERSION, RHEL_VERSION, and OS_NAME directly.

manifest.yaml Outdated
RHEL_VERSION=$(grep -oP '(?<=^RHEL_VERSION=).+' /etc/os-release | tr -d '"')
OS_NAME=$(grep -oP '(?<=^ID=).+' /etc/os-release | tr -d '"')

mkdir -p /var/roothome/buildinfo/content_manifests && cd "$_" && touch $OS_NAME-$OSTREE_VERSION.json
Copy link
Member

Choose a reason for hiding this comment

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

We can't write to /var at compose time. Maybe /usr/share/buildinfo/content_manifests? I recall we talked about adding a tmpfiles.d dropin if we really need it available under /root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I assumed we could write to /varand not to /root. I will go with /usr/share/buildinfo/content_manifests for now. Thanks:)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we'll need to solve the problem of landing the JSON file in the specific /root/buildinfo/content_manifests location because that's where the security scanners expect it

Copy link
Member

Choose a reason for hiding this comment

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

Where can we ask these security scanners to change? Using /root for this just conflicts with ostree today, and actually more generally would be problematic for many "image based updates" models.

Even having traditional yum/rpm write to /root is pretty ugly IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Where can we ask these security scanners to change? Using /root for this just conflicts with ostree today, and actually more generally would be problematic for many "image based updates" models.

Even having traditional yum/rpm write to /root is pretty ugly IMO.

I made the assumption that it was a security scanner, but in the original ticket there is a reference to just scanners, which I guess could be anything doing any kind of scanning.

Since this request came from RHT ProdSec, we'd have to follow-up with them in https://issues.redhat.com/browse/GRPA-3731

Copy link
Member

Choose a reason for hiding this comment

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

I think it is fine to start working to change the security scanners, though I don't know how fast we can make that happen. Let's work with ProdSec to understand which scanners are looking for this file and see if we can get them modified.

Additionally, @ashcrow pointed out that the location of the file comes from OSBS [1][2], so we could work with OSBS to modify the location of the JSON file. Or suggest a secondary location if the first location isn't available.

But given the nature of this request, I think we have to work within the constraints of the existing tools, so we should pursue the use of tmpfiles.d to land the file in the location desired.

[1] https://github.com/containerbuildsystem/atomic-reactor/blob/master/atomic_reactor/constants.py#L200
[2] https://osbs.readthedocs.io/en/latest/users.html#image-content-manifests

Copy link
Member

Choose a reason for hiding this comment

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

There's a reason though that ostree designs the constraints it does around /usr and /var etc. We can certainly use tmpfiles.d or a custom systemd unit or whatever to write this content in /var.

But the thing is - that means that rpm-ostree rollback will mean the data in /var is out of sync with /usr.

This doesn't matter a huge amount for containers because the larger container ecosystem doesn't support in-place offline updates - it's all about either running yum update (in place, online), or just tearing down the whole container and starting a new one (Kubernetes native style).

As I commented on the ticket, because rpm-ostree natively sticks this data in the commit object today, it is versioned alongside the deployment, which means that one can easily do e.g. diffs between them, and rpm-ostree rollback will always have the right data.

(Also, data in /var is mutable by default, whereas clearly this should be immutable)

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, the proposal here is to make e.g. /root/buildinfo be a symlink to /usr/share/buildinfo, not to copy.
Agree though that ideally we get scanners and other producers to use the data that's already available in commit metadata.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for commenting on the Jira ticket with your input. I think we all completely missed the use of rpmostree.rpmmd-repos as a solution to this problem.

However, since the current tooling is looking for the information in a specific location, I still think we have to provide the data there...somehow.

Perhaps this is hacky, but might provide a solution given the constraints:

  1. postprocess script that writes out the desired JSON file at /usr/share/buildinfo/content_manifests/<nvr>.json
  2. a systemd unit that runs at boot which ensures there is a /var/roothome/buildinfo/content_manifests/ directory
  3. same systemd unit creates a symlink from /usr/share/buildinfo/content_manifests/<nvr>.json to /var/roothome/buildinfo/content_manifests/<nvr>.json

WDYT?

Choose a reason for hiding this comment

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

I think the location of the content_manifests/.json does not have to be in /root/buildinfo, and /usr/share is fine. We can talk to our security scanning partners about the location once the content is ready.

Also, I don't think we can use the rpmostree.rpmmd-repos as is. We need to translate those repositories names to something in pulp, like the the repositories listed here, or directly to CPE (from Errata tool).

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 19, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gursewak1997

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2021
@gursewak1997
Copy link
Contributor Author

/retest

@gursewak1997 gursewak1997 marked this pull request as ready for review November 22, 2021 06:17
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 22, 2021
@gursewak1997 gursewak1997 requested a review from jlebon November 22, 2021 16:35
@@ -0,0 +1,10 @@
# This file is part of systemd.
Copy link
Member

Choose a reason for hiding this comment

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

From #670 (comment) it seems like we don't need 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.

True but @miabbott suggested on going through with the creation of .conf file and symlinking the JSON file to /roothome.

set -euo pipefail

REPOS=`jq .repos /usr/share/rpm-ostree/treefile.json`
ARCH=`jq .basearch /usr/share/rpm-ostree/treefile.json | tr -d '"'`
Copy link
Member

Choose a reason for hiding this comment

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

jq -r will print the string without quotes. Or you should be be able to just use $(arch) for this.

. /etc/os-release
mkdir -p /usr/share/buildinfo/content_manifests && cd "$_" && touch $ID-$OSTREE_VERSION.json
# Make an empty json and add all the relevant fields
echo "$(var=$REPOS; jq --argjson repos "$var" '. +=
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 entirely follow the jq syntax here, though one thing to note is that the repo names need to be translated to those in https://access.redhat.com/security/data/metrics/repository-to-cpe.json which I think is missing here.

The actual set of repos we use doesn't really change that often, so for this first iteration, we could just hardcode the fact that e.g. rhel-8-baseos maps to rhel-8-for-$ARCH-baseos-rpms, rhel-8-appstream to rhel-8-for-$ARCH-appstream-rpms, etc... and error out if we get a repo name we don't know.

Maybe cleaner eventually is to make that mapping part of the repo files themselves and add a treefile option to teach rpm-ostree to extract it and generate the JSON file and potentially even validate against https://access.redhat.com/security/data/metrics/repository-to-cpe.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mapping the repo names to their pulp repo IDs (ie, "[rhel-8-baseos]" to "[rhel-8-for-x86_64-baseos-eus-rpms__8_DOT_4]") would be a bit more complex task to put in postprocess so we decided to not do that here. I have just added the given repo names for now so that we can access them when mapping them to repository-to-cpe.json
So far, we are just landing the changes to create the relevant DIR/JSON file and symlinking it to /root.

Copy link
Member

Choose a reason for hiding this comment

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

Had a chat with @gursewak1997 about this. I think in the end doing this in a postprocess script is probably not the right approach. We could hardcode equivalencies here, though the second we change the repo definition files in the internal redhat-coreos repo, it'll become incorrect data. Because of this, I think this data belongs best sitting alongside the repo files.

Apart from the rpm-ostree suggestion in the comment above, another easier option is a YAML file similar to the official content_sets.yml currently in use (internal example):

# content_sets.yaml
repo_mapping:
  rhel-8-baseos:
    name: rhel-8-for-$ARCH-baseos-eus-rpms__8_DOT_4
  rhel-8-appstream:
    name: rhel-8-for-$ARCH-appstream-eus-rpms__8_DOT_4
  ...

(This would live in the redhat-coreos repo so it could be maintained atomically with the repo definitions and would be similarly copied into the workdir before builds.)

And then cosa build would check for the existence of that file and auto-generate the JSON file to inject in the OS.

Copy link
Member

Choose a reason for hiding this comment

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

My understanding was that Gursewak was going to work on the tooling to generate the <nvr>.json file and we would handle the problem of the repo names separately. The last working idea was just to rename all the repos to match what is in the repository-to-cpe.json file after the tooling was in place.

The YAML mapping file would also work; either way we are going to incur some amount of manual cost to maintain that the repo names are valid according to repository-to-cpe.json.

Copy link
Member

@miabbott miabbott Nov 29, 2021

Choose a reason for hiding this comment

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

I spoke with @jlebon today and we discussed various approaches to this, but ultimately settled on the proposal that Jonathan made in his last comment:

  • a YAML doc that lives alongside the repo files which maps repo names to names in repository-to-cpe.json
    • additionally should include the baseurl so we can detect when that changes
  • changes to cosa build that can consume the mapping YAML doc and produce the <nvr>.json file
  • (future project) CI job on openshift/release that enforces changing the mapping YAML doc when changing the repo files

@gursewak1997 if you have any additional questions/concerns, please let us know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants