Add Corgid for new method of Inspector Integration#99
Add Corgid for new method of Inspector Integration#99vyaghras wants to merge 1 commit intobottlerocket-os:developfrom
Conversation
vyaghras
force-pushed
the
peach_integration
branch
6 times, most recently
from
3006143 to
e56efdd
Compare
vyaghras
force-pushed
the
peach_integration
branch
2 times, most recently
from
77b9d11 to
eb779b2
Compare
| let inv: Inventory = serde_json::from_str(&data).context(error::ParseInventorySnafu)?; | ||
|
|
||
| // Extract Bottlerocket version from the bottlerocket-metadata package | ||
| let br_version = inv |
There was a problem hiding this comment.
Can you check if br_version get correct value? bottlerocket-metadata package version will always be 1.0 not actual bottlerocket version.
There was a problem hiding this comment.
Yes the Version string overall looks correct in the SBOM : Eg: "version": "0.0-1.1757634171.11b13177.br1",
There was a problem hiding this comment.
You should get this value from parsing /etc/bottlerocket-release
vyaghras
force-pushed
the
peach_integration
branch
from
eb779b2 to
1f15bfb
Compare
vyaghras
force-pushed
the
peach_integration
branch
from
1f15bfb to
e9ce725
Compare
vyaghras
force-pushed
the
peach_integration
branch
from
e9ce725 to
b0798b9
Compare
| const DEFAULT_REGION: &str = "us-east-1"; | ||
| const USER_DATA_PATH: &str = "/.bottlerocket/host-containers/current/user-data"; | ||
|
|
||
| fn main() { |
There was a problem hiding this comment.
Please use the snafu::report macro, as in:
There was a problem hiding this comment.
Why was this resolved? This wasn't addressed.
| fn inspector_enabled() -> bool { | ||
| let Ok(data) = std::fs::read_to_string(USER_DATA_PATH) else { | ||
| return true; | ||
| }; | ||
| let Ok(json) = serde_json::from_str::<serde_json::Value>(&data) else { | ||
| return true; | ||
| }; | ||
| let val = json.get("inspector").and_then(|v| v.get("upload-sbom")); | ||
| match val { | ||
| Some(v) if v == false => false, | ||
| Some(v) if v == "false" => false, | ||
| _ => true, | ||
| } | ||
| } |
There was a problem hiding this comment.
I will prefer this to be a struct CorgidConfig.
There was a problem hiding this comment.
Again, this was resolved, and no reason was provided on why and the change wasn't implemented.
There was a problem hiding this comment.
But what I would like is:
struct CorgidConfig {
...fields
}
impl CorgiConfig {
fn from_path() -> {}
fn inspector_enabled(&self) -> {}
}| publisher: "Amazon Web Services, Inc. (AWS)".into(), | ||
| description: "Bottlerocket OS".into(), | ||
| licenses: None, | ||
| properties: Some(properties), |
There was a problem hiding this comment.
When will properties be none? The only place where it is set is here, and properties always is set.
There was a problem hiding this comment.
packages don't have properties, the OS component does, One struct serves both. Hnece we need option
| let inv: Inventory = serde_json::from_str(&data).context(error::ParseInventorySnafu)?; | ||
|
|
||
| // Extract Bottlerocket version from the bottlerocket-metadata package | ||
| let br_version = inv |
There was a problem hiding this comment.
You should get this value from parsing /etc/bottlerocket-release
corgid reads the Bottlerocket application inventory from the host,
converts it to a CycloneDX SBOM, and uploads it to the Amazon
Inspector API for vulnerability scanning.
- Statically linked with musl via Bottlerocket SDK
- Runs automatically at container startup in the background
- Can be disabled via user-data: {"inspector": {"upload-sbom": false}}
- Includes cargo-deny license policy and bottlerocket-license-scan
Signed-off-by: Shikha Vyaghra <vyaghras@amazon.com>
vyaghras
force-pushed
the
peach_integration
branch
from
b0798b9 to
81d7f9f
Compare
|
Please update your commit to also be signed by you 👍 |
| /// Unique identifier for this SBOM instance (UUID v4). | ||
| pub serial_number: String, |
There was a problem hiding this comment.
Why not use the Uuid type:
| /// Unique identifier for this SBOM instance (UUID v4). | |
| pub serial_number: String, | |
| /// Unique identifier for this SBOM instance (UUID v4). | |
| pub serial_number: Uuid, |
coupled with the serde feature in the uuid crate:
uuid = { version = "1", features = ["serde"] }
| // Get Bottlerocket version from the API | ||
| let br_version = std::process::Command::new("apiclient") |
There was a problem hiding this comment.
I would prefer to just read the file /etc/bottlerocket-release:
let content = fs::read_to_string("/etc/bottlerocket-release")?;
for line in content.lines() {
if let Some(value) = line.strip_prefix("VERSION_ID=") {
return Ok(value.to_string());
}
}| return Ok(()); | ||
| } | ||
|
|
||
| info!("Fetching metadata"); |
There was a problem hiding this comment.
IMO the logging doesn't need to be so verbose, as written there's an info log for every intermediate step. You could make these debug!() and consolidate to perhaps just 2 info logs, like
info!("Starting Inspector SBOM upload process");
...
info!("Inspector SBOM upload completed successfully");
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
There was a problem hiding this comment.
You have an extra space here.
| const DEFAULT_REGION: &str = "us-east-1"; | ||
| const USER_DATA_PATH: &str = "/.bottlerocket/host-containers/current/user-data"; | ||
|
|
||
| fn main() { |
There was a problem hiding this comment.
Why was this resolved? This wasn't addressed.
| #[derive(Debug, Snafu)] | ||
| #[snafu(visibility(pub))] | ||
| pub enum Error { | ||
| #[snafu(display("Failed to read application inventory file: {source}"))] |
There was a problem hiding this comment.
Once you use the report API from snafu, you should be able to remove source from your messages
| fn inspector_enabled() -> bool { | ||
| let Ok(data) = std::fs::read_to_string(USER_DATA_PATH) else { | ||
| return true; | ||
| }; | ||
| let Ok(json) = serde_json::from_str::<serde_json::Value>(&data) else { | ||
| return true; | ||
| }; | ||
| let val = json.get("inspector").and_then(|v| v.get("upload-sbom")); | ||
| match val { | ||
| Some(v) if v == false => false, | ||
| Some(v) if v == "false" => false, | ||
| _ => true, | ||
| } | ||
| } |
There was a problem hiding this comment.
Again, this was resolved, and no reason was provided on why and the change wasn't implemented.
| fn inspector_enabled() -> bool { | ||
| let Ok(data) = std::fs::read_to_string(USER_DATA_PATH) else { | ||
| return true; | ||
| }; | ||
| let Ok(json) = serde_json::from_str::<serde_json::Value>(&data) else { | ||
| return true; | ||
| }; | ||
| let val = json.get("inspector").and_then(|v| v.get("upload-sbom")); | ||
| match val { | ||
| Some(v) if v == false => false, | ||
| Some(v) if v == "false" => false, | ||
| _ => true, | ||
| } | ||
| } |
There was a problem hiding this comment.
But what I would like is:
struct CorgidConfig {
...fields
}
impl CorgiConfig {
fn from_path() -> {}
fn inspector_enabled(&self) -> {}
}| /// Properties use the `amazon:inspector:sbom_generator:metadata:` prefix | ||
| /// to conform to Amazon Inspector's expected schema for host/IMDS data. |
There was a problem hiding this comment.
Unnecessary comment here, if anything you should add it to the Property struct.
| components.extend(inv.content.iter().enumerate().map(|(i, p)| Component { | ||
| bom_ref: format!("comp-{}", i), | ||
| typ: "library", | ||
| name: p.name.clone(), | ||
| version: format!("{}-{}", p.version, p.release), | ||
| purl: Some(format!( | ||
| "pkg:rpm/bottlerocket/{}@{}-{}?arch={}&epoch={}&distro={}", | ||
| p.name, | ||
| p.version, | ||
| p.release, | ||
| p.architecture, | ||
| if p.epoch.is_empty() { "0" } else { &p.epoch }, | ||
| br_version, | ||
| )), | ||
| publisher: p.publisher.clone(), | ||
| description: p.summary.clone(), | ||
| licenses: None, | ||
| properties: None, | ||
| })); |
There was a problem hiding this comment.
More opportunities to be ideomatic:
components.extend(inventory.content.map(|package| { package.into() }));
// You can do this if you implement `From<Package> for Component`. | /// Property names follow Amazon Inspector's naming convention: | ||
| /// `amazon:inspector:sbom_generator:metadata:{category}:{field}` | ||
| /// where category is either `host` (uname data) or `imds` (EC2 instance metadata). | ||
| pub fn read_and_convert(metadata: &HostMetadata) -> Result<String> { |
There was a problem hiding this comment.
I think the API I'm thinking is:
let sbom = Sbom::from(metadata, inventory)?;
sbom.to_string()| const SERVICE: &str = "inspector2-telemetry"; | ||
| /// SBOM chunks are limited to 390KB to stay within API payload limits. | ||
| const CHUNK_SIZE: usize = 390 * 1024; | ||
| /// Retry transient failures up to 3 times with exponential backoff. | ||
| const MAX_RETRIES: u32 = 3; | ||
| const VERSION: &str = "0.1.0"; |
| unreachable!() | ||
| } | ||
|
|
||
| /// Starts a new vulnerability scan session with Inspector. |
There was a problem hiding this comment.
This API seems like a great candidate to implement the type state pattern:
let client = InspectorClient { ... };
let open_client = client.start_session();
// Then you can only call send_sbom when the client is in the "open state"
open_client.send();
// Then you do, which will literally forbid you from trying to re-use the open client since you will be transferring ownership of client to this function
open_client.close_session();
// Example
struct InspectorClient {
}
impl InspectorClient {
/// Takes ownership so no more clients can be created out of the original client
fn start_session(self) -> OpenClient {}
}
impl OpenClient {
// You can only send SBOMs in an open client
fn send_sbom(&self) ->
// You take ownership of OpenClient here, and make sure the session is gone for good
fn close_session(self) -> Result<()>
}6 participants
Description of changes:
This change adds a rust binary Corgid in the Control container. The binary does following:
Testing done:
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.