From 436e863c29c3b3153342f84fd185f09db9ef9a17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Agust=C3=ADn=20Mart=C3=ADnez=20Fay=C3=B3?= Date: Fri, 7 Jun 2024 21:35:02 -0300 Subject: [PATCH] Do not save the CA journal file anymore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Agustín Martínez Fayó --- pkg/server/ca/manager/journal.go | 76 ++------------------ pkg/server/ca/manager/journal_test.go | 100 ++++++-------------------- pkg/server/ca/manager/manager_test.go | 7 -- pkg/server/ca/manager/slot.go | 10 ++- pkg/server/ca/manager/slot_test.go | 11 ++- 5 files changed, 37 insertions(+), 167 deletions(-) diff --git a/pkg/server/ca/manager/journal.go b/pkg/server/ca/manager/journal.go index d8f1f0c82d..14463fba5c 100644 --- a/pkg/server/ca/manager/journal.go +++ b/pkg/server/ca/manager/journal.go @@ -3,14 +3,11 @@ package manager import ( "context" "crypto/x509" - "encoding/pem" "fmt" - "os" "sync" "time" "github.com/sirupsen/logrus" - "github.com/spiffe/spire/pkg/common/diskutil" "github.com/spiffe/spire/pkg/common/telemetry" "github.com/spiffe/spire/pkg/common/x509util" "github.com/spiffe/spire/pkg/server/ca" @@ -25,15 +22,11 @@ const ( // journalCap is the maximum number of entries per type that we'll // hold onto. journalCap = 10 - - // journalPEMType is the type in the PEM header - journalPEMType = "SPIRE CA JOURNAL" ) type journalConfig struct { - cat catalog.Catalog - log logrus.FieldLogger - filePath string + cat catalog.Catalog + log logrus.FieldLogger } // Journal stores X509 CAs and JWT keys on disk as they are rotated by the @@ -49,27 +42,11 @@ type Journal struct { func LoadJournal(ctx context.Context, config *journalConfig) (*Journal, error) { // Look for the CA journal of this server in the datastore. - journalDS, err := loadJournalFromDS(ctx, config) + j, err := loadJournalFromDS(ctx, config) if err != nil { return nil, fmt.Errorf("failed to load journal from datastore: %w", err) } - if journalDS != nil { - // A CA journal record corresponding to this server was found in the - // datastore. - return journalDS, nil - } - - // There is no CA journal record corresponding to this server in the - // datastore. Try to load the journal from disk. - - // TODO: stop trying to load the journal from disk in v1.10 and delete - // the journal file if exists. - journalDisk, err := loadJournalFromDisk(config) - if err != nil { - return nil, fmt.Errorf("failed to load journal from disk: %w", err) - } - - return journalDisk, nil + return j, nil } func (j *Journal) getEntries() *journal.Entries { @@ -311,15 +288,6 @@ func (j *Journal) save(ctx context.Context) error { } j.caJournalID = caJournalID - pemBytes := pem.EncodeToMemory(&pem.Block{ - Type: journalPEMType, - Bytes: entriesBytes, - }) - - if err := diskutil.AtomicWritePubliclyReadableFile(j.config.filePath, pemBytes); err != nil { - return errs.Wrap(err) - } - return nil } @@ -331,40 +299,6 @@ func chainDER(chain []*x509.Certificate) [][]byte { return der } -// loadJournalFromDisk loads the journal from disk if it exists. -// TODO: stop loading the journal from disk in v1.10 and remove this function. -func loadJournalFromDisk(config *journalConfig) (*Journal, error) { - config.log.WithField(telemetry.Path, config.filePath).Debug("Loading journal from disk") - - j := &Journal{ - config: config, - entries: new(journal.Entries), - } - - pemBytes, err := os.ReadFile(config.filePath) - if err != nil { - if os.IsNotExist(err) { - // There is no journal on disk. A new CA journal is created and will - // be stored in the next save operation. - return j, nil - } - return nil, errs.Wrap(err) - } - pemBlock, _ := pem.Decode(pemBytes) - if pemBlock == nil { - return nil, errs.New("invalid PEM block") - } - if pemBlock.Type != journalPEMType { - return nil, errs.New("invalid PEM block type %q", pemBlock.Type) - } - - if err := proto.Unmarshal(pemBlock.Bytes, j.entries); err != nil { - return nil, errs.New("unable to unmarshal entries: %v", err) - } - - return j, nil -} - // loadJournalFromDS loads the CA journal from the datastore. // It does that by looking for a CA journal record that matches with one of the // public keys of this server. @@ -382,7 +316,7 @@ func loadJournalFromDS(ctx context.Context, config *journalConfig) (*Journal, er } if caJournal == nil { j.config.log.Info("There is not a CA journal record that matches any of the local X509 authority IDs") - return nil, nil + return j, nil } j.caJournalID = caJournal.ID diff --git a/pkg/server/ca/manager/journal_test.go b/pkg/server/ca/manager/journal_test.go index adeccfc692..5a32b5b92c 100644 --- a/pkg/server/ca/manager/journal_test.go +++ b/pkg/server/ca/manager/journal_test.go @@ -4,15 +4,13 @@ import ( "context" "crypto/x509" "crypto/x509/pkix" - "encoding/pem" "errors" - "os" - "path/filepath" "testing" "time" "github.com/andres-erbsen/clock" "github.com/sirupsen/logrus/hooks/test" + "github.com/spiffe/spire/pkg/common/x509util" "github.com/spiffe/spire/pkg/server/ca" "github.com/spiffe/spire/pkg/server/credtemplate" "github.com/spiffe/spire/pkg/server/plugin/keymanager" @@ -81,9 +79,8 @@ func setupJournalTest(t *testing.T) *journalTest { return &journalTest{ ds: ds, jc: &journalConfig{ - cat: cat, - log: log, - filePath: filepath.Join(t.TempDir(), "journal.pem"), + cat: cat, + log: log, }, } } @@ -124,29 +121,10 @@ func TestJournalPersistence(t *testing.T) { require.NoError(t, j.UpdateX509CAStatus(ctx, now, journal.Status_ACTIVE)) // Check that the CA journal was properly stored in the datastore. - journalDS := test.loadJournalFromDS(t) + journalDS := test.loadJournal(t) require.NotNil(t, journalDS) spiretest.RequireProtoEqual(t, j.getEntries(), journalDS.getEntries()) - // TODO: the following checks assume that the CA journal is stored both in - // datastore and on disk. Revisit this in v1.10. - journalDisk := test.loadJournalFromDisk(t) - require.NotNil(t, journalDisk) - spiretest.RequireProtoEqual(t, j.getEntries(), journalDisk.getEntries()) - - // Test for the case when SPIRE starts with a CA journal on disk and does - // not yet have a CA journal stored in the datastore. Reset the datastore so - // we only have the CA journal on disk. - test.ds = fakedatastore.New(t) - test.jc.cat.(*fakeservercatalog.Catalog).SetDataStore(test.ds) - - // Load the journal again. It should still get the CA journal stored on - // disk. - j = test.loadJournal(t) - journalDisk = test.loadJournalFromDisk(t) - require.NotNil(t, journalDisk) - spiretest.RequireProtoEqual(t, j.getEntries(), journalDisk.getEntries()) - // Append a new X.509 CA, which will make the CA journal to be stored // on disk and in the datastore. now = now.Add(time.Minute) @@ -158,12 +136,9 @@ func TestJournalPersistence(t *testing.T) { require.NoError(t, err) require.NoError(t, j.UpdateX509CAStatus(ctx, now, journal.Status_ACTIVE)) - journalDS = test.loadJournalFromDS(t) + journalDS = test.loadJournal(t) require.NotNil(t, journalDS) spiretest.RequireProtoEqual(t, j.getEntries(), journalDS.getEntries()) - journalDisk = test.loadJournalFromDisk(t) - require.NotNil(t, journalDisk) - spiretest.RequireProtoEqual(t, j.getEntries(), journalDisk.getEntries()) // Simulate a datastore error dsError := errors.New("ds error") @@ -175,11 +150,6 @@ func TestJournalPersistence(t *testing.T) { }) require.Error(t, err) require.EqualError(t, err, "could not save CA journal in the datastore: ds error") - - // CA journal on disk should have been saved successfully - journalDisk = test.loadJournalFromDisk(t) - require.NotNil(t, journalDisk) - spiretest.RequireProtoEqual(t, j.getEntries(), journalDisk.getEntries()) } func TestAppendSetPreparedStatus(t *testing.T) { @@ -357,63 +327,35 @@ func TestJWTKeyOverflow(t *testing.T) { require.Equal(t, now, time.Unix(lastEntry.IssuedAt, 0).UTC()) } -func TestBadPEM(t *testing.T) { - test := setupJournalTest(t) - - test.writeString(t, test.jc.filePath, "NOT PEM") - _, err := LoadJournal(ctx, test.jc) - require.EqualError(t, err, "failed to load journal from disk: invalid PEM block") -} - -func TestUnexpectedPEMType(t *testing.T) { - test := setupJournalTest(t) - - test.writeBytes(t, test.jc.filePath, pem.EncodeToMemory(&pem.Block{ - Type: "WHATEVER", - Bytes: []byte("FOO"), - })) - _, err := LoadJournal(ctx, test.jc) - require.EqualError(t, err, `failed to load journal from disk: invalid PEM block type "WHATEVER"`) -} - func TestBadProto(t *testing.T) { test := setupJournalTest(t) - - test.writeBytes(t, test.jc.filePath, pem.EncodeToMemory(&pem.Block{ - Type: journalPEMType, - Bytes: []byte("FOO"), - })) - _, err := LoadJournal(ctx, test.jc) + j := &Journal{ + config: test.jc, + activeX509AuthorityID: getOneX509AuthorityID(ctx, t, test.jc.cat.GetKeyManager()), + } + caJournalID, err := j.saveInDatastore(ctx, []byte("FOO")) + require.NoError(t, err) + require.NotZero(t, caJournalID) + j, err = LoadJournal(ctx, test.jc) require.Error(t, err) - require.Contains(t, err.Error(), `unable to unmarshal entries: `) + require.Nil(t, j) + require.Contains(t, err.Error(), `failed to load journal from datastore: unable to unmarshal entries from CA journal record:`) } -func (j *journalTest) loadJournal(t *testing.T) *Journal { - journal, err := LoadJournal(ctx, j.jc) +func getOneX509AuthorityID(ctx context.Context, t *testing.T, km keymanager.KeyManager) string { + kmKeys, err := km.GetKeys(ctx) require.NoError(t, err) - return journal -} - -func (j *journalTest) loadJournalFromDisk(t *testing.T) *Journal { - journal, err := loadJournalFromDisk(j.jc) + subjectKeyID, err := x509util.GetSubjectKeyID(kmKeys[0].Public()) require.NoError(t, err) - return journal + return x509util.SubjectKeyIDToString(subjectKeyID) } -func (j *journalTest) loadJournalFromDS(t *testing.T) *Journal { - journal, err := loadJournalFromDS(ctx, j.jc) +func (j *journalTest) loadJournal(t *testing.T) *Journal { + journal, err := LoadJournal(ctx, j.jc) require.NoError(t, err) return journal } -func (j *journalTest) writeString(t *testing.T, path, data string) { - j.writeBytes(t, path, []byte(data)) -} - -func (j *journalTest) writeBytes(t *testing.T, path string, data []byte) { - require.NoError(t, os.WriteFile(path, data, 0600)) -} - func (j *journalTest) now() time.Time { // return truncated UTC time for cleaner failure messages return time.Now().UTC().Truncate(time.Second) diff --git a/pkg/server/ca/manager/manager_test.go b/pkg/server/ca/manager/manager_test.go index b26c17287a..3049851b70 100644 --- a/pkg/server/ca/manager/manager_test.go +++ b/pkg/server/ca/manager/manager_test.go @@ -10,8 +10,6 @@ import ( "errors" "fmt" "math/big" - "os" - "path/filepath" "sync" "testing" "time" @@ -1062,7 +1060,6 @@ func (m *managerTest) selfSignedConfigWithKeyTypes(x509CAKeyType, jwtKeyType key TrustDomain: testTrustDomain, X509CAKeyType: x509CAKeyType, JWTKeyType: jwtKeyType, - Dir: m.dir, Metrics: m.metrics, Log: m.log, Clock: m.clock, @@ -1206,10 +1203,6 @@ func (m *managerTest) addTimeAndPrune(d time.Duration) { } func (m *managerTest) wipeJournal(t *testing.T) { - // TODO: journal saved on this will no longer be supported in v1.10. Remove - // this. - require.NoError(t, os.Remove(filepath.Join(m.m.c.Dir, "journal.pem"))) - // Have a clean datastore. m.ds = fakedatastore.New(t) m.cat.SetDataStore(m.ds) diff --git a/pkg/server/ca/manager/slot.go b/pkg/server/ca/manager/slot.go index 85fdc80625..a772554631 100644 --- a/pkg/server/ca/manager/slot.go +++ b/pkg/server/ca/manager/slot.go @@ -7,6 +7,7 @@ import ( "crypto/x509" "errors" "fmt" + "os" "path/filepath" "time" @@ -57,9 +58,8 @@ func (s *SlotLoader) load(ctx context.Context) (*Journal, map[SlotPosition]Slot, log := s.Log jc := &journalConfig{ - cat: s.Catalog, - log: log, - filePath: s.journalPath(), + cat: s.Catalog, + log: log, } // Load the journal and see if we can figure out the next and current @@ -109,6 +109,10 @@ func (s *SlotLoader) load(ctx context.Context) (*Journal, map[SlotPosition]Slot, slots[NextJWTKeySlot] = nextJWTKey } + // TODO: Remove after 1.11.0 release. + // No point in checking for errors here, we're just trying to clean up the + // CA journal file that was used before 1.9.0. + os.Remove(s.journalPath()) return loadedJournal, slots, nil } diff --git a/pkg/server/ca/manager/slot_test.go b/pkg/server/ca/manager/slot_test.go index 0a77dcd833..d7864fecf7 100644 --- a/pkg/server/ca/manager/slot_test.go +++ b/pkg/server/ca/manager/slot_test.go @@ -4,7 +4,6 @@ import ( "context" "crypto/x509" "crypto/x509/pkix" - "path/filepath" "testing" "time" @@ -181,8 +180,7 @@ func TestJournalLoad(t *testing.T) { jwtKeyBPKIX, err := x509.MarshalPKIXPublicKey(jwtKeyB.Public()) require.NoError(t, err) - tempDir := t.TempDir() - journalFile := filepath.Join(tempDir, "journal.pem") + activeX509AuthorityID := getOneX509AuthorityID(ctx, t, km) // Dates firstIssuedAtUnix := now.Add(-3 * time.Minute).Unix() @@ -776,18 +774,17 @@ func TestJournalLoad(t *testing.T) { loghook.Reset() journal := new(Journal) journal.config = &journalConfig{ - cat: cat, - filePath: journalFile, - log: log, + cat: cat, + log: log, } journal.setEntries(tt.entries) + journal.activeX509AuthorityID = activeX509AuthorityID err = journal.save(ctx) require.NoError(t, err) loader := &SlotLoader{ TrustDomain: td, Log: log, - Dir: tempDir, Catalog: cat, }