Skip to content

Commit

Permalink
Optimize spire server tls connection bundle conversion amount (spiffe…
Browse files Browse the repository at this point in the history
…#3759)

Signed-off-by: Guilherme Carvalho <[email protected]>
  • Loading branch information
guilhermocc authored Jan 26, 2023
1 parent 2ed7d91 commit 2c29cd4
Show file tree
Hide file tree
Showing 5 changed files with 175 additions and 24 deletions.
25 changes: 2 additions & 23 deletions pkg/server/endpoints/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ import (
"github.com/spiffe/go-spiffe/v2/spiffetls/tlsconfig"
"github.com/spiffe/go-spiffe/v2/svid/x509svid"
"github.com/spiffe/spire/pkg/common/telemetry"
"github.com/spiffe/spire/pkg/server/cache/dscache"
"github.com/spiffe/spire/pkg/server/svid"
"github.com/spiffe/spire/proto/spire/common"
)

var (
Expand Down Expand Up @@ -44,11 +42,11 @@ func shouldLogFederationMisconfiguration(td spiffeid.TrustDomain) bool {

// bundleGetter fetches the bundle for the given trust domain and parse it as x509 certificates.
func (e *Endpoints) bundleGetter(ctx context.Context, td spiffeid.TrustDomain) ([]*x509.Certificate, error) {
commonServerBundle, err := e.DataStore.FetchBundle(dscache.WithCache(ctx), td.IDString())
serverBundle, err := e.BundleCache.FetchBundleX509(ctx, td)
if err != nil {
return nil, fmt.Errorf("get bundle from datastore: %w", err)
}
if commonServerBundle == nil {
if serverBundle == nil {
if td != e.TrustDomain && shouldLogFederationMisconfiguration(td) {
e.Log.
WithField(telemetry.TrustDomain, td.String()).
Expand All @@ -60,11 +58,6 @@ func (e *Endpoints) bundleGetter(ctx context.Context, td spiffeid.TrustDomain) (
return nil, fmt.Errorf("no bundle found for trust domain %q", td)
}

serverBundle, err := parseBundle(e.TrustDomain, commonServerBundle)
if err != nil {
return nil, err
}

return serverBundle.X509Authorities(), nil
}

Expand Down Expand Up @@ -105,20 +98,6 @@ func matchMemberOrOneOf(trustDomain spiffeid.TrustDomain, adminIds ...spiffeid.I
}
}

// parseBundle parses a *x509bundle.Bundle from a *common.bundle.
func parseBundle(td spiffeid.TrustDomain, commonBundle *common.Bundle) (*x509bundle.Bundle, error) {
var caCerts []*x509.Certificate
for _, rootCA := range commonBundle.RootCas {
rootCACerts, err := x509.ParseCertificates(rootCA.DerBytes)
if err != nil {
return nil, fmt.Errorf("parse bundle: %w", err)
}
caCerts = append(caCerts, rootCACerts...)
}

return x509bundle.FromX509Authorities(td, caCerts), nil
}

type x509SVIDSource struct {
getter func() svid.State
}
Expand Down
98 changes: 98 additions & 0 deletions pkg/server/endpoints/bundle/cache.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
package bundle

import (
"context"
"crypto/x509"
"fmt"
"sync"
"time"

"github.com/andres-erbsen/clock"
"github.com/spiffe/go-spiffe/v2/spiffeid"
"github.com/spiffe/spire/proto/spire/common"
"google.golang.org/protobuf/proto"

"github.com/spiffe/go-spiffe/v2/bundle/x509bundle"
"github.com/spiffe/spire/pkg/server/datastore"
)

const (
cacheExpiry = time.Second
)

type Cache struct {
ds datastore.DataStore
bundlesMtx sync.Mutex
bundles map[spiffeid.TrustDomain]*bundleEntry
clock clock.Clock
}

func NewCache(ds datastore.DataStore, clk clock.Clock) *Cache {
return &Cache{
ds: ds,
clock: clk,
bundles: make(map[spiffeid.TrustDomain]*bundleEntry),
}
}

type bundleEntry struct {
mu sync.Mutex
ts time.Time
bundle *common.Bundle
x509Bundle *x509bundle.Bundle
}

func (c *Cache) FetchBundleX509(ctx context.Context, td spiffeid.TrustDomain) (*x509bundle.Bundle, error) {
c.bundlesMtx.Lock()
entry, ok := c.bundles[td]
if !ok {
entry = &bundleEntry{}
c.bundles[td] = entry
}
c.bundlesMtx.Unlock()

entry.mu.Lock()
defer entry.mu.Unlock()
if entry.ts.IsZero() || c.clock.Now().Sub(entry.ts) >= cacheExpiry {
bundle, err := c.ds.FetchBundle(ctx, td.IDString())
if err != nil {
return nil, err
}
if bundle == nil {
c.deleteEntry(td)
return nil, nil
}

entry.ts = c.clock.Now()
if proto.Equal(entry.bundle, bundle) {
return entry.x509Bundle, nil
}
x509Bundle, err := parseBundle(td, bundle)
if err != nil {
return nil, err
}
entry.x509Bundle = x509Bundle
entry.bundle = bundle
}
return entry.x509Bundle, nil
}

func (c *Cache) deleteEntry(td spiffeid.TrustDomain) {
c.bundlesMtx.Lock()
delete(c.bundles, td)
c.bundlesMtx.Unlock()
}

// parseBundle parses a *x509bundle.Bundle from a *common.bundle.
func parseBundle(td spiffeid.TrustDomain, commonBundle *common.Bundle) (*x509bundle.Bundle, error) {
var caCerts []*x509.Certificate
for _, rootCA := range commonBundle.RootCas {
rootCACerts, err := x509.ParseCertificates(rootCA.DerBytes)
if err != nil {
return nil, fmt.Errorf("parse bundle: %w", err)
}
caCerts = append(caCerts, rootCACerts...)
}

return x509bundle.FromX509Authorities(td, caCerts), nil
}
68 changes: 68 additions & 0 deletions pkg/server/endpoints/bundle/cache_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package bundle

import (
"context"
"testing"

"github.com/spiffe/spire/test/clock"

"github.com/spiffe/go-spiffe/v2/bundle/x509bundle"
"github.com/spiffe/go-spiffe/v2/spiffeid"
"github.com/spiffe/spire/proto/spire/common"
"github.com/spiffe/spire/test/fakes/fakedatastore"
"github.com/spiffe/spire/test/testca"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestFetchBundleX509(t *testing.T) {
td := spiffeid.RequireTrustDomainFromString("spiffe://domain.test")
ca := testca.New(t, td)
certs1, _ := ca.CreateX509Certificate()
certs2, _ := ca.CreateX509Certificate()

bundleX509Response := x509bundle.FromX509Authorities(td, certs1)
updatedBundleX509Response := x509bundle.FromX509Authorities(td, certs2)
bundle1 := &common.Bundle{TrustDomainId: "spiffe://domain.test", RefreshHint: 1, RootCas: []*common.Certificate{{DerBytes: certs1[0].Raw}}}
bundle2 := &common.Bundle{TrustDomainId: "spiffe://domain.test", RefreshHint: 2, RootCas: []*common.Certificate{{DerBytes: certs2[0].Raw}}}
ds := fakedatastore.New(t)
clock := clock.NewMock(t)
cache := NewCache(ds, clock)
ctx := context.Background()

// Assert bundle is missing
bundleX509, err := cache.FetchBundleX509(ctx, td)
require.NoError(t, err)
require.Nil(t, bundleX509)

// Add bundle
_, err = ds.SetBundle(ctx, bundle1)
require.NoError(t, err)

// Assert that we didn't cache the bundle miss and that the newly added
// bundle is there
bundleX509, err = cache.FetchBundleX509(ctx, td)
require.NoError(t, err)
assert.Equal(t, bundleX509Response, bundleX509)

// Change bundle
_, err = ds.SetBundle(context.Background(), bundle2)
require.NoError(t, err)

// Assert bundle contents unchanged since cache is still valid
bundleX509, err = cache.FetchBundleX509(ctx, td)
require.NoError(t, err)
assert.Equal(t, bundleX509Response, bundleX509)

// If caches expires by time, FetchBundleX509 must fetch a fresh bundle
clock.Add(cacheExpiry)
bundleX509, err = cache.FetchBundleX509(ctx, td)
require.NoError(t, err)
assert.Equal(t, updatedBundleX509Response, bundleX509)

// If caches expires by time, but bundle didn't change, FetchBundleX509 must fetch a fresh bundle
clock.Add(cacheExpiry)
bundleX509, err = cache.FetchBundleX509(ctx, td)
require.NoError(t, err)
assert.Equal(t, updatedBundleX509Response, bundleX509)
}
7 changes: 6 additions & 1 deletion pkg/server/endpoints/endpoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/spiffe/go-spiffe/v2/spiffetls/tlsconfig"
"github.com/spiffe/spire/pkg/server/cache/entrycache"
"github.com/spiffe/spire/pkg/server/endpoints/bundle"
"golang.org/x/net/context"
"golang.org/x/net/http2"
"google.golang.org/grpc"
Expand Down Expand Up @@ -62,6 +63,7 @@ type Endpoints struct {
SVIDObserver svid.Observer
TrustDomain spiffeid.TrustDomain
DataStore datastore.DataStore
BundleCache *bundle.Cache
APIServers APIServers
BundleEndpointServer Server
Log logrus.FieldLogger
Expand Down Expand Up @@ -117,12 +119,15 @@ func New(ctx context.Context, c Config) (*Endpoints, error) {
return nil, err
}

ds := c.Catalog.GetDataStore()

return &Endpoints{
TCPAddr: c.TCPAddr,
LocalAddr: c.LocalAddr,
SVIDObserver: c.SVIDObserver,
TrustDomain: c.TrustDomain,
DataStore: c.Catalog.GetDataStore(),
DataStore: ds,
BundleCache: bundle.NewCache(ds, c.Clock),
APIServers: c.makeAPIServers(ef),
BundleEndpointServer: c.maybeMakeBundleEndpointServer(),
Log: c.Log,
Expand Down
1 change: 1 addition & 0 deletions pkg/server/endpoints/endpoints_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ func TestListenAndServe(t *testing.T) {
SVIDObserver: newSVIDObserver(serverSVID),
TrustDomain: testTD,
DataStore: ds,
BundleCache: bundle.NewCache(ds, clk),
APIServers: APIServers{
AgentServer: &agentv1.UnimplementedAgentServer{},
BundleServer: &bundlev1.UnimplementedBundleServer{},
Expand Down

0 comments on commit 2c29cd4

Please sign in to comment.