Skip to content

Commit

Permalink
Allow empty x509 bundles to be sent in responses (#288)
Browse files Browse the repository at this point in the history
This causes the workload to be unable to fetch X509-SVIDs, even though its own trust domain still has a valid trust bundle

Signed-off-by: Sorin Dumitru <sorin@returnze.ro>
  • Loading branch information
sorindumitru authored Jun 17, 2024
1 parent fb781b6 commit 5460476
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 27 deletions.
14 changes: 8 additions & 6 deletions v2/bundle/x509bundle/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,14 @@ func Read(trustDomain spiffeid.TrustDomain, r io.Reader) (*Bundle, error) {
// blocks.
func Parse(trustDomain spiffeid.TrustDomain, b []byte) (*Bundle, error) {
bundle := New(trustDomain)
if len(b) == 0 {
return bundle, nil
}

certs, err := pemutil.ParseCertificates(b)
if err != nil {
return nil, x509bundleErr.New("cannot parse certificate: %v", err)
}
if len(certs) == 0 {
return nil, x509bundleErr.New("no certificates found")
}
for _, cert := range certs {
bundle.AddX509Authority(cert)
}
Expand All @@ -80,13 +81,14 @@ func Parse(trustDomain spiffeid.TrustDomain, b []byte) (*Bundle, error) {
// with no intermediate padding if there are more than one certificate)
func ParseRaw(trustDomain spiffeid.TrustDomain, b []byte) (*Bundle, error) {
bundle := New(trustDomain)
if len(b) == 0 {
return bundle, nil
}

certs, err := x509.ParseCertificates(b)
if err != nil {
return nil, x509bundleErr.New("cannot parse certificate: %v", err)
}
if len(certs) == 0 {
return nil, x509bundleErr.New("no certificates found")
}
for _, cert := range certs {
bundle.AddX509Authority(cert)
}
Expand Down
21 changes: 10 additions & 11 deletions v2/bundle/x509bundle/bundle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,20 +96,15 @@ func TestParse(t *testing.T) {
expNumAuthorities: 1,
},
{
name: "Parse empty bytes should fail",
path: "testdata/empty.pem",
expErrContains: "x509bundle: cannot parse certificate: no PEM blocks found",
name: "Parse empty bytes should result in empty bundle",
path: "testdata/empty.pem",
expNumAuthorities: 0,
},
{
name: "Parse non-PEM bytes should fail",
path: "testdata/not-pem.pem",
expErrContains: "x509bundle: cannot parse certificate: no PEM blocks found",
},
{
name: "Parse should fail if no certificate block is is found",
path: "testdata/key.pem",
expErrContains: "x509bundle: no certificates found",
},
{
name: "Parse a corrupted certificate should fail",
path: "testdata/corrupted.pem",
Expand Down Expand Up @@ -155,9 +150,9 @@ func TestParseRaw(t *testing.T) {
expNumAuthorities: 1,
},
{
name: "Parse should fail if no certificate block is is found",
path: "testdata/key.pem",
expErrContains: "x509bundle: no certificates found",
name: "Parse should not fail if no certificate block is is found",
path: "testdata/empty.pem",
expNumAuthorities: 0,
},
}

Expand Down Expand Up @@ -322,6 +317,10 @@ func loadRawCertificates(t *testing.T, path string) []byte {
certsBytes, err := os.ReadFile(path)
require.NoError(t, err)

if len(certsBytes) == 0 {
return nil
}

certs, err := pemutil.ParseCertificates(certsBytes)
require.NoError(t, err)

Expand Down
4 changes: 0 additions & 4 deletions v2/workloadapi/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"crypto/x509"
"errors"
"fmt"
"time"

"github.com/spiffe/go-spiffe/v2/bundle/jwtbundle"
Expand Down Expand Up @@ -489,9 +488,6 @@ func parseX509Bundle(spiffeID string, bundle []byte) (*x509bundle.Bundle, error)
if err != nil {
return nil, err
}
if len(certs) == 0 {
return nil, fmt.Errorf("empty X.509 bundle for trust domain %q", td)
}
return x509bundle.FromX509Authorities(td, certs), nil
}

Expand Down
18 changes: 12 additions & 6 deletions v2/workloadapi/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,16 +190,22 @@ func TestFetchX509Context(t *testing.T) {
assertX509Bundle(t, x509Ctx.Bundles, td, ca.X509Bundle())
assertX509Bundle(t, x509Ctx.Bundles, federatedTD, federatedCA.X509Bundle())

// Now set the next response without any bundles and assert that the call
// fails since the bundle cannot be empty.
// Now set the next response with an empty federated bundle and assert that the call
// still succeeds.
wl.SetX509SVIDResponse(&fakeworkloadapi.X509SVIDResponse{
SVIDs: svids,
Bundle: ca.X509Bundle(),
SVIDs: svids,
FederatedBundles: []*x509bundle.Bundle{x509bundle.FromX509Authorities(federatedCA.Bundle().TrustDomain(), nil)},
})

x509Ctx, err = c.FetchX509Context(context.Background())

require.EqualError(t, err, `empty X.509 bundle for trust domain "example.org"`)
require.Nil(t, x509Ctx)
require.NoError(t, err)
// inspect svids
require.Len(t, x509Ctx.SVIDs, 4)
assertX509SVID(t, x509Ctx.SVIDs[0], fooID, resp.SVIDs[0].Certificates, hintInternal)
assertX509SVID(t, x509Ctx.SVIDs[1], barID, resp.SVIDs[1].Certificates, hintExternal)
assertX509SVID(t, x509Ctx.SVIDs[2], emptyHintSVID1.ID, resp.SVIDs[3].Certificates, "")
assertX509SVID(t, x509Ctx.SVIDs[3], emptyHintSVID2.ID, resp.SVIDs[4].Certificates, "")
}

func TestWatchX509Context(t *testing.T) {
Expand Down

0 comments on commit 5460476

Please sign in to comment.