-
Notifications
You must be signed in to change notification settings - Fork 41
feat: add check-advisories to check BRSA fields #604
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
base: develop
Are you sure you want to change the base?
Conversation
ba9f677 to
1eb07a3
Compare
jmt-lab
left a comment
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.
LGTM
ed9e89a to
9b86a6b
Compare
|
Fixed some of the comments above. |
34db683 to
a78f0ec
Compare
|
Resolved all comments. |
6179367 to
8a85572
Compare
cbgbt
left a comment
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.
I had to fix the advisories in the earlier versions because they didn't have patched-epoch field. We can plan on either defaulting to 0 here or add those fields in the advisories.
Zero as a default should be OK. RPM treats unspecified as 0.
I noticed that bottlerocket-core-kit/advisories/2.1.0/BRSA-1j0o73qa.toml has an odd patched version:
patched-version = "kernel-5.15.160-104.158.amzn2"
I suppose this just shows up as a warning? What should happen here?
|
8d45429 to
f064191
Compare
|
Fixed all comments. |
e24be77 to
d3f188d
Compare
0d1536d to
5ef22a5
Compare
cbgbt
left a comment
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.
Do we still need changes to Makefile.toml to ensure that these get run during a kit build? Maybe I missed it, but I'm not seeing how this gets invoked.
This runs as part of the |
twoliter/embedded/build.Dockerfile
Outdated
| RUN --mount=source=advisories,target=/home/builder/advisories \ | ||
| --mount=target=/host \ | ||
| /host/build/tools/advisory-checker rpmbuild/SPECS/${PACKAGE}.spec /home/builder/advisories/ |
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.
Better to run this before the package build.
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.
I placed it before the rpmbuild but after we put the sources and macros in the right place. Should I move this before that and add the line for copying macros into this?
dc3a561 to
8802745
Compare
0e976b4 to
ebf88db
Compare
|
rebased ^ |
83241ec to
7592ad1
Compare
| assert!(output.status.success()); | ||
| let stdout = String::from_utf8_lossy(&output.stdout); | ||
|
|
||
| let output_pattern = r"Advisories directory .* not found, skipping"; |
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.
I'm glad that this behavior is documented but it has me wondering if we need to consider how to protect against cases where we accidentally point the checker at the wrong directory. Should the script fail if the directory doesn't exist? That seems like maybe a choice that would reduce the chance of making a mistake.
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.
That was the behaviour I implemented initially but switched to this because of this comment - #604 (comment). There may be kit users who get rid of the advisories directory and we don't want to break the build for them.
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.
I wonder if the "right way" is to make it so that we don't invoke the tool when there are no advisories present, but the invocation of the tool implies the presence of advisories.
7592ad1 to
9bf36f1
Compare
tools/advisory-checker/src/main.rs
Outdated
| parse_rpm_metadata(pkg_prefix, output) | ||
| } | ||
|
|
||
| fn parse_rpm_metadata(pkg_prefix: String, output: String) -> Result<HashMap<String, String>> { |
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 function does some kind of string processing, but it's actually hard to tell just by looking at the function signature what. At the very least, it needs a docstring.
The "Rust way" would lean more heavily on using a composite type instead of a HashMap though -- You could alter the return type to be Result<HashMap<PackageName, EVR>> where PackageName and EVR were newtypes or type aliases. Alternatively you could return Result<PackageVersionManifest> or some kind of composite type that documented what was being processed.
With the existing code "parse_rpm_metadata" could mean any kind of information from the RPM, and the function signature doesn't really tell you what. The only way to know what the code is doing is to read all of it.
9bf36f1 to
26035a0
Compare
Signed-off-by: Piyush Jena <jepiyush@amazon.com>
26035a0 to
f585605
Compare
Description of changes:
Add
check-advisoriestaskHow it works?
We implement a
advisory-checkerrust crate that takes 2 arguments - The rpmspec file and advisories directory. The binary is called during build by the upper level build.Dockerfile. Since, its already in the sdk context and the rpm macros are loaded, we can userpmspecandrpm --evalmacros to extract package name and versions. We deserialize the advisory files to extract the expected version and then compare using rpm.vercmp to evaluate if we are in a newer version.Testing done:
Cropping output for brevity.
Testing it on
bottlerocket-core-kit. I introduced 2 errors to test different cases. I created a sub-package BRSA with wrong epoch which got flagged. A different package with wrong version also got flagged.Testing it on
bottlerocket-kernel-kit.Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.