From dc9d74af2170e1707521348c02712dc1b661ec97 Mon Sep 17 00:00:00 2001 From: linus-sun Date: Mon, 14 Oct 2024 20:17:30 +0000 Subject: [PATCH] refactor workflow inputs to use string/bool vals instead of pointers Signed-off-by: linus-sun --- cmd/verifier/main.go | 7 +++- pkg/rekor/identity.go | 4 +-- pkg/rekor/verifier.go | 35 ++++++++++++++----- pkg/rekor/verifier_test.go | 71 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 105 insertions(+), 12 deletions(-) diff --git a/cmd/verifier/main.go b/cmd/verifier/main.go index f9ddcaf2..5ce979d4 100644 --- a/cmd/verifier/main.go +++ b/cmd/verifier/main.go @@ -83,7 +83,12 @@ func main() { log.Fatal(err) } - err = rekor.RunConsistencyCheck(interval, rekorClient, verifier, logInfoFile, monitoredVals, outputIdentitiesFile, once) + err = rekor.VerifyConsistencyCheckInputs(interval, logInfoFile, outputIdentitiesFile, once) + if err != nil { + log.Fatal(err) + } + + err = rekor.RunConsistencyCheck(*interval, rekorClient, verifier, *logInfoFile, monitoredVals, *outputIdentitiesFile, *once) if err != nil { log.Fatalf("%v", err) } diff --git a/pkg/rekor/identity.go b/pkg/rekor/identity.go index 3b443205..2929be67 100644 --- a/pkg/rekor/identity.go +++ b/pkg/rekor/identity.go @@ -342,7 +342,7 @@ func oidMatchesPolicy(cert *x509.Certificate, oid asn1.ObjectIdentifier, extensi } // writeIdentitiesBetweenCheckpoints monitors for given identities between two checkpoints and writes any found identities to file. -func writeIdentitiesBetweenCheckpoints(logInfo *models.LogInfo, prevCheckpoint *util.SignedCheckpoint, checkpoint *util.SignedCheckpoint, monitoredValues identity.MonitoredValues, rekorClient *client.Rekor, outputIdentitiesFile *string) error { +func writeIdentitiesBetweenCheckpoints(logInfo *models.LogInfo, prevCheckpoint *util.SignedCheckpoint, checkpoint *util.SignedCheckpoint, monitoredValues identity.MonitoredValues, rekorClient *client.Rekor, outputIdentitiesFile string) error { // Get log size of inactive shards totalSize := 0 for _, s := range logInfo.InactiveShards { @@ -366,7 +366,7 @@ func writeIdentitiesBetweenCheckpoints(logInfo *models.LogInfo, prevCheckpoint * for _, idEntry := range idEntries { fmt.Fprintf(os.Stderr, "Found %s\n", idEntry.String()) - if err := file.WriteIdentity(*outputIdentitiesFile, idEntry); err != nil { + if err := file.WriteIdentity(outputIdentitiesFile, idEntry); err != nil { return fmt.Errorf("failed to write entry: %v", err) } } diff --git a/pkg/rekor/verifier.go b/pkg/rekor/verifier.go index 4d5cb26c..d68f3090 100644 --- a/pkg/rekor/verifier.go +++ b/pkg/rekor/verifier.go @@ -18,6 +18,7 @@ import ( "context" "crypto" "encoding/hex" + "errors" "fmt" "os" "time" @@ -65,9 +66,9 @@ func verifyLatestCheckpointSignature(logInfo *models.LogInfo, verifier signature // verifyCheckpointConsistency reads and verifies the consistency of the previous latest checkpoint from a log info file against the current up-to-date checkpoint. // If it successfully fetches and verifies the consistency between these two checkpoints, it returns the previous checkpoint; otherwise, it returns an error. -func verifyCheckpointConsistency(logInfoFile *string, checkpoint *util.SignedCheckpoint, treeID string, rekorClient *client.Rekor, verifier signature.Verifier) (*util.SignedCheckpoint, error) { +func verifyCheckpointConsistency(logInfoFile string, checkpoint *util.SignedCheckpoint, treeID string, rekorClient *client.Rekor, verifier signature.Verifier) (*util.SignedCheckpoint, error) { var prevCheckpoint *util.SignedCheckpoint - prevCheckpoint, err := file.ReadLatestCheckpoint(*logInfoFile) + prevCheckpoint, err := file.ReadLatestCheckpoint(logInfoFile) if err != nil { return nil, fmt.Errorf("reading checkpoint log: %v", err) } @@ -82,10 +83,26 @@ func verifyCheckpointConsistency(logInfoFile *string, checkpoint *util.SignedChe return prevCheckpoint, nil } +// VerifyConsistencyCheckInputs verifies that the input flag values to the rekor-monitor workflow are not nil. +func VerifyConsistencyCheckInputs(interval *time.Duration, logInfoFile *string, outputIdentitiesFile *string, once *bool) error { + if interval == nil { + return errors.New("--interval flag equal to nil") + } + if logInfoFile == nil { + return errors.New("--file flag equal to nil") + } + if outputIdentitiesFile == nil { + return errors.New("--output-identities equal to nil") + } + if once == nil { + return errors.New("--once equal to nil") + } + return nil +} + // RunConsistencyCheck periodically verifies the root hash consistency of a Rekor log. -// TODO: RunConsistencyCheck should take in string/bool flags directly instead of pointers and check that flags are being set correctly. -func RunConsistencyCheck(interval *time.Duration, rekorClient *client.Rekor, verifier signature.Verifier, logInfoFile *string, mvs identity.MonitoredValues, outputIdentitiesFile *string, once *bool) error { - ticker := time.NewTicker(*interval) +func RunConsistencyCheck(interval time.Duration, rekorClient *client.Rekor, verifier signature.Verifier, logInfoFile string, mvs identity.MonitoredValues, outputIdentitiesFile string, once bool) error { + ticker := time.NewTicker(interval) defer ticker.Stop() // Loop will: @@ -104,7 +121,7 @@ func RunConsistencyCheck(interval *time.Duration, rekorClient *client.Rekor, ver return fmt.Errorf("failed to verify signature of latest checkpoint: %v", err) } - fi, err := os.Stat(*logInfoFile) + fi, err := os.Stat(logInfoFile) // File containing previous checkpoints exists var prevCheckpoint *util.SignedCheckpoint if err == nil && fi.Size() != 0 { @@ -117,7 +134,7 @@ func RunConsistencyCheck(interval *time.Duration, rekorClient *client.Rekor, ver // Write if there was no stored checkpoint or the sizes differ if prevCheckpoint == nil || prevCheckpoint.Size != checkpoint.Size { - if err := file.WriteCheckpoint(checkpoint, *logInfoFile); err != nil { + if err := file.WriteCheckpoint(checkpoint, logInfoFile); err != nil { // TODO: Once the consistency check and identity search are split into separate tasks, this should hard fail. // Temporarily skipping this to allow this job to succeed, remediating the issue noted here: https://github.com/sigstore/rekor-monitor/issues/271 fmt.Fprintf(os.Stderr, "failed to write checkpoint: %v", err) @@ -134,11 +151,11 @@ func RunConsistencyCheck(interval *time.Duration, rekorClient *client.Rekor, ver // TODO: Switch to writing checkpoints to GitHub so that the history is preserved. Then we only need // to persist the last checkpoint. // Delete old checkpoints to avoid the log growing indefinitely - if err := file.DeleteOldCheckpoints(*logInfoFile); err != nil { + if err := file.DeleteOldCheckpoints(logInfoFile); err != nil { return fmt.Errorf("failed to delete old checkpoints: %v", err) } - if *once { + if once { return nil } } diff --git a/pkg/rekor/verifier_test.go b/pkg/rekor/verifier_test.go index 67145f20..f5249cec 100644 --- a/pkg/rekor/verifier_test.go +++ b/pkg/rekor/verifier_test.go @@ -19,7 +19,9 @@ import ( "crypto/ecdsa" "crypto/elliptic" "crypto/rand" + "errors" "testing" + "time" "github.com/sigstore/rekor-monitor/pkg/rekor/mock" "github.com/sigstore/rekor/pkg/generated/client" @@ -47,3 +49,72 @@ func TestGetLogVerifier(t *testing.T) { t.Fatalf("expected equal keys: %v", err) } } + +func TestVerifyConsistencyCheckInputs(t *testing.T) { + interval := 5 * time.Minute + logInfoFile := "./test/example_log_info_file_path.txt" + outputIdentitiesFile := "./test/example_output_identities_file.txt" + once := true + verifyConsistencyCheckInputTests := map[string]struct { + interval *time.Duration + logInfoFile *string + outputIdentitiesFile *string + once *bool + expectedError error + }{ + "successful verification": { + interval: &interval, + logInfoFile: &logInfoFile, + outputIdentitiesFile: &outputIdentitiesFile, + once: &once, + expectedError: nil, + }, + "fail --interval verification": { + interval: nil, + logInfoFile: &logInfoFile, + outputIdentitiesFile: &outputIdentitiesFile, + once: &once, + expectedError: errors.New("--interval flag equal to nil"), + }, + "fail --file verification": { + interval: &interval, + logInfoFile: nil, + outputIdentitiesFile: &outputIdentitiesFile, + once: &once, + expectedError: errors.New("--file flag equal to nil"), + }, + "fail --output-identities verification": { + interval: &interval, + logInfoFile: &logInfoFile, + outputIdentitiesFile: nil, + once: &once, + expectedError: errors.New("--output-identities flag equal to nil"), + }, + "fail --once verification": { + interval: &interval, + logInfoFile: &logInfoFile, + outputIdentitiesFile: &outputIdentitiesFile, + once: nil, + expectedError: errors.New("--once flag equal to nil"), + }, + "empty case": { + interval: nil, + logInfoFile: nil, + outputIdentitiesFile: nil, + once: nil, + expectedError: errors.New("--interval flag equal to nil"), + }, + } + + for _, verifyConsistencyCheckInputTestCase := range verifyConsistencyCheckInputTests { + interval := verifyConsistencyCheckInputTestCase.interval + logInfoFile := verifyConsistencyCheckInputTestCase.logInfoFile + outputIdentitiesFile := verifyConsistencyCheckInputTestCase.outputIdentitiesFile + once := verifyConsistencyCheckInputTestCase.once + expectedError := verifyConsistencyCheckInputTestCase.expectedError + err := VerifyConsistencyCheckInputs(interval, logInfoFile, outputIdentitiesFile, once) + if err != expectedError { + t.Errorf("expected error %v, received error %v", expectedError, err) + } + } +}