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.

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

Allow empty x509 bundles to be sent in responses #288

Merged
merged 4 commits into from
Jun 17, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
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 []byte{}
sorindumitru marked this conversation as resolved.
Show resolved Hide resolved
}

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 bundles and assert that the call
sorindumitru marked this conversation as resolved.
Show resolved Hide resolved
// 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