fix(sdk): Fix possible panic for AttributeValueFQN.Prefix() (#1472)

This commit adds additional checks to validate that Attribute Name /
Values are not empty or contain a slash. This fixes a panic condition
where `NewAttributeValueFQN` would return without error but `Prefix()`
would then fail when the prefix is attempted to be parsed by
`NewAttributeNameFQN`.

This commit includes unit testing, and the fuzz testing that initially
discovered this issue. See there for possibly dangerous values.

An alternative or additional solution that we may want to consider:
If `Prefix()` is a common or frequent operation we could proactively
invoke the `NewAttributeNameFQN` from within `NewAttributeValueFQN`.
That way any future validation added to `NewAttributeNameFQN` would be
proactively checked rather than needing `Prefix()` to be invoked to be
tested.

Also included are some minor doc changes, spelling fixes, and some other
fuzz testing that was co-located with these changes.

PR closes #1468

Author: jentfoo

sdk/fuzz_test.go

@@ -2,10 +2,15 @@ package sdk
 
 import (
 	"bytes"
+	"context"
 	"encoding/base64"
 	"io"
+	"net/http"
 	"testing"
 
+	"github.com/lestrrat-go/jwx/v2/jwk"
+	"github.com/opentdf/platform/sdk/auth"
+
 	"github.com/opentdf/platform/lib/ocrypto"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -25,10 +30,22 @@ func newSDK() *SDK {
 	sdk := &SDK{
 		config:      *cfg,
 		kasKeyCache: newKasKeyCache(),
+		tokenSource: &fakeTokenSource{},
 	}
 	return sdk
 }
 
+type fakeTokenSource struct {
+}
+
+func (f *fakeTokenSource) AccessToken(_ context.Context, _ *http.Client) (auth.AccessToken, error) {
+	return "fake token", nil
+}
+
+func (f *fakeTokenSource) MakeToken(func(jwk.Key) ([]byte, error)) ([]byte, error) {
+	return []byte("fake token"), nil
+}
+
 func unverifiedBase64Bytes(str string) []byte {
 	b, _ := base64.StdEncoding.DecodeString(str)
 	return b
@@ -92,6 +109,48 @@ func FuzzLoadTDF(f *testing.F) {
 	})
 }
 
+func FuzzReadNanoTDF(f *testing.F) {
+	sdk := newSDK()
+	f.Add([]byte{ // seed from xtest
+		// header
+		0x4c, 0x31, 0x4c, // version
+		0x00, 0x12, 0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x68, 0x6f, 0x73, 0x74, 0x3a, 0x38, 0x30, 0x38, 0x30, 0x2f, 0x6b, 0x61, 0x73, // kas
+		0x00, // binding_mode
+		0x01, // symmetric_and_payload_config
+		// policy
+		0x02, 0x00, 0x68, 0xef, 0x70, 0x7b, 0x5f, 0x20, 0xb9, 0x0b, 0xf5, 0x96, 0xc3, 0xd7, 0x42, 0x85, 0x17, 0x6c, 0xd8, 0x98,
+		0xad, 0x47, 0xc4, 0x9a, 0x81, 0x5f, 0x67, 0xc4, 0x0f, 0xff, 0x16, 0xbb, 0xf0, 0xf4, 0xcd, 0x31, 0xa5, 0xf6, 0x86, 0x59,
+		0x3d, 0xf1, 0x53, 0x39, 0x3c, 0x3e, 0x16, 0xd8, 0xd2, 0x3b, 0x37, 0x50, 0x86, 0x6c, 0xfd, 0x2b, 0xce, 0xc7, 0x10, 0x89,
+		0x66, 0x74, 0x22, 0xf0, 0x3f, 0x16, 0x7a, 0xed, 0x37, 0x93, 0x03, 0x30, 0xcc, 0x05, 0x21, 0xd2, 0x9e, 0x5d, 0xc3, 0x34,
+		0xc5, 0x51, 0x60, 0xe6, 0xbf, 0x16, 0xdf, 0x92, 0xd0, 0x8d, 0xb0, 0xf0, 0x57, 0x6f, 0x7c, 0x37, 0xb9, 0x84, 0x44, 0xc7,
+		0x64, 0x99, 0x6a, 0xd3, 0x6e, 0xaa, 0x04,
+		0xf3, 0x18, 0xe9, 0x0b, 0xd0, 0xdc, 0x05, 0x38, // gmac_binding
+		// ephemeral_key
+		0x03, 0x66, 0x95, 0xd9, 0x3b, 0x84, 0xee, 0xc5, 0x65, 0xc1, 0x13, 0x1c, 0x94, 0xc6, 0x00, 0x8b, 0xcb, 0x6a, 0xf5, 0x90,
+		0xd5, 0x0d, 0x90, 0xc5, 0xf4, 0xe5, 0x96, 0x56, 0xb2, 0xd9, 0x4a, 0x9b, 0x51,
+		// payload
+		0x00, 0x00, 0x8f, // length
+		0x54, 0x2b, 0x53, // iv
+		// ciphertext
+		0xce, 0x35, 0x1d, 0x0a, 0xd9, 0x7a, 0x81, 0xb5, 0xda, 0x93, 0x39, 0xd5, 0xa2, 0x42, 0x22, 0xa3, 0x64, 0x97, 0x2e, 0x33,
+		0x41, 0x84, 0x12, 0x26, 0x81, 0xf5, 0x10, 0xc9, 0xf4, 0x94, 0xb8, 0x55, 0x52, 0x24, 0xeb, 0xaf, 0x89, 0xc3, 0x24, 0x7e,
+		0x32, 0xcf, 0xd5, 0xda, 0xa2, 0xcb, 0x98, 0x67, 0x71, 0xc3, 0xa5, 0xf6, 0xa8, 0xe3, 0x4e, 0x64, 0x23, 0x2e, 0x40, 0xee,
+		0x2e, 0xd9, 0xa4, 0x97, 0x87, 0x83, 0xd4, 0xe7, 0x11, 0xfe, 0xdb, 0xf4, 0x42, 0xc1, 0x71, 0x3b, 0x5a, 0x07, 0x01, 0x76,
+		0xb2, 0xf8, 0x48, 0x23, 0x2d, 0xb3, 0x53, 0x61, 0x98, 0x39, 0x13, 0x7b, 0x45, 0xcd, 0x55, 0x76, 0xbe, 0x71, 0x3a, 0x88,
+		0xf3, 0xce, 0xec, 0xc2, 0x68, 0x7d, 0xfd, 0x38, 0x4d, 0x49, 0xef, 0x57, 0x9a, 0xc7, 0x45, 0x81, 0xe4, 0x6f, 0xab, 0x4b,
+		0x50, 0xa2, 0x43, 0x08, 0x71, 0x78, 0x43, 0xa2,
+		0x66, 0x8e, 0x2b, 0xfd, 0x64, 0xc3, 0xed, 0x09, 0x1f, 0xa6, 0xe8, 0xa2, // mac
+	})
+
+	f.Fuzz(func(t *testing.T, data []byte) {
+		writer := bytes.NewBuffer(nil)
+		_, err := sdk.ReadNanoTDF(writer, bytes.NewReader(data))
+
+		require.Error(t, err) // will always err due to no server running
+		require.Equal(t, 0, writer.Len())
+	})
+}
+
 func FuzzReadPolicyBody(f *testing.F) {
 	pb := &PolicyBody{
 		mode: 0,
@@ -138,3 +197,55 @@ func FuzzNewResourceLocatorFromReader(f *testing.F) {
 		require.NotNil(t, r)
 	})
 }
+
+var attributeSeeds = []string{
+	// select seeds taken from granter_test.go
+	"http://e/attr/a/value/1",
+	"http://e/attr/1/value/one",
+	"http://e/attr/value/value/one",
+	"http://e/attr/a/value/%20",
+	// error cases
+	"http://e/attr",
+	"hxxp://e/attr/a",
+	"https://a/attr/%😁",
+	"http://e/attr/a/value/b",
+	// fuzzer discovered panic cases
+	"http://0/attr//0/value/",
+	"http://e/attr///value/0",
+	"http://e/attr////value/0",
+	"http://0/attr/0//value/0",
+	"http://0/attr//0/value/0",
+	"http://0/attr/0/0/value/0",
+}
+
+func FuzzNewAttributeNameFQN(f *testing.F) {
+	for _, s := range attributeSeeds {
+		f.Add(s)
+	}
+
+	f.Fuzz(func(_ *testing.T, data string) {
+		fqn, err := NewAttributeNameFQN(data)
+		if err == nil { // if possible validate additional functionality does not result in failure
+			_ = fqn.Authority()
+			_ = fqn.Name()
+			_ = fqn.Prefix()
+			_ = fqn.String()
+		}
+	})
+}
+
+func FuzzNewAttributeValueFQN(f *testing.F) {
+	for _, s := range attributeSeeds {
+		f.Add(s)
+	}
+
+	f.Fuzz(func(_ *testing.T, data string) {
+		fqn, err := NewAttributeValueFQN(data)
+		if err == nil {
+			_ = fqn.Authority()
+			_ = fqn.Name()
+			_ = fqn.Prefix()
+			_ = fqn.String()
+		}
+	})
+}

sdk/granter.go

@@ -27,16 +27,28 @@ const (
 	emptyTerm   = "DEFAULT"
 )
 
-// Represents a which KAS a split with the associated ID should shared with.
+// keySplitStep represents a which KAS a split with the associated ID should be shared with.
 type keySplitStep struct {
 	KAS, SplitID string
 }
 
-// Utility type to represent an FQN for an attribute.
+// AttributeNameFQN is a utility type to represent an FQN for an attribute.
 type AttributeNameFQN struct {
 	url, key string
 }
 
+func attributeURLPartsValid(parts []string) error {
+	for i, part := range parts {
+		if part == "" {
+			return fmt.Errorf("%w: empty url path parts are not allowed", ErrInvalid)
+		} else if i > 1 && // skip first two parts as they will have protocol slashes
+			strings.Contains(part, "/") {
+			return fmt.Errorf("%w: slash not allowed in name or values", ErrInvalid)
+		}
+	}
+	return nil
+}
+
 func NewAttributeNameFQN(u string) (AttributeNameFQN, error) {
 	re := regexp.MustCompile(`^(https?://[\w./-]+)/attr/([^/\s]*)$`)
 	m := re.FindStringSubmatch(u)
@@ -48,6 +60,11 @@ func NewAttributeNameFQN(u string) (AttributeNameFQN, error) {
 	if err != nil {
 		return AttributeNameFQN{}, fmt.Errorf("%w: error in attribute name [%s]", ErrInvalid, m[2])
 	}
+
+	if err := attributeURLPartsValid(m); err != nil {
+		return AttributeNameFQN{}, err
+	}
+
 	return AttributeNameFQN{u, strings.ToLower(u)}, nil
 }
 
@@ -86,7 +103,7 @@ func (a AttributeNameFQN) Name() string {
 	return v
 }
 
-// Utility type to represent an FQN for an attribute value.
+// AttributeValueFQN is a utility type to represent an FQN for an attribute value.
 type AttributeValueFQN struct {
 	url, key string
 }
@@ -107,6 +124,10 @@ func NewAttributeValueFQN(u string) (AttributeValueFQN, error) {
 		return AttributeValueFQN{}, fmt.Errorf("%w: error in attribute value [%s]", ErrInvalid, m[3])
 	}
 
+	if err := attributeURLPartsValid(m); err != nil {
+		return AttributeValueFQN{}, err
+	}
+
 	return AttributeValueFQN{u, strings.ToLower(u)}, nil
 }
 
@@ -351,7 +372,7 @@ func (r granter) plan(defaultKas []string, genSplitID func() string) ([]keySplit
 	k = k.reduce()
 	l := k.Len()
 	if l == 0 {
-		// default behavior: split key accross all default kases
+		// default behavior: split key across all default kases
 		switch len(defaultKas) {
 		case 0:
 			return nil, fmt.Errorf("no default KAS specified; required for grantless plans")

sdk/granter_test.go

@@ -290,6 +290,9 @@ func TestAttributeValueFromMalformedURL(t *testing.T) {
 		{"invalid prefix 1", "hxxp://e/attr/a/value/1"},
 		{"invalid prefix 2", "e/attr/a/a/value/1"},
 		{"bad encoding", "https://a/attr/emoji/value/%😁"},
+		{"empty name", "http://e/attr//value/0"},
+		{"slash name", "http://e/attr///value/0"},
+		{"slash in name", "http://0/attr/0/0/value/0"},
 	} {
 		t.Run(tc.n, func(t *testing.T) {
 			a, err := NewAttributeValueFQN(tc.u)