From 54604766f7b20cbafd311b91b8f77fa4a452d6fc Mon Sep 17 00:00:00 2001 From: Sorin Dumitru Date: Mon, 17 Jun 2024 14:40:06 +0100 Subject: [PATCH] Allow empty x509 bundles to be sent in responses (#288) 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 --- v2/bundle/x509bundle/bundle.go | 14 ++++++++------ v2/bundle/x509bundle/bundle_test.go | 21 ++++++++++----------- v2/workloadapi/client.go | 4 ---- v2/workloadapi/client_test.go | 18 ++++++++++++------ 4 files changed, 30 insertions(+), 27 deletions(-) diff --git a/v2/bundle/x509bundle/bundle.go b/v2/bundle/x509bundle/bundle.go index ffe28561..a70bb62f 100644 --- a/v2/bundle/x509bundle/bundle.go +++ b/v2/bundle/x509bundle/bundle.go @@ -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) } @@ -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) } diff --git a/v2/bundle/x509bundle/bundle_test.go b/v2/bundle/x509bundle/bundle_test.go index 081b17c9..2348a3cc 100644 --- a/v2/bundle/x509bundle/bundle_test.go +++ b/v2/bundle/x509bundle/bundle_test.go @@ -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", @@ -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, }, } @@ -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) diff --git a/v2/workloadapi/client.go b/v2/workloadapi/client.go index b357468f..b499eef8 100644 --- a/v2/workloadapi/client.go +++ b/v2/workloadapi/client.go @@ -4,7 +4,6 @@ import ( "context" "crypto/x509" "errors" - "fmt" "time" "github.com/spiffe/go-spiffe/v2/bundle/jwtbundle" @@ -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 } diff --git a/v2/workloadapi/client_test.go b/v2/workloadapi/client_test.go index 13da9025..5040d8fb 100644 --- a/v2/workloadapi/client_test.go +++ b/v2/workloadapi/client_test.go @@ -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) {