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

Do not save the CA journal file anymore #5202

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Do not save the CA journal file anymore #5202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
76 changes: 5 additions & 71 deletions pkg/server/ca/manager/journal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}

Expand All @@ -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.
Expand All @@ -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
Expand Down
100 changes: 21 additions & 79 deletions pkg/server/ca/manager/journal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
},
}
}
Expand Down Expand Up @@ -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)
Expand All @@ -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")
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 0 additions & 7 deletions pkg/server/ca/manager/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ import (
"errors"
"fmt"
"math/big"
"os"
"path/filepath"
"sync"
"testing"
"time"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down
10 changes: 7 additions & 3 deletions pkg/server/ca/manager/slot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"crypto/x509"
"errors"
"fmt"
"os"
"path/filepath"
"time"

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
11 changes: 4 additions & 7 deletions pkg/server/ca/manager/slot_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"crypto/x509"
"crypto/x509/pkix"
"path/filepath"
"testing"
"time"

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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,
}

Expand Down
Loading