-
Notifications
You must be signed in to change notification settings - Fork 113
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
Resolve transparent release binary filename issues #5004
Conversation
Change-Id: I586074edf736b1a886bd228c1148fcf3a060caaf
kokoro/build_binaries_rust.sh
Outdated
@@ -42,8 +42,8 @@ readonly generated_binaries=( | |||
./enclave_apps/target/x86_64-unknown-none/release/oak_orchestrator | |||
) | |||
readonly binary_names=( | |||
oak_restricted_kernel_simple_io_wrapper_bin | |||
oak_restricted_kernel_simple_io_init_rd_wrapper_bin | |||
oak_restricted_kernel_simple_io_bzimage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work, as the binary names are already locked down (use internal codesearch).
But I don't understand how this resolves the problem discussed - in artifacts_path we have the * (wildcard) which matches three files. One of the files (name needs to be configurable) is special, since it is the one for the internal TR binary package. I don't see a fundamentally different way than #5001 to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work, as the binary names are already locked down (use internal codesearch).
Good catch, there's internal references to these names. I personally think we should probably rename the internal binary names to use the bzimage suffix for clarity. But I agree, let's not do this change right now.
Updated this PR to match established internal binary names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I don't understand how this resolves the problem discussed - in artifacts_path we have the * (wildcard) which matches three files. One of the files (name needs to be configurable) is special, since it is the one for the internal TR binary package. I don't see a fundamentally different way than #5001 to fix this.
With this PR:
- the filename of the binary imported with TR remains the same as it used to be prior to Directly issue kernel provenance for attestation measurements #4976 etc.
- the tag under which its provenance is uploaded to ent remains the same.
- it remains listed as a subject in this provenance under the same name.
With the additional files (image and setup data) there's now some additional tags under which the provenance will be uploaded. But TR can ignore those.
There's now multiple subjects in the provenance, but TR can find the right one by name.
I might be missing something here, but I don't think there's a fundamentally breaking change that means we need to add new data to the config. We used filenames as identifiers in the past, and we still do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I get it now. The bzImage file name is constructed such that it coincides with the package name. Good. So assume we can remove binary_file
everywhere in #5001. But how about buildconfigs/oak_containers_kernel_bzimage.toml? This also needs a split, in exactly the same way The package name is oak_containers_kernel.
To get the package_name into the reusable_provenance.yaml we could also use the basename of the buildconfig (need to rename buildconfigs appropriately). Or we add it to the matrix in provenance.yaml. I can find a better way here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bzImage file name is constructed such that it coincides with the package name. Good. So assume we can remove binary_file everywhere in #5001.
yup!
But how about buildconfigs/oak_containers_kernel_bzimage.toml? This also needs a split, in exactly the same way The package name is oak_containers_kernel.
Yes, eventually we'll want to extend that provenance to include the image and setup data also. I'm not sure I understand why we'd need the changes in #5001 to do that, but I'd be happy to sync more on Monday. :)
Right now there haven't yet been any changes to the containers kernel.
Change-Id: Ie6e47bd929c2a309accf819da2dbc0172bede21c
Change-Id: I831ddbda48d0b61e934e7d940e630c4f4f6705ad
The tag under which provenances are uploaded must match the binary names defined in kokoro/build_binaries_rust.sh. Our existing logic derives the tag using the filename of the binary artifact. This creates an implicit, global, and unchangeable namespace for the filename of binary artifacts we import with TR.
Recent prior PRs had renamed a binary artifact of the restricted kernel.
As @thmsbinder correctly identified in #5001, this inadvertently led to an invisible collision in this implicit namespace.
This PR remedies this, by restoring the original filename of the binary artifact.