Make nil verifier case a little bit clearer

buildkite · Aug 3, 2023 · c78f0de · c78f0de
1 parent f41fe60
commit c78f0de
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 37 deletions.
diff --git a/agent/run_job.go b/agent/run_job.go
@@ -79,35 +79,47 @@ func (r *JobRunner) Run(ctx context.Context) error {
 		r.cleanup(ctx, wg, exit)
 	}(ctx, &wg) // Note the non-cancellable context (ctx rather than cctx) here - we don't want to be interrupted during cleanup
 
-	ise := &invalidSignatureError{}
-	switch err := r.verifyJob(verifier); {
-	case err == nil: // no error, all good, keep going
-		if verifier != nil {
-			r.logger.Info("Successfully verified job %s with signature %s", r.conf.Job.ID, r.conf.Job.Step.Signature.Value)
-			r.logStreamer.Process([]byte(fmt.Sprintf("✅ Verified job with signature %s\n", r.conf.Job.Step.Signature.Value)))
-		}
+	job := r.conf.Job
 
-	case errors.Is(err, ErrNoSignature):
-		r.verificationFailureLogs(err, r.NoSignatureBehavior)
-		if r.NoSignatureBehavior == VerificationBehaviourBlock {
-			exit.Status = -1
-			exit.SignalReason = "job_verification_failed_no_signature"
-			return nil
-		}
+	if verifier == nil && job.Step.Signature != nil {
+		r.verificationFailureLogs(
+			fmt.Errorf("job %q was signed with signature %q, but no verification key was provided, so the job can't be verified", job.ID, job.Step.Signature.Value),
+			VerificationBehaviourBlock,
+		)
+		exit.Status = -1
+		exit.SignalReason = "job_verification_failed_with_error"
+		return nil
+	}
 
-	case errors.As(err, &ise):
-		r.verificationFailureLogs(err, r.InvalidSignatureBehavior)
-		if r.InvalidSignatureBehavior == VerificationBehaviourBlock {
+	if verifier != nil {
+		ise := &invalidSignatureError{}
+		switch err := r.verifyJob(verifier); {
+		case errors.Is(err, ErrNoSignature):
+			r.verificationFailureLogs(err, r.NoSignatureBehavior)
+			if r.NoSignatureBehavior == VerificationBehaviourBlock {
+				exit.Status = -1
+				exit.SignalReason = "job_verification_failed_no_signature"
+				return nil
+			}
+
+		case errors.As(err, &ise):
+			r.verificationFailureLogs(err, r.InvalidSignatureBehavior)
+			if r.InvalidSignatureBehavior == VerificationBehaviourBlock {
+				exit.Status = -1
+				exit.SignalReason = "job_verification_failed_invalid_signature"
+				return nil
+			}
+
+		case err != nil: // some other error
+			r.verificationFailureLogs(err, VerificationBehaviourBlock) // errors in verification are always fatal
 			exit.Status = -1
-			exit.SignalReason = "job_verification_failed_invalid_signature"
+			exit.SignalReason = "job_verification_failed_with_error"
 			return nil
-		}
 
-	default: // some other error
-		r.verificationFailureLogs(err, VerificationBehaviourBlock) // errors in verification are always fatal
-		exit.Status = -1
-		exit.SignalReason = "job_verification_failed_with_error"
-		return nil
+		default: // no error, all good, keep going
+			r.logger.Info("Successfully verified job %s with signature %s", job.ID, job.Step.Signature.Value)
+			r.logStreamer.Process([]byte(fmt.Sprintf("✅ Verified job with signature %s\n", job.Step.Signature.Value)))
+		}
 	}
 
 	// Before executing the bootstrap process with the received Job env, execute the pre-bootstrap hook (if present) for

diff --git a/agent/verify_job.go b/agent/verify_job.go
@@ -25,11 +25,6 @@ func (e *invalidSignatureError) Unwrap() error {
 	return e.underlying
 }
 
-func (e *invalidSignatureError) As(target any) bool {
-	_, ok := target.(*invalidSignatureError)
-	return ok
-}
-
 func (r *JobRunner) verificationFailureLogs(err error, behavior string) {
 	label := "WARNING"
 	if behavior == VerificationBehaviourBlock {
@@ -48,14 +43,6 @@ func (r *JobRunner) verificationFailureLogs(err error, behavior string) {
 func (r *JobRunner) verifyJob(verifier pipeline.Verifier) error {
 	step := r.conf.Job.Step
 
-	if verifier == nil {
-		if step.Signature != nil {
-			return fmt.Errorf("job %q was signed with signature %q, but no verification key was provided, so the job can't be verified", r.conf.Job.ID, step.Signature.Value)
-		}
-
-		return nil // no signature, no verifier, no problem
-	}
-
 	if step.Matrix != nil {
 		r.logger.Warn("Signing/Verification of matrix jobs is not currently supported")
 		r.logger.Warn("Watch this space 👀")