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.

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

sign: Readability cleanups #41

Merged
merged 4 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
Pass ctx idiomatically
It shouldn't be passed through an option pattern, since that causes it to be stored in a struct. It should be passed as the first arg. See https://pkg.go.dev/context.

This adds a ctx arg to Verify, which uses it, and Sign, which doesn't, but now looks weird without it.
  • Loading branch information
DrJosh9000 committed Jun 26, 2024
commit 5cb116bd779d47578a68a2956b1dd790368eeafc
11 changes: 3 additions & 8 deletions signature/sign.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ type options struct {
env map[string]string
logger Logger
debugSigning bool
ctx context.Context
}

type Option interface {
Expand All @@ -54,22 +53,18 @@ type Option interface {
type envOption struct{ env map[string]string }
type loggerOption struct{ logger Logger }
type debugSigningOption struct{ debugSigning bool }
type contextOption struct{ ctx context.Context }

func (o envOption) apply(opts *options) { opts.env = o.env }
func (o loggerOption) apply(opts *options) { opts.logger = o.logger }
func (o debugSigningOption) apply(opts *options) { opts.debugSigning = o.debugSigning }
func (o contextOption) apply(opts *options) { opts.ctx = o.ctx }

func WithEnv(env map[string]string) Option { return envOption{env} }
func WithLogger(logger Logger) Option { return loggerOption{logger} }
func WithDebugSigning(debugSigning bool) Option { return debugSigningOption{debugSigning} }
func WithContext(ctx context.Context) Option { return contextOption{ctx} }

func configureOptions(opts ...Option) options {
options := options{
env: make(map[string]string),
ctx: context.Background(),
}
for _, o := range opts {
o.apply(&options)
Expand All @@ -79,7 +74,7 @@ func configureOptions(opts ...Option) options {

// Sign computes a new signature for an environment (env) combined with an
// object containing values (sf) using a given key.
func Sign(key jwk.Key, sf SignedFielder, opts ...Option) (*pipeline.Signature, error) {
func Sign(_ context.Context, key jwk.Key, sf SignedFielder, opts ...Option) (*pipeline.Signature, error) {
options := configureOptions(opts...)

values, err := sf.SignedFields()
Expand Down Expand Up @@ -150,7 +145,7 @@ func Sign(key jwk.Key, sf SignedFielder, opts ...Option) (*pipeline.Signature, e

// Verify verifies an existing signature against environment (env) combined with
// an object containing values (sf) using keys from a keySet.
func Verify(s *pipeline.Signature, keySet jwk.Set, sf SignedFielder, opts ...Option) error {
func Verify(ctx context.Context, s *pipeline.Signature, keySet jwk.Set, sf SignedFielder, opts ...Option) error {
options := configureOptions(opts...)

if len(s.SignedFields) == 0 {
Expand Down Expand Up @@ -190,7 +185,7 @@ func Verify(s *pipeline.Signature, keySet jwk.Set, sf SignedFielder, opts ...Opt
return err
}

for it := keySet.Keys(options.ctx); it.Next(options.ctx); {
for it := keySet.Keys(ctx); it.Next(ctx); {
pair := it.Pair()
publicKey := pair.Value.(jwk.Key)
fingerprint, err := publicKey.Thumbprint(crypto.SHA256)
Expand Down
48 changes: 31 additions & 17 deletions signature/sign_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package signature

import (
"context"
"errors"
"fmt"
"math/rand"
Expand All @@ -21,6 +22,9 @@ const (
)

func TestSignVerify(t *testing.T) {
t.Parallel()
ctx := context.Background()

step := &pipeline.CommandStep{
Command: "llamas",
Plugins: pipeline.Plugins{
Expand Down Expand Up @@ -96,7 +100,7 @@ func TestSignVerify(t *testing.T) {
t.Fatalf("jwkutil.LoadKey(%v, %v) error = %v", privPath, keyName, err)
}

sig, err := Sign(sKey, stepWithInvariants, WithEnv(signEnv))
sig, err := Sign(ctx, sKey, stepWithInvariants, WithEnv(signEnv))
if err != nil {
t.Fatalf("Sign(CommandStep, signer) error = %v", err)
}
Expand All @@ -122,8 +126,8 @@ func TestSignVerify(t *testing.T) {
t.Fatalf("verifier.AddKey(%v) error = %v", vKey, err)
}

if err := Verify(sig, verifier, stepWithInvariants, WithEnv(verifyEnv)); err != nil {
t.Errorf("Verify(sig,CommandStep, verifier) = %v", err)
if err := Verify(ctx, sig, verifier, stepWithInvariants, WithEnv(verifyEnv)); err != nil {
t.Errorf("Verify(sig, CommandStep, verifier) = %v", err)
}
})
}
Expand All @@ -147,6 +151,7 @@ func (m testFields) ValuesForFields(fields []string) (map[string]any, error) {

func TestSignConcatenatedFields(t *testing.T) {
t.Parallel()
ctx := context.Background()

// Tests that Sign is resilient to concatenation.
// Specifically, these maps should all have distinct "content". (If you
Expand Down Expand Up @@ -183,7 +188,7 @@ func TestSignConcatenatedFields(t *testing.T) {
}

for _, m := range maps {
sig, err := Sign(key, m)
sig, err := Sign(ctx, key, m)
if err != nil {
t.Fatalf("Sign(%v, pts) error = %v", m, err)
}
Expand All @@ -204,6 +209,7 @@ func TestSignConcatenatedFields(t *testing.T) {

func TestUnknownAlgorithm(t *testing.T) {
t.Parallel()
ctx := context.Background()

signer, _, err := jwkutil.NewSymmetricKeyPairFromString(keyID, "alpacas", jwa.HS256)
if err != nil {
Expand All @@ -217,16 +223,20 @@ func TestUnknownAlgorithm(t *testing.T) {

key.Set(jwk.AlgorithmKey, "rot13")

if _, err := Sign(
key,
&CommandStepWithInvariants{CommandStep: pipeline.CommandStep{Command: "llamas"}},
); err == nil {
step := &CommandStepWithInvariants{
CommandStep: pipeline.CommandStep{
Command: "llamas",
},
}

if _, err := Sign(ctx, key, step); err == nil {
t.Errorf("Sign(nil, CommandStep, signer) = %v, want non-nil error", err)
}
}

func TestVerifyBadSignature(t *testing.T) {
t.Parallel()
ctx := context.Background()

cs := &CommandStepWithInvariants{CommandStep: pipeline.CommandStep{Command: "llamas"}}

Expand All @@ -241,13 +251,14 @@ func TestVerifyBadSignature(t *testing.T) {
t.Fatalf("NewSymmetricKeyPairFromString(alpacas) error = %v", err)
}

if err := Verify(sig, verifier, cs); err == nil {
if err := Verify(ctx, sig, verifier, cs); err == nil {
t.Errorf("Verify(sig,CommandStep, alpacas) = %v, want non-nil error", err)
}
}

func TestSignUnknownStep(t *testing.T) {
t.Parallel()
ctx := context.Background()

steps := pipeline.Steps{
&pipeline.UnknownStep{
Expand All @@ -265,13 +276,14 @@ func TestSignUnknownStep(t *testing.T) {
t.Fatalf("signer.Key(0) = _, false, want true")
}

if err := SignSteps(steps, key, ""); !errors.Is(err, errSigningRefusedUnknownStepType) {
if err := SignSteps(ctx, steps, key, ""); !errors.Is(err, errSigningRefusedUnknownStepType) {
t.Errorf("steps.sign(signer) = %v, want %v", err, errSigningRefusedUnknownStepType)
}
}

func TestSignVerifyEnv(t *testing.T) {
t.Parallel()
ctx := context.Background()

cases := []struct {
name string
Expand Down Expand Up @@ -353,12 +365,12 @@ func TestSignVerifyEnv(t *testing.T) {
RepositoryURL: tc.repositoryURL,
}

sig, err := Sign(key, stepWithInvariants, WithEnv(tc.pipelineEnv))
sig, err := Sign(ctx, key, stepWithInvariants, WithEnv(tc.pipelineEnv))
if err != nil {
t.Fatalf("Sign(CommandStep, signer) error = %v", err)
}

if err := Verify(sig, verifier, stepWithInvariants, WithEnv(tc.verifyEnv)); err != nil {
if err := Verify(ctx, sig, verifier, stepWithInvariants, WithEnv(tc.verifyEnv)); err != nil {
t.Errorf("Verify(sig,CommandStep, verifier) = %v", err)
}
})
Expand All @@ -367,6 +379,7 @@ func TestSignVerifyEnv(t *testing.T) {

func TestSignatureStability(t *testing.T) {
t.Parallel()
ctx := context.Background()

// The idea here is to sign and verify a step that is likely to encode in a
// non-stable way if there are ordering bugs.
Expand Down Expand Up @@ -408,18 +421,19 @@ func TestSignatureStability(t *testing.T) {
t.Fatalf("signer.Key(0) = _, false, want true")
}

sig, err := Sign(key, stepWithInvariants, WithEnv(env))
sig, err := Sign(ctx, key, stepWithInvariants, WithEnv(env))
if err != nil {
t.Fatalf("Sign(env, CommandStep, signer) error = %v", err)
}

if err := Verify(sig, verifier, stepWithInvariants, WithEnv(env)); err != nil {
t.Errorf("Verify(sig,env, CommandStep, verifier) = %v", err)
if err := Verify(ctx, sig, verifier, stepWithInvariants, WithEnv(env)); err != nil {
t.Errorf("Verify(sig, env, CommandStep, verifier) = %v", err)
}
}

func TestDebugSigning(t *testing.T) {
t.Parallel()
ctx := context.Background()

step := &pipeline.CommandStep{
Command: "llamas",
Expand Down Expand Up @@ -469,7 +483,7 @@ func TestDebugSigning(t *testing.T) {
logger := &mockLogger{
expectedFormat: "Signed Step: %s",
}
_, err = Sign(sKey, stepWithInvariants, WithEnv(signEnv), WithDebugSigning(false), WithLogger(logger))
_, err = Sign(ctx, sKey, stepWithInvariants, WithEnv(signEnv), WithDebugSigning(false), WithLogger(logger))
if err != nil {
t.Fatalf("Sign(CommandStep, signer) error = %v", err)
}
Expand All @@ -482,7 +496,7 @@ func TestDebugSigning(t *testing.T) {
logger = &mockLogger{
expectedFormat: "Signed Step: %s",
}
_, err = Sign(sKey, stepWithInvariants, WithEnv(signEnv), WithDebugSigning(true), WithLogger(logger))
_, err = Sign(ctx, sKey, stepWithInvariants, WithEnv(signEnv), WithDebugSigning(true), WithLogger(logger))
if err != nil {
t.Fatalf("Sign(CommandStep, signer) error = %v", err)
}
Expand Down
11 changes: 6 additions & 5 deletions signature/steps.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package signature

import (
"context"
"errors"
"fmt"

Expand All @@ -12,7 +13,7 @@ var errSigningRefusedUnknownStepType = errors.New("refusing to sign pipeline con

// SignSteps adds signatures to each command step (and recursively to any command steps that are within group steps).
// The steps are mutated directly, so an error part-way through may leave some steps un-signed.
func SignSteps(s pipeline.Steps, key jwk.Key, repoURL string, opts ...Option) error {
func SignSteps(ctx context.Context, s pipeline.Steps, key jwk.Key, repoURL string, opts ...Option) error {
for _, step := range s {
switch step := step.(type) {
case *pipeline.CommandStep:
Expand All @@ -21,14 +22,14 @@ func SignSteps(s pipeline.Steps, key jwk.Key, repoURL string, opts ...Option) er
RepositoryURL: repoURL,
}

sig, err := Sign(key, stepWithInvariants, opts...)
sig, err := Sign(ctx, key, stepWithInvariants, opts...)
if err != nil {
return fmt.Errorf("signing step with command %q: %w", step.Command, err)
}
step.Signature = sig

case *pipeline.GroupStep:
if err := SignSteps(step.Steps, key, repoURL, opts...); err != nil {
if err := SignSteps(ctx, step.Steps, key, repoURL, opts...); err != nil {
return fmt.Errorf("signing group step: %w", err)
}

Expand All @@ -45,8 +46,8 @@ func SignSteps(s pipeline.Steps, key jwk.Key, repoURL string, opts ...Option) er
}

// SignPipeline adds signatures to each command step (and recursively to any command steps that are within group steps) within a pipeline
func SignPipeline(s pipeline.Steps, key jwk.Key, repo string, opts ...Option) error {
if err := SignSteps(s, key, repo, opts...); err != nil {
func SignPipeline(ctx context.Context, s pipeline.Steps, key jwk.Key, repo string, opts ...Option) error {
if err := SignSteps(ctx, s, key, repo, opts...); err != nil {
return fmt.Errorf("signing steps: %w", err)
}
return nil
Expand Down