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

Jump to bottom

fix: #161: replace prints with logs #772

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

fix: #161: replace prints with logs #772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d6f5ebb
replace fmt.Printf(os.Stderr, ...), with slog.Error and slog.Info
ramonpetgrave64 May 17, 2024
7acf187
replace fmt.Sprintf("...", ...) with slog.Info
ramonpetgrave64 May 17, 2024
af32d96
lint: unnecessary fmt.sprintf
ramonpetgrave64 May 17, 2024
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
3 changes: 2 additions & 1 deletion cli/experimental/service/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package main
import (
"fmt"
"log"
"log/slog"
"net/http"
"time"

Expand All @@ -19,7 +20,7 @@ func main() {
http.Handle("/", r)

address := ":8000"
fmt.Printf("Starting HTTP server on %v ...\n", address)
slog.Info(fmt.Sprintf("Starting HTTP server on %v ...\n", address))
srv := &http.Server{
Handler: r,
Addr: address,
Expand Down
5 changes: 3 additions & 2 deletions cli/slsa-verifier/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package main
import (
"errors"
"fmt"
"log/slog"
"os"

"github.com/slsa-framework/slsa-verifier/v2/options"
Expand All @@ -12,14 +13,14 @@ import (

func check(err error) {
if err != nil {
fmt.Fprintln(os.Stderr, err)
slog.Error(fmt.Sprint(err))
os.Exit(1)
}
}

func envWarnings() {
if options.TestingEnabled() {
fmt.Fprintf(os.Stderr, "WARNING: Insecure SLSA_VERIFIER_TESTING is enabled.\n")
slog.Warn("WARNING: Insecure SLSA_VERIFIER_TESTING is enabled.\n")
}
}

Expand Down
21 changes: 11 additions & 10 deletions cli/slsa-verifier/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package main
import (
"errors"
"fmt"
"log/slog"
"os"

"github.com/slsa-framework/slsa-verifier/v2/cli/slsa-verifier/verify"
Expand Down Expand Up @@ -61,10 +62,10 @@ func verifyArtifactCmd() *cobra.Command {
}

if _, err := v.Exec(cmd.Context(), args); err != nil {
fmt.Fprintf(os.Stderr, "%s: %v\n", FAILURE, err)
slog.Error(fmt.Sprintf("%s: %v\n", FAILURE, err))
os.Exit(1)
} else {
fmt.Fprintf(os.Stderr, "%s\n", SUCCESS)
slog.Error(fmt.Sprintf("%s\n", SUCCESS))
}
},
}
Expand Down Expand Up @@ -113,10 +114,10 @@ func verifyImageCmd() *cobra.Command {
}

if _, err := v.Exec(cmd.Context(), args); err != nil {
fmt.Fprintf(os.Stderr, "%s: %v\n", FAILURE, err)
slog.Error(fmt.Sprintf("%s: %v\n", FAILURE, err))
os.Exit(1)
} else {
fmt.Fprintf(os.Stderr, "%s\n", SUCCESS)
slog.Info(fmt.Sprintf("%s\n", SUCCESS))
}
},
}
Expand Down Expand Up @@ -153,30 +154,30 @@ func verifyNpmPackageCmd() *cobra.Command {
v.PackageVersion = &o.PackageVersion
}
if cmd.Flags().Changed("source-branch") {
fmt.Fprintf(os.Stderr, "%s: --source-branch not supported\n", FAILURE)
slog.Error(fmt.Sprintf("%s: --source-branch not supported\n", FAILURE))
os.Exit(1)
}
if cmd.Flags().Changed("source-tag") {
fmt.Fprintf(os.Stderr, "%s: --source-tag not supported\n", FAILURE)
slog.Error(fmt.Sprintf("%s: --source-tag not supported\n", FAILURE))
os.Exit(1)
}
if cmd.Flags().Changed("source-versioned-tag") {
fmt.Fprintf(os.Stderr, "%s: --source-versioned-tag not supported\n", FAILURE)
slog.Error(fmt.Sprintf("%s: --source-versioned-tag not supported\n", FAILURE))
os.Exit(1)
}
if cmd.Flags().Changed("print-provenance") {
fmt.Fprintf(os.Stderr, "%s: --print-provenance not supported\n", FAILURE)
slog.Error(fmt.Sprintf("%s: --print-provenance not supported\n", FAILURE))
os.Exit(1)
}
if cmd.Flags().Changed("builder-id") {
v.BuilderID = &o.BuilderID
}

if _, err := v.Exec(cmd.Context(), args); err != nil {
fmt.Fprintf(os.Stderr, "%s: %v\n", FAILURE, err)
slog.Error(fmt.Sprintf("%s: %v\n", FAILURE, err))
os.Exit(1)
} else {
fmt.Fprintf(os.Stderr, "%s\n", SUCCESS)
slog.Info(fmt.Sprintf("%s\n", SUCCESS))
}
},
}
Expand Down
11 changes: 6 additions & 5 deletions cli/slsa-verifier/verify/verify_artifact.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"crypto/sha256"
"fmt"
"log/slog"
"os"

"github.com/slsa-framework/slsa-verifier/v2/options"
Expand All @@ -43,7 +44,7 @@ func (c *VerifyArtifactCommand) Exec(ctx context.Context, artifacts []string) (*
for _, artifact := range artifacts {
artifactHash, err := computeFileHash(artifact, sha256.New())
if err != nil {
fmt.Fprintf(os.Stderr, "Verifying artifact %s: FAILED: %v\n\n", artifact, err)
slog.Error(fmt.Sprintf("Verifying artifact %s: FAILED: %v\n\n", artifact, err))
return nil, err
}

Expand All @@ -62,13 +63,13 @@ func (c *VerifyArtifactCommand) Exec(ctx context.Context, artifacts []string) (*

provenance, err := os.ReadFile(c.ProvenancePath)
if err != nil {
fmt.Fprintf(os.Stderr, "Verifying artifact %s: FAILED: %v\n\n", artifact, err)
slog.Error(fmt.Sprintf("Verifying artifact %s: FAILED: %v\n\n", artifact, err))
return nil, err
}

verifiedProvenance, outBuilderID, err := verifiers.VerifyArtifact(ctx, provenance, artifactHash, provenanceOpts, builderOpts)
if err != nil {
fmt.Fprintf(os.Stderr, "Verifying artifact %s: FAILED: %v\n\n", artifact, err)
slog.Error(fmt.Sprintf("Verifying artifact %s: FAILED: %v\n\n", artifact, err))
return nil, err
}

Expand All @@ -80,10 +81,10 @@ func (c *VerifyArtifactCommand) Exec(ctx context.Context, artifacts []string) (*
builderID = outBuilderID
} else if *builderID != *outBuilderID {
err := fmt.Errorf("encountered different builderIDs %v %v", builderID, outBuilderID)
fmt.Fprintf(os.Stderr, "Verifying artifact %s: FAILED: %v\n\n", artifact, err)
slog.Error(fmt.Sprintf("Verifying artifact %s: FAILED: %v\n\n", artifact, err))
return nil, err
}
fmt.Fprintf(os.Stderr, "Verifying artifact %s: PASSED\n\n", artifact)
slog.Error(fmt.Sprintf("Verifying artifact %s: PASSED\n\n", artifact))
}

return builderID, nil
Expand Down
13 changes: 7 additions & 6 deletions cli/slsa-verifier/verify/verify_npm_package.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"crypto/sha512"
"errors"
"fmt"
"log/slog"
"os"

"github.com/slsa-framework/slsa-verifier/v2/options"
Expand All @@ -43,18 +44,18 @@ func (c *VerifyNpmPackageCommand) Exec(ctx context.Context, tarballs []string) (
var builderID *utils.TrustedBuilderID
if !options.ExperimentalEnabled() {
err := errors.New("feature support is only provided in SLSA_VERIFIER_EXPERIMENTAL mode")
fmt.Fprintf(os.Stderr, "Verifying npm package: FAILED: %v\n\n", err)
slog.Error(fmt.Sprintf("Verifying npm package: FAILED: %v\n\n", err))
return nil, err
}
for _, tarball := range tarballs {
tarballHash, err := computeFileHash(tarball, sha512.New())
if err != nil {
fmt.Fprintf(os.Stderr, "Verifying npm package %s: FAILED: %v\n\n", tarball, err)
slog.Error(fmt.Sprintf("Verifying npm package %s: FAILED: %v\n\n", tarball, err))
return nil, err
}

if c.AttestationsPath == "" {
fmt.Fprintf(os.Stderr, "--attestations-path is required.\n\n")
slog.Error("--attestations-path is required.\n\n")
return nil, err
}
provenanceOpts := &options.ProvenanceOpts{
Expand All @@ -74,13 +75,13 @@ func (c *VerifyNpmPackageCommand) Exec(ctx context.Context, tarballs []string) (

attestations, err := os.ReadFile(c.AttestationsPath)
if err != nil {
fmt.Fprintf(os.Stderr, "Verifying npm package %s: FAILED: %v\n\n", tarball, err)
slog.Error(fmt.Sprintf("Verifying npm package %s: FAILED: %v\n\n", tarball, err))
return nil, err
}

verifiedProvenance, outBuilderID, err := verifiers.VerifyNpmPackage(ctx, attestations, tarballHash, provenanceOpts, builderOpts)
if err != nil {
fmt.Fprintf(os.Stderr, "Verifying npm package %s: FAILED: %v\n\n", tarball, err)
slog.Error(fmt.Sprintf("Verifying npm package %s: FAILED: %v\n\n", tarball, err))
return nil, err
}

Expand All @@ -89,7 +90,7 @@ func (c *VerifyNpmPackageCommand) Exec(ctx context.Context, tarballs []string) (
}

builderID = outBuilderID
fmt.Fprintf(os.Stderr, "Verifying npm package %s: PASSED\n\n", tarball)
Copy link
Contributor

Choose a reason for hiding this comment

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

where does slog write the result? We need to ensure compatibility and ensure it's written to stderr when fmt.Fprintf(os.Stderr. Otherwise folks won't be able to pipe the result into jq or other tools: jq will pick not just the printed provenance, but the other messages too.

I think before changing this API we should define what the interface is for our logger. And we should provide an interface as logger, so that callers can customize how they log. Here's a fast logging library https://github.com/uber-go/zap, among others, that would need to be instantiable by callers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I confirmed that by default slog will send to stderr. And then I removed the changes against fmt.Fprintf(os.Stdout, ... statements.

I'll look into how to best allow caller-provided loggers.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I'm personally in favor of using the stdlib logging packages rather than including a dependency.

slog.Info(fmt.Sprintf("Verifying npm package %s: PASSED\n\n", tarball))
}

return builderID, nil
Expand Down
12 changes: 6 additions & 6 deletions verifiers/internal/gcb/provenance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"crypto/sha256"
"encoding/json"
"fmt"
"os"
"log/slog"
"reflect"
"strings"

Expand Down Expand Up @@ -452,10 +452,10 @@ func (p *Provenance) VerifySourceURI(expectedSourceURI string, builderID utils.T

// The build was not configured with a GitHub trigger. Warn.
if strings.HasPrefix(uri, "gs://") {
fmt.Fprintf(os.Stderr, `This build was not configured with a GitHub trigger `+
`and will not match on an expected, version controlled source URI. `+
`See Cloud Build's documentation on building repositories from GitHub: `+
`https://cloud.google.com/build/docs/automating-builds/github/build-repos-from-github`)
slog.Warn(fmt.Sprintf(`This build was not configured with a GitHub trigger ` +
`and will not match on an expected, version controlled source URI. ` +
`See Cloud Build's documentation on building repositories from GitHub: ` +
`https://cloud.google.com/build/docs/automating-builds/github/build-repos-from-github`))
}

predicateType, err := statement.PredicateType()
Expand Down Expand Up @@ -607,7 +607,7 @@ func (p *Provenance) verifySignatures(prov *provenance) error {

p.verifiedStatement = stmt
p.verifiedProvenance = prov
fmt.Fprintf(os.Stderr, "Verification succeeded with key %q\n", keyName)
slog.Info(fmt.Sprintf("Verification succeeded with key %q\n", keyName))
return nil
}

Expand Down
4 changes: 2 additions & 2 deletions verifiers/internal/gha/provenance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"crypto/x509"
"encoding/json"
"fmt"
"os"
"log/slog"
"strings"

dsselib "github.com/secure-systems-lab/go-securesystemslib/dsse"
Expand Down Expand Up @@ -222,7 +222,7 @@ func VerifyProvenanceSignature(ctx context.Context, trustedRoot *TrustedRoot,
}

// Fallback on using the redis search index to get matching UUIDs.
fmt.Fprintf(os.Stderr, "No certificate provided, trying Redis search index to find entries by subject digest\n")
slog.Warn("No certificate provided, trying Redis search index to find entries by subject digest\n")

// Verify the provenance and return the signing certificate.
return SearchValidSignedAttestation(ctx, artifactHash,
Expand Down
6 changes: 3 additions & 3 deletions verifiers/internal/gha/rekor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"encoding/base64"
"errors"
"fmt"
"os"
"log/slog"
"strings"
"time"

Expand Down Expand Up @@ -242,7 +242,7 @@ func GetValidSignedAttestationWithCert(rClient *client.Rekor,
}
rekorEntry = e
url := fmt.Sprintf("%v/%v/%v", defaultRekorAddr, "api/v1/log/entries", uuid)
fmt.Fprintf(os.Stderr, "Verified signature against tlog entry index %d at URL: %s\n", *e.LogIndex, url)
slog.Info(fmt.Sprintf("Verified signature against tlog entry index %d at URL: %s\n", *e.LogIndex, url))
}

certs, err := cryptoutils.UnmarshalCertificatesFromPEM(certPem)
Expand Down Expand Up @@ -326,7 +326,7 @@ func SearchValidSignedAttestation(ctx context.Context, artifactHash string, prov

// success!
url := fmt.Sprintf("%v/%v/%v", defaultRekorAddr, "api/v1/log/entries", uuid)
fmt.Fprintf(os.Stderr, "Verified signature against tlog entry index %d at URL: %s\n", *entry.LogIndex, url)
slog.Info(fmt.Sprintf("Verified signature against tlog entry index %d at URL: %s\n", *entry.LogIndex, url))
return proposedSignedAtt, nil
}

Expand Down
16 changes: 8 additions & 8 deletions verifiers/internal/gha/verifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"encoding/base64"
"errors"
"fmt"
"os"
"log/slog"
"strings"

"github.com/google/go-containerregistry/pkg/name"
Expand Down Expand Up @@ -86,9 +86,9 @@ func verifyEnvAndCert(env *dsse.Envelope,
}
}

fmt.Fprintf(os.Stderr, "Verified build using builder %q at commit %s\n",
slog.Info(fmt.Sprintf("Verified build using builder %q at commit %s\n",
verifiedBuilderID.String(),
workflowInfo.SourceSha1)
workflowInfo.SourceSha1))

// Return verified provenance.
r, err := base64.StdEncoding.DecodeString(env.Payload)
Expand Down Expand Up @@ -201,9 +201,9 @@ func verifyNpmEnvAndCert(env *dsse.Envelope,
return nil, err
}

fmt.Fprintf(os.Stderr, "Verified build using builder %s at commit %s\n",
slog.Info(fmt.Sprintf("Verified build using builder %s at commit %s\n",
trustedBuilderID.String(),
workflowInfo.SourceSha1)
workflowInfo.SourceSha1))

return trustedBuilderID, nil
}
Expand Down Expand Up @@ -293,17 +293,17 @@ func (v *GHAVerifier) VerifyImage(ctx context.Context,
for _, att := range atts {
pyld, err := att.Payload()
if err != nil {
fmt.Fprintf(os.Stderr, "unexpected error getting payload from OCI registry %s", err)
slog.Error(fmt.Sprintf("unexpected error getting payload from OCI registry %s", err))
continue
}
env, err := EnvelopeFromBytes(pyld)
if err != nil {
fmt.Fprintf(os.Stderr, "unexpected error parsing envelope from OCI registry %s", err)
slog.Error(fmt.Sprintf("unexpected error parsing envelope from OCI registry %s", err))
continue
}
cert, err := att.Cert()
if err != nil {
fmt.Fprintf(os.Stderr, "unexpected error getting certificate from OCI registry %s", err)
slog.Error(fmt.Sprintf("unexpected error getting certificate from OCI registry %s", err))
continue
}
verifiedProvenance, builderID, err = verifyEnvAndCert(env,
Expand Down
Loading