Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions cmd/kosli/attest.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ func newAttestCmd(out io.Writer) *cobra.Command {
newAttestPRCmd(out),
newAttestSonarCmd(out),
newAttestCustomCmd(out),
newAttestSbomCmd(out),
Comment thread
AlexKantor87 marked this conversation as resolved.
newAttestOverrideCmd(out),
newAttestDecisionCmd(out),
)
Expand Down
292 changes: 292 additions & 0 deletions cmd/kosli/attestSbom.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,292 @@
package main

import (
"crypto/sha256"
"fmt"
"io"
"net/http"
"net/url"
"os"

"github.com/kosli-dev/cli/internal/requests"
"github.com/kosli-dev/cli/internal/sbom"
"github.com/spf13/cobra"
)

// The API rejects a request body over 10MB, and the JSON payload is counted
// alongside the file, so this leaves room for it rather than sitting on the
// limit. Lifting the ceiling needs direct-to-S3 upload, tracked separately.
const maxSbomFileBytes = 9 * 1024 * 1024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The 1MB of headroom is sized against "the JSON payload", but the JSON payload is not bounded: --user-data embeds an arbitrary JSON file into the same body (payload.UserData, err = LoadJsonData(o.userDataFilePath), attestation.go:104), and --annotate, --external-url and the commit info ride along with it.

So --sbom-file 9mb.json --user-data 2mb.json passes this guard and still comes back as the bare 413 the guard exists to avoid — the one case where the friendlier local error is most wanted, since the user now has two files to suspect.

Not necessarily worth code: the fix would be measuring the marshalled payload after building it, which is a different check in a different place. But the comment currently reads as if the 10MB limit is accounted for, and it is only accounted for on the file side. One clause ("…the JSON payload, whose size --user-data can raise past this margin") would stop the next reader trusting it further than it goes.

Comment on lines +16 to +19

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worth noting the one way this ceiling still doesn't hold: --user-data is arbitrary JSON loaded straight into the payload (LoadJsonData, via CommonAttestationOptions.run), and it rides in the same request body. A 9MB SBOM plus a 2MB user-data file is a 10MB+ POST, so the user gets the bare 413 this guard exists to pre-empt.

Not worth a second guard — the 1MB of headroom is the right shape for the common case, and the combination is rare. But the comment currently reads as if the payload is a known small constant, which is what makes the number look safe. A clause saying the headroom assumes a small --user-data would stop the next reader from trusting the bound further than it goes.


// Both are also carried inside attestation_data, where the server's schema can
// enforce them. These are the copy a reader sees on the trail page.
const (
sbomFormatAnnotation = "sbom_format"
sbomSha256Annotation = "sbom_sha256"
)

type SbomAttestationData struct {
Format string `json:"format"`
OriginalFingerprint string `json:"original_fingerprint"`
Document *sbom.Document `json:"document"`
}

type SbomAttestationPayload struct {
*CommonAttestationPayload
TypeName string `json:"type_name"`
AttestationData SbomAttestationData `json:"attestation_data"`
}

type attestSbomOptions struct {
*CommonAttestationOptions
sbomFilePath string
payload SbomAttestationPayload
}

const attestSbomShortDesc = `Report a software bill of materials to an artifact or a trail in a Kosli flow. `

const attestSbomLongDesc = attestSbomShortDesc + `
The SBOM file is given with the ^--sbom-file^ flag. CycloneDX (JSON and XML) and
SPDX (JSON and tag-value) are supported.

The file is uploaded as it is, so the recorded checksum is the checksum of the
file you supplied and you can verify it by hand. It must be a single file: it is
not compressed, and an already-compressed file is rejected, because the format
and the summary below are read from it.
Comment on lines +52 to +55

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"an already-compressed file is rejected" over-promises slightly: only gzip is detected, by magic bytes (internal/sbom/sbom.go:85). A .zip, .xz or .bz2 SBOM is also refused, but via the generic unrecognised file: expected a CycloneDX or SPDX SBOM in JSON, XML or tag-value form, which doesn't tell the user that decompressing is the fix — and the help text has just told them compression is the reason.

Naming gzip keeps the sentence true to what the code recognises:

Suggested change
The file is uploaded as it is, so the recorded checksum is the checksum of the
file you supplied and you can verify it by hand. It must be a single file: it is
not compressed, and an already-compressed file is rejected, because the format
and the summary below are read from it.
The file is uploaded as it is, so the recorded checksum is the checksum of the
file you supplied and you can verify it by hand. It must be a single file: it is
not compressed, and a gzipped file is rejected, because the format
and the summary below are read from it.


Kosli reads the format, the creation time, the tools that produced it, the
subject it describes and how many packages it lists. Nothing is checked against
the artifact; the SBOM is recorded as reported.

^--attachments^ cannot be used with this command, for the same reason: a second
attachment would trigger compression.

The format and the file checksum are also added as the ^sbom_format^ and
^sbom_sha256^ annotations.
` + attestationBindingDesc + `

` + kosliIgnoreDesc + `

` + commitDescription

const attestSbomExample = `
# report an SBOM about a pre-built docker artifact (kosli finds the fingerprint):
kosli attest sbom yourDockerImageName \
--artifact-type docker \
--name yourAttestationName \
--sbom-file yourSbomPath \
--flow yourFlowName \
--trail yourTrailName \
--api-token yourAPIToken \
--org yourOrgName

# report an SBOM about a trail:
kosli attest sbom \
--name yourAttestationName \
--sbom-file yourSbomPath \
--flow yourFlowName \
--trail yourTrailName \
--api-token yourAPIToken \
--org yourOrgName
`

func newAttestSbomCmd(out io.Writer) *cobra.Command {
o := &attestSbomOptions{
CommonAttestationOptions: &CommonAttestationOptions{
fingerprintOptions: &fingerprintOptions{},
},
payload: SbomAttestationPayload{
CommonAttestationPayload: &CommonAttestationPayload{},
TypeName: "sbom",
},
}
cmd := &cobra.Command{
// Args: cobra.MaximumNArgs(1), // See CustomMaximumNArgs() below
Use: "sbom [IMAGE-NAME | FILE-PATH | DIR-PATH]",
Short: attestSbomShortDesc,
Long: attestSbomLongDesc,
Example: attestSbomExample,
Annotations: map[string]string{betaCLIAnnotation: ""},
PreRunE: func(cmd *cobra.Command, args []string) error {
err := CustomMaximumNArgs(1, args)
if err != nil {
return err
}

err = RequireGlobalFlags(global, []string{"Org", "ApiToken"})
if err != nil {
return ErrorBeforePrintingUsage(cmd, err.Error())
}

err = MuXRequiredFlags(cmd, []string{"fingerprint", "artifact-type"}, false)
if err != nil {
return err
}

// One file per SBOM attestation. Two or more attachments are tarred
// and gzipped before upload, which would compress the SBOM and break
// the checksum recorded against it.
err = MuXRequiredFlags(cmd, []string{"sbom-file", "attachments"}, false)
if err != nil {
return err
}
Comment thread
AlexKantor87 marked this conversation as resolved.

err = o.rejectReservedAnnotations()
if err != nil {
return err
}

err = ValidateSliceValues(o.redactedCommitInfo, allowedCommitRedactionValues)
if err != nil {
return fmt.Errorf("%s for --redact-commit-info", err.Error())
}

err = ValidateAttestationArtifactArg(args, o.fingerprintOptions.artifactType, o.payload.ArtifactFingerprint)
if err != nil {
return ErrorBeforePrintingUsage(cmd, err.Error())
}

return ValidateRegistryFlags(cmd, o.fingerprintOptions)
},

RunE: func(cmd *cobra.Command, args []string) error {
o.repoURLExplicit = cmd.Flags().Changed("repo-url")
o.repoNameExplicit = cmd.Flags().Changed("repository")
return o.run(args)
},
}

ci := WhichCI()
addAttestationFlags(cmd, o.CommonAttestationOptions, o.payload.CommonAttestationPayload, ci)
cmd.Flags().StringVar(&o.sbomFilePath, "sbom-file", "", attestationSbomFileFlag)

err := RequireFlags(cmd, []string{"flow", "trail", "name", "sbom-file"})
if err != nil {
logger.Error("failed to configure required flags: %v", err)
}

return cmd
}

func (o *attestSbomOptions) run(args []string) error {
// The slug is the attestation family, not the type. The server tells them
// apart by type_name in the body.
url, err := url.JoinPath(global.Host, "api/v2/attestations", global.Org, o.flowName, "trail", o.trailName, "system")
if err != nil {
return err
}

err = o.CommonAttestationOptions.run(args, o.payload.CommonAttestationPayload)
if err != nil {
return err
}

err = o.loadSbom()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The reserved-annotation check moved to PreRunE so a typo would not cost a repo walk and a pass over the file. The file's own cheap checks still sit behind the most expensive thing this command does:

CommonAttestationOptions.runGetSha256Digest(args[0], …) (attestation.go:75-80), which for --artifact-type docker can pull and fingerprint an image. So

kosli attest sbom myimage --artifact-type docker --sbom-file typo.json …

pulls the image, then fails with failed to read SBOM file [typo.json]: no such file or directory. Same for the directory and oversize cases — the three errors that are pure local input.

Worth knowing before reordering, and worth a comment either way: loadSbom cannot simply move above line 179. annotate writes into o.payload.Annotations, and CommonAttestationOptions.run finishes by replacing that field wholesale (payload.Annotations, err = processAnnotations(o.annotations), attestation.go:116), so a hoisted loadSbom would have both annotations silently discarded. The nil-guard inside annotate makes it read as order-independent when it is not.

Two ways out, both small:

  • split the cheap guards out (open/Stat/IsDir/IsRegular, returning the open handle) and call that from PreRunE, leaving the read and annotate where they are; or
  • leave the order and say in a comment on annotate that it must run after processAnnotations.

The dry-run goldens would catch the dropped annotations, so this is about not setting the trap rather than about an undetected break.

if err != nil {
return err
}
o.attachments = append(o.attachments, o.sbomFilePath)
Comment thread
AlexKantor87 marked this conversation as resolved.

form, cleanupNeeded, evidencePath, err := prepareAttestationForm(o.payload, o.attachments)
if err != nil {
return err
}
// if we created a tar package, remove it after uploading it
if cleanupNeeded {
defer func() {
if err := os.Remove(evidencePath); err != nil {
logger.Warn("failed to remove evidence file: %v", err)
}
}()
}

reqParams := &requests.RequestParams{
Method: http.MethodPost,
URL: url,
Form: form,
DryRun: global.DryRun,
Token: global.ApiToken,
}
_, err = kosliClient.Do(reqParams)
if err == nil && !global.DryRun {
logger.Info("sbom attestation '%s' is reported to trail: %s", o.payload.AttestationName, o.trailName)
}
return wrapAttestationError(err)
}

func (o *attestSbomOptions) loadSbom() error {
file, err := os.Open(o.sbomFilePath)
if err != nil {
return fmt.Errorf("failed to read SBOM file [%s]: %s", o.sbomFilePath, err)
}
defer func() { _ = file.Close() }()

// Stat the open handle rather than the path, so the bytes measured are the
// bytes about to be read: a file still being written, or a symlink
// repointed in between, would otherwise walk past a stat on the path.
info, err := file.Stat()
if err != nil {
return fmt.Errorf("failed to read SBOM file [%s]: %s", o.sbomFilePath, err)
}
// A directory is what a user hits when tab-completion stops a path short,
// so it gets its own advice. Reading one fails with a message about file
// descriptors, and a fifo blocks until something writes to it.
if info.IsDir() {
return fmt.Errorf("SBOM file [%s] is a directory; supply the SBOM file itself", o.sbomFilePath)
}
if !info.Mode().IsRegular() {
return fmt.Errorf("SBOM file [%s] is not a regular file", o.sbomFilePath)
Comment thread
AlexKantor87 marked this conversation as resolved.
}
// One read for both, so the recorded fingerprint always describes the bytes
// the recorded summary was taken from. Reading one byte past the limit is
// what makes the limit a bound on memory rather than a claim about a size
// measured earlier: a file a build is still writing grows after the stat.
content, err := io.ReadAll(io.LimitReader(file, maxSbomFileBytes+1))
if err != nil {
return fmt.Errorf("failed to read SBOM file [%s]: %s", o.sbomFilePath, err)
}
if int64(len(content)) > maxSbomFileBytes {
return fmt.Errorf(
"SBOM file [%s] is above the %d byte limit for an SBOM attestation",
o.sbomFilePath, maxSbomFileBytes,
)
}
fingerprint := fmt.Sprintf("%x", sha256.Sum256(content))

data, err := sbom.ProcessSBOM(content)
if err != nil {
return fmt.Errorf("failed to parse SBOM file [%s]: %s", o.sbomFilePath, err)
}
Comment thread
AlexKantor87 marked this conversation as resolved.

o.payload.AttestationData = SbomAttestationData{
Format: data.Format,
OriginalFingerprint: fingerprint,
Document: data.Document,
}
o.annotate(data.Format, fingerprint)
return nil
}

// rejectReservedAnnotations runs in PreRunE: it needs nothing from the file, so
// a typo should not cost a repository walk and a pass over nine megabytes first.
func (o *attestSbomOptions) rejectReservedAnnotations() error {
for _, reserved := range []string{sbomFormatAnnotation, sbomSha256Annotation} {
if _, taken := o.annotations[reserved]; taken {
return fmt.Errorf(
"annotation key '%s' is set by this command from the SBOM file and cannot be provided with --annotate",
reserved,
)
}
}
return nil
}

// annotate records what the file said about itself where a reader sees it on
// the trail page. The same two values are carried inside attestation_data.
func (o *attestSbomOptions) annotate(format, fingerprint string) {
if o.payload.Annotations == nil {
o.payload.Annotations = map[string]string{}
}
o.payload.Annotations[sbomFormatAnnotation] = format
o.payload.Annotations[sbomSha256Annotation] = fingerprint
}
Loading
Loading