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.

Sign up for GitHub

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

Generate source code SBOM in 'context' mode #1430

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+61 −37
Open

Generate source code SBOM in 'context' mode #1430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
34d5faa
Run SBOM for 'context' mode
sergii-ssh Mar 16, 2023
c4467a3
fix lint
sergii-ssh Mar 16, 2023
ea29811
Add version prefix
sergii-ssh Mar 16, 2023
d9d505f
Add sbom uri to build.sh
sergii-ssh Mar 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions pkg/build/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,7 @@ func Build(manifest model.Manifest) error {
return fmt.Errorf("failed to package license file: %v", err)
}

if manifest.DockerOutput == model.DockerOutputContext {
log.Warnf("Docker output in 'context' mode; will not produce SBOM.")
} else if manifest.SkipGenerateBillOfMaterials {
if manifest.SkipGenerateBillOfMaterials {
Copy link

Choose a reason for hiding this comment

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

We create a new option, SkipGenerateBillOfMaterials, and we output a message if set, but I don't see that we actually act on it. I would have expected some check around the actual BOM generation to not run if this value is set.

log.Warnf("Input manifest set SkipGenerateBillOfMaterials; will not produce SBOM.")
} else {
if err := GenerateBillOfMaterials(manifest); err != nil {
Expand Down
77 changes: 48 additions & 29 deletions pkg/build/sbom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,42 +26,38 @@ import (
"istio.io/release-builder/pkg/util"
)

// Sbom generates Software Bill Of Materials for istio repo in an SPDX readable format.
const (
sbomOutputURI string = "https://storage.googleapis.com/istio-release/releases"
Copy link

Choose a reason for hiding this comment

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

We generate images to multiple places: We may also put them in istio-prerelease-testing or istio-testing depending on what we are building. That said I see we seem to use a single URI in the replaced code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was hardcoded so I am keeping it not to break anything. This is the default unless manifest.BillOfMaterialsURI is specified.
I suspect sbom location needs to be specified here as well? https://github.com/istio/release-builder/blob/master/release/build.sh#L38

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated build script. Now we'll upload SBOMs to the same bucket as other artifacts.

)

// GenerateBillOfMaterials generates Software Bill Of Materials for istio repo in an SPDX readable format.
func GenerateBillOfMaterials(manifest model.Manifest) error {
// Retrieve istio repository path to run the sbom generator
istioRepoDir := manifest.RepoDir("istio")
sourceSbomFile := path.Join(manifest.OutDir(), "istio-source.spdx")
sourceSbomNamespace := fmt.Sprintf("https://storage.googleapis.com/istio-release/releases/%s/istio-source.spdx",
manifest.Version)
releaseSbomFile := path.Join(manifest.OutDir(), "istio-release.spdx")
releaseSbomNamespace := fmt.Sprintf("https://storage.googleapis.com/istio-release/releases/%s/istio-release.spdx",
manifest.Version)

// construct all the docker image tarball names as bom currently cannot accept directory as input
dockerDir := path.Join(manifest.OutDir(), "docker")
dockerImages := []string{}
if err := filepath.Walk(dockerDir, func(path string, fi os.FileInfo, err error) error {
if err != nil {
return err
}
if fi == nil {
return fmt.Errorf("failed to get fileinfo for file at path %s", path)
}
if fi.IsDir() {
return nil
}
dockerImages = append(dockerImages, path)
return nil
}); err != nil {
return fmt.Errorf("failed to walk directory %s: %v", dockerDir, err)
nameSpaceURI := sbomOutputURI
if manifest.BillOfMaterialsURI != "" {
nameSpaceURI = manifest.BillOfMaterialsURI
}
sourceSbomFile := path.Join(manifest.OutDir(), fmt.Sprintf("istio-source-%s.spdx", manifest.Version))
sourceSbomNamespace := path.Join(nameSpaceURI, manifest.Version, fmt.Sprintf("istio-source-%s.spdx", manifest.Version))

releaseSbomFile := path.Join(manifest.OutDir(), fmt.Sprintf("istio-source-%s.spdx", manifest.Version))
releaseSbomNamespace := path.Join(nameSpaceURI, manifest.Version, fmt.Sprintf("istio-source-%s.spdx", manifest.Version))

// Run bom generator to generate the software bill of materials(SBOM) for istio.
log.Infof("Generating Software Bill of Materials for istio release artifacts")
if err := util.VerboseCommand("bom", "--log-level", "error", "generate", "--name", "Istio Release "+manifest.Version,
"--namespace", releaseSbomNamespace, "--ignore", "licenses,'*.sha256',docker", "--dirs", manifest.OutDir(),
"--image-archive", strings.Join(dockerImages, ","), "--output", releaseSbomFile).Run(); err != nil {
return fmt.Errorf("couldn't generate sbom for istio release artifacts: %v", err)
// For Docker output in 'context' mode we will not produce SBOM.
// `bom` can produce bill only for tar and remote images.
Comment on lines +49 to +50
Copy link

Choose a reason for hiding this comment

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

@sergii-ssh What happens when you build in "context" mode? Happy to learn about the use case if you think a feature is missing. Or is it for local development?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @puerco In context mode images we build are stored in local registry/context, i.e. gcr.io/istio-release/pilot:1.17.2. Now if I run bom generate -o istio-release.spdx --image gcr.io/istio-release/pilot:1.17.2 it will fail because image hasn't been pushed to remote registry yet.

FATA generating doc: creating SPDX document: generating SPDX package from image ref gcr.io/istio-release/pilot:1.17.2: while downloading images to archive: fetching remote descriptor: GET https://gcr.io/v2/istio-release/pilot/manifests/1.17.2: MANIFEST_UNKNOWN: Failed to fetch "1.17.2" from request "/v2/istio-release/pilot/manifests/1.17.2".

Workaround is to docker save and then run with --image-archive but it would be nice to have something like --image-local flag to avoid doing so.

Copy link

Choose a reason for hiding this comment

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

Can we not point to the images that were created when in context (that's a bad name) mode? They won't been the final location. Or is the SBOM going to end up with inaccurate data? Maybe it would be accurate if we set sbomOutputURI to the correct value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SBOM location is configurable in manifest.BillOfMaterialsURI and filenames are stamped with version. Accuracy shouldn't be an issue unless spdx report overwriten on purpose.

Copy link

Choose a reason for hiding this comment

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

We used to generate a message that BOM generation wouldn't happen if we were in context mode and removed that above. If we are in context mode and have not specified to skip the generation, we will not have generated an SBOM, and there would have been no messages in the log that SBOM generation was skipped.

if manifest.DockerOutput == model.DockerOutputTar {
Copy link
Member

Choose a reason for hiding this comment

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

i am wondering where we are running SBOM in 'context' mode as the PR title says.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we don't call bom at all if context mode is specified. I am enabling it here https://github.com/istio/release-builder/pull/1430/files#diff-e3115b8ca9fdf611ae3dc61dc235e39aeeddbca61036b803ba3c0826f25cc3c9L91

For this snippet. I am skipping sbom generation for docker artifacts since bom doesn't support local context/registries but generate one for the source code.

Copy link
Member

Choose a reason for hiding this comment

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

OK, so for source code we need to generate SBOM, as docker output mode does not matter in that case. Previously we were fully skipping. @puerco is this correct, that bom cannot support local registries? I remember you mentioning that it also should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@puerco is it possible to generate bom for local registers without exporting to tar?

Copy link
Member

Choose a reason for hiding this comment

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

Could you please clarify how the PR title "[Run SBOM in 'context' mode]" makes sense now? You run it in context mode, but the SBOM is generated only for source code right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the title to highlight that explicitly.

dockerDir := path.Join(manifest.OutDir(), "docker")
// construct all the docker image tarball names as bom currently cannot accept directory as input
dockerImages := DockerTarPaths(dockerDir)
if err := util.VerboseCommand("bom", "--log-level", "error",
"generate", "--name", "Istio Release "+manifest.Version,
"--namespace", releaseSbomNamespace, "--ignore", "licenses,'*.sha256',docker", "--dirs", manifest.OutDir(),
"--image-archive", strings.Join(dockerImages, ","), "--output", releaseSbomFile).Run(); err != nil {
return fmt.Errorf("couldn't generate sbom for istio release artifacts: %v", err)
}
}

// Run bom generator to generate the software bill of materials(SBOM) for istio.
Expand All @@ -72,3 +68,26 @@ func GenerateBillOfMaterials(manifest model.Manifest) error {
}
return nil
}

// DockerTarPaths construct all the docker image tarball names as bom currently
// cannot accept directory as input
func DockerTarPaths(dockerDir string) []string {
var dockerImages []string
err := filepath.Walk(dockerDir, func(path string, fi os.FileInfo, err error) error {
if err != nil {
return err
}
if fi == nil {
return fmt.Errorf("failed to get fileinfo for file at path %s", path)
}
if fi.IsDir() {
return nil
}
dockerImages = append(dockerImages, path)
return nil
})
if err != nil {
return nil
}
return dockerImages
}
1 change: 1 addition & 0 deletions pkg/manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ func InputManifestToManifest(in model.InputManifest) (model.Manifest, error) {
ProxyOverride: in.ProxyOverride,
GrafanaDashboards: in.GrafanaDashboards,
SkipGenerateBillOfMaterials: in.SkipGenerateBillOfMaterials,
BillOfMaterialsURI: in.BillOfMaterialsURI,
Architectures: arch,
}, nil
}
Expand Down
14 changes: 9 additions & 5 deletions pkg/model/model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ const (
DockerOutputContext DockerOutput = "context"
)

// Manifest defines what is in a release
// InputManifest defines what is in a release
type InputManifest struct {
// Dependencies declares all git repositories used to build this release
Dependencies IstioDependencies `json:"dependencies"`
Expand All @@ -151,9 +151,11 @@ type InputManifest struct {
BuildOutputs []string `json:"outputs"`
// GrafanaDashboards defines a mapping of dashboard name -> ID of the dashboard on grafana.com
GrafanaDashboards map[string]int `json:"dashboards"`
// BillOfMaterials flag determines if a Bill of Materials should be produced
// by the build.
// SkipGenerateBillOfMaterials flag determines if a Bill of Materials should
// be produced by the build.
SkipGenerateBillOfMaterials bool `json:"skipGenerateBillOfMaterials"`
// BillOfMaterialsURI flag sets URI for bill of materials.
BillOfMaterialsURI string `json:"billOfMaterialsURI"`
}

// Manifest defines what is in a release
Expand Down Expand Up @@ -181,9 +183,11 @@ type Manifest struct {
// GrafanaDashboards defines a mapping of dashboard name -> ID of the dashboard on grafana.com
// Note: this tool is not yet smart enough to create dashboards that do not already exist, it can only update dashboards.
GrafanaDashboards map[string]int `json:"dashboards"`
// BillOfMaterials flag determines if a Bill of Materials should be produced
// by the build.
// SkipGenerateBillOfMaterials flag determines if a Bill of Materials should
// be produced by the build.
SkipGenerateBillOfMaterials bool `json:"skipGenerateBillOfMaterials"`
// BillOfMaterialsURI flag sets URI for bill of materials.
BillOfMaterialsURI string `json:"billOfMaterialsURI"`
}

// RepoDir is a helper to return the working directory for a repo
Expand Down
2 changes: 2 additions & 0 deletions release/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ fi

PRERELEASE_DOCKER_HUB=${PRERELEASE_DOCKER_HUB:-gcr.io/istio-prerelease-testing}
GCS_BUCKET=${GCS_BUCKET:-istio-prerelease/prerelease}
SBOM_OUTPUT_URI="https://storage.googleapis.com/${GCS_BUCKET}/releases"
Copy link

Choose a reason for hiding this comment

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

Do we want this value to be overwrite able as it isn't as written here? If not, then we could include it in the URI below and save a line.

HELM_BUCKET=${HELM_BUCKET:-istio-prerelease/charts}
COSIGN_KEY=${COSIGN_KEY:-}
GITHUB_ORG=${GITHUB_ORG:-istio}
Expand All @@ -62,6 +63,7 @@ version: "${VERSION}"
docker: "${DOCKER_HUB}"
directory: "${WORK_DIR}"
architectures: ${ARCHS}
billOfMaterialsURI: ${SBOM_OUTPUT_URI}
dependencies:
${DEPENDENCIES:-$(cat <<EOD
istio:
Expand Down