etag: fix incorrect multipart detection (#14631)

This commit fixes a subtle bug in the ETag
`IsEncrypted` implementation.

An encrypted ETag may contain random bytes,
i.e. some randomness used for encryption.
This random value can contain a '-' byte
simple due to being randomly generated.

Before, the `IsEncrypted` implementation
incorrectly assumed that an encrypted ETag
cannot contain a '-' since it would be a
multipart ETag. Multipart ETags have a
16 byte value followed by a '-' and the part number.
For example:
```
059ba80b807c3c776fb3bcf3f33e11ae-2
```

However, the following encrypted ETag
```
20000f00db2d90a7b40782d4cff2b41a7799fc1e7ead25972db65150118dfbe2ba76a3c002da28f85c840cd2001a28a9
```
also contains a '-' byte but is not a multipart ETag.

This commit fixes the `IsEncrypted` implementation
simply by checking whether the ETag is at least 32
bytes long. A valid multipart ETag is never 32 bytes
long since a part number must be <= 10000.

However, an encrypted ETag must be at least 32 bytes
long. It contains the encrypted ETag bytes (16 bytes)
and the authentication tag added by the AEAD cipher (again
16 bytes).

Signed-off-by: Andreas Auernhammer <hi@aead.dev>

internal/etag/etag.go

@@ -143,7 +143,22 @@ func (e ETag) String() string {
 
 // IsEncrypted reports whether the ETag is encrypted.
 func (e ETag) IsEncrypted() bool {
-	return len(e) > 16 && !bytes.ContainsRune(e, '-')
+	// An encrypted ETag must be at least 32 bytes long.
+	// It contains the encrypted ETag value + an authentication
+	// code generated by the AEAD cipher.
+	//
+	// Here is an incorrect implementation of IsEncrypted:
+	//
+	//   return len(e) > 16 && !bytes.ContainsRune(e, '-')
+	//
+	// An encrypted ETag may contain some random bytes - e.g.
+	// and nonce value. This nonce value may contain a '-'
+	// just by its nature of being randomly generated.
+	// The above implementation would incorrectly consider
+	// such an ETag (with a nonce value containing a '-')
+	// as non-encrypted.
+
+	return len(e) >= 32 // We consider all ETags longer than 32 bytes as encrypted
 }
 
 // IsMultipart reports whether the ETag belongs to an
@@ -151,7 +166,7 @@ func (e ETag) IsEncrypted() bool {
 // API.
 // An S3 multipart ETag has a -<part-number> suffix.
 func (e ETag) IsMultipart() bool {
-	return len(e) > 16 && bytes.ContainsRune(e, '-')
+	return len(e) > 16 && !e.IsEncrypted() && bytes.ContainsRune(e, '-')
 }
 
 // Parts returns the number of object parts that are

internal/etag/etag_test.go

@@ -78,6 +78,10 @@ var stringTests = []struct {
 		ETag:   ETag{144, 64, 44, 120, 210, 220, 205, 222, 225, 233, 232, 98, 34, 206, 44, 99, 97, 103, 95, 53, 41, 210, 96, 0, 174, 46, 144, 15, 242, 22, 179, 203, 89, 225, 48, 224, 146, 216, 162, 152, 30, 119, 111, 77, 11, 214, 9, 65},
 		String: "90402c78d2dccddee1e9e86222ce2c6361675f3529d26000ae2e900ff216b3cb59e130e092d8a2981e776f4d0bd60941",
 	},
+	{ // 5
+		ETag:   ETag{32, 0, 15, 0, 219, 45, 144, 167, 180, 7, 130, 212, 207, 242, 180, 26, 119, 153, 252, 30, 126, 173, 37, 151, 45, 182, 81, 80, 17, 141, 251, 226, 186, 118, 163, 192, 2, 218, 40, 248, 92, 132, 12, 210, 0, 26, 40, 169},
+		String: "20000f00db2d90a7b40782d4cff2b41a7799fc1e7ead25972db65150118dfbe2ba76a3c002da28f85c840cd2001a28a9",
+	},
 }
 
 func TestString(t *testing.T) {
@@ -185,6 +189,30 @@ func TestMultipart(t *testing.T) {
 	}
 }
 
+var isEncryptedTests = []struct {
+	ETag        string
+	IsEncrypted bool
+}{
+	{ETag: "20000f00db2d90a7b40782d4cff2b41a7799fc1e7ead25972db65150118dfbe2ba76a3c002da28f85c840cd2001a28a9", IsEncrypted: true}, // 0
+
+	{ETag: "3b83ef96387f14655fc854ddc3c6bd57"},       // 1
+	{ETag: "7b976cc68452e003eec7cb0eb631a19a-1"},     // 2
+	{ETag: "a7d414b9133d6483d9a1c4e04e856e3b-2"},     // 3
+	{ETag: "7b976cc68452e003eec7cb0eb631a19a-10000"}, // 4
+}
+
+func TestIsEncrypted(t *testing.T) {
+	for i, test := range isEncryptedTests {
+		tag, err := Parse(test.ETag)
+		if err != nil {
+			t.Fatalf("Test %d: failed to parse ETag: %v", i, err)
+		}
+		if isEncrypted := tag.IsEncrypted(); isEncrypted != test.IsEncrypted {
+			t.Fatalf("Test %d: got '%v' - want '%v'", i, isEncrypted, test.IsEncrypted)
+		}
+	}
+}
+
 var fromContentMD5Tests = []struct {
 	Header     http.Header
 	ETag       ETag