auth: preserve valid authorization header matches - #245
Conversation
|
@starius Could you please take a look when you have a chance? Thanks! |
|
@grandpig, remember to re-request review from reviewers when ready |
Thanks, good catch. The break introduced a regression when an earlier Authorization value matches the permissive regex but contains an invalid macaroon, while a later value is valid. I changed the loop to retain the last syntactically matching LSAT/L402 value while ignoring unrelated Authorization values, preserving the parent implementation's behavior. I also added a regression test for The multi-value compatibility is needed because SetHeader currently emits both LSAT and L402 Authorization values to support older Aperture clients. Supporting unrelated values such as Bearer is an additional compatibility extension, and I added coverage for that behavior as well. |
|
Hmm, so now it permits requests like this: Why are they needed? Note that requests like are already supported. I'm worried that permitting non-conformant requests like ones adding I would go in the opposite direction: reject requests with valid-after-invalid headers. This looks a valid hardening direction to me. What do you think? I.e. permit exactly one of these forms:
Thoughts? |
Agreed. I don't have a concrete compatibility requirement for accepting an unrelated Bearer value, and I agree that allowing it would broaden the accepted request surface unnecessarily. I tightened FromHeader so that every Authorization value must be a valid LSAT or L402 credential. It now accepts exactly one LSAT, one L402, or one of each in either order. Any malformed, unrecognized, duplicate, or extra Authorization value is rejected, including both valid-before-invalid and invalid-before-valid cases. I also added regression tests covering these cases. |
|
@starius Modified. Please review it again. Thansk |
|
There are three blocking gaps.
Therefore this still succeeds: The reverse order fails. So cryptographic invalid-before-valid remains order-dependent. The test uses
|
Thanks, these were good catches. I updated FromHeader so that when both LSAT and L402 are present, their decoded macaroon bytes and decoded preimages must be identical. A credential is no longer allowed to overwrite a previous credential, so mismatched credentials are rejected in either order. I also added tests using structurally valid but cryptographically unauthorized macaroons, including both invalid-before-valid and valid-before-invalid ordering. Finally, I relaxed the parser according to the L402 grammar: authentication schemes are case-insensitive, 1*SP is accepted, and uppercase hexadecimal preimages are accepted. |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
The changes look good. It is a reasonable hardening. Here are some nits in the code. Please fix and squash all the commits and sync PR title and description to the final shape of the PR.
f674c74 to
d5a3a46
Compare
Please review it again. Thnaks. |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Thank you for updates!
I found another hardening vector in this function and also a way to deduplicate some code. Could you fix it, while we are on this, please?
Signed-off-by: grandpig <grandpig@outlook.com>
d5a3a46 to
057a008
Compare
Thanks, fixed. FromHeader now uses header.Values to distinguish an absent header from an empty value. Any supplied empty Authorization value is rejected instead of falling back to Macaroon or Grpc-Metadata-Macaroon. Multiple metadata macaroon headers and multiple Macaroon headers are also rejected. I deduplicated the decoding and validation logic across all supported header formats. When both LSAT and L402 are present, their encoded macaroon and encoded preimage values must match exactly. Please review it again. |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Found a bug and a test coverage gap.
Also could you update the PR title?
"auth: preserve valid authorization header matches" -> "l402: harden authorization header parsing"
| var matches []string | ||
| authHeaders := header.Values(HeaderAuthorization) | ||
| case len(authHeaders) > 0: | ||
| var scheme string |
There was a problem hiding this comment.
Duplicate second scheme is accepted. var scheme remembers only the first scheme. It therefore accepts:
Authorization: L402 M:P
Authorization: LSAT M:P
Authorization: LSAT M:PThe third value is compared only against the first scheme (L402), so it is not detected as a duplicate. The inverse also works:
Authorization: LSAT M:P
Authorization: L402 M:P
Authorization: L402 M:PThis contradicts "exactly one LSAT, one L402, or one of each." Use a seenSchemes map or reject len(authHeaders) > 2.
Add both three-value cases as tests:
diff --git a/auth/authenticator_test.go b/auth/authenticator_test.go
index 48886da..0d8925b 100644
--- a/auth/authenticator_test.go
+++ b/auth/authenticator_test.go
@@ -379,6 +379,34 @@ func TestL402Authenticator(t *testing.T) {
},
result: false,
},
+ {
+ id: "duplicate LSAT after matching L402 and LSAT",
+ header: &http.Header{
+ l402.HeaderAuthorization: []string{
+ "L402 " + testMacBase64 + ":" +
+ testPreimage,
+ "LSAT " + testMacBase64 + ":" +
+ testPreimage,
+ "LSAT " + testMacBase64 + ":" +
+ testPreimage,
+ },
+ },
+ result: false,
+ },
+ {
+ id: "duplicate L402 after matching LSAT and L402",
+ header: &http.Header{
+ l402.HeaderAuthorization: []string{
+ "LSAT " + testMacBase64 + ":" +
+ testPreimage,
+ "L402 " + testMacBase64 + ":" +
+ testPreimage,
+ "L402 " + testMacBase64 + ":" +
+ testPreimage,
+ },
+ },
+ result: false,
+ },
{
id: "extra auth header value",
header: &http.Header{| { | ||
| id: "invalid base64 followed by valid credential", | ||
| header: &http.Header{ | ||
| l402.HeaderAuthorization: []string{ | ||
| "LSAT A:" + testPreimage, | ||
| "L402 " + testMacBase64 + ":" + | ||
| testPreimage, | ||
| }, | ||
| }, | ||
| result: false, | ||
| }, | ||
| { | ||
| id: "valid credential followed by invalid base64", | ||
| header: &http.Header{ | ||
| l402.HeaderAuthorization: []string{ | ||
| "L402 " + testMacBase64 + ":" + | ||
| testPreimage, | ||
| "LSAT A:" + testPreimage, | ||
| }, | ||
| }, | ||
| result: false, | ||
| }, | ||
| { | ||
| id: "invalid encoded macaroon followed by valid credential", | ||
| header: &http.Header{ | ||
| l402.HeaderAuthorization: []string{ | ||
| "LSAT YQ==:" + testPreimage, | ||
| "L402 " + testMacBase64 + ":" + | ||
| testPreimage, | ||
| }, | ||
| }, | ||
| result: false, | ||
| }, | ||
| { | ||
| id: "valid credential followed by invalid encoded macaroon", | ||
| header: &http.Header{ | ||
| l402.HeaderAuthorization: []string{ | ||
| "L402 " + testMacBase64 + ":" + | ||
| testPreimage, | ||
| "LSAT YQ==:" + testPreimage, | ||
| }, | ||
| }, | ||
| result: false, | ||
| }, |
There was a problem hiding this comment.
These test cases now fail at encoded credential comparison:
currentMacBase64 != macBase64They never reach Base64 decoding or Authorization macaroon unmarshalling.
Coverage confirms:
FromHeader: 96.1%- Base64 decoder error at
header.go:138: uncovered - Missing-preimage-caveat error: uncovered, but pre-existing
- Macaroon unmarshal block is covered through the empty metadata-header case, not through
Authorization: L402 YQ==:...
Add single-header cases:
L402 A:<valid-preimage>
L402 YQ==:<valid-preimage>
diff --git a/auth/authenticator_test.go b/auth/authenticator_test.go
index 0d8925b..9b23dbb 100644
--- a/auth/authenticator_test.go
+++ b/auth/authenticator_test.go
@@ -39,6 +39,25 @@ func createDummyMacHexWithID(preimage, id string) string {
return hex.EncodeToString(macBytes)
}
+// createDummyMacHexWithoutPreimage creates a valid macaroon without a
+// preimage caveat.
+func createDummyMacHexWithoutPreimage() string {
+ dummyMac, err := macaroon.New(
+ []byte("aabbccddeeff00112233445566778899"), []byte("AA=="),
+ "aperture", macaroon.LatestVersion,
+ )
+ if err != nil {
+ panic(err)
+ }
+
+ macBytes, err := dummyMac.MarshalBinary()
+ if err != nil {
+ panic(err)
+ }
+
+ return hex.EncodeToString(macBytes)
+}
+
// TestL402Authenticator tests that the authenticator properly handles auth
// headers and the tokens contained in them.
func TestL402Authenticator(t *testing.T) {
@@ -47,10 +66,11 @@ func TestL402Authenticator(t *testing.T) {
"9787b609f088c8df09bacc7b4bd21b39"
otherPreimage = "59349dfea4abed3cd14f6d356afa83de" +
"9787b609f088c8df09bacc7b4bd21b39"
- testMacHex = createDummyMacHex(testPreimage)
- otherIDMacHex = createDummyMacHexWithID(testPreimage, "BB==")
- testMacBytes, _ = hex.DecodeString(testMacHex)
- testMacBase64 = base64.StdEncoding.EncodeToString(
+ testMacHex = createDummyMacHex(testPreimage)
+ noPreimageMacHex = createDummyMacHexWithoutPreimage()
+ otherIDMacHex = createDummyMacHexWithID(testPreimage, "BB==")
+ testMacBytes, _ = hex.DecodeString(testMacHex)
+ testMacBase64 = base64.StdEncoding.EncodeToString(
testMacBytes,
)
otherIDMacBytes, _ = hex.DecodeString(otherIDMacHex)
@@ -112,6 +132,13 @@ func TestL402Authenticator(t *testing.T) {
},
result: false,
},
+ {
+ id: "macaroon header without preimage caveat",
+ header: &http.Header{
+ l402.HeaderMacaroon: []string{noPreimageMacHex},
+ },
+ result: false,
+ },
{
id: "empty auth header with valid fallback macaroon",
header: &http.Header{
@@ -302,6 +329,24 @@ func TestL402Authenticator(t *testing.T) {
},
result: false,
},
+ {
+ id: "invalid base64 credential",
+ header: &http.Header{
+ l402.HeaderAuthorization: []string{
+ "L402 A:" + testPreimage,
+ },
+ },
+ result: false,
+ },
+ {
+ id: "invalid encoded macaroon credential",
+ header: &http.Header{
+ l402.HeaderAuthorization: []string{
+ "L402 YQ==:" + testPreimage,
+ },
+ },
+ result: false,
+ },
{
id: "invalid base64 followed by valid credential",
header: &http.Header{
3 participants
Summary
Details
SetHeaderemits both LSAT and L402 credentials for compatibility with older Aperture versions. When both are present,FromHeaderrequires their encoded macaroon and encoded preimage values to be identical.Aperture currently supports exactly one macaroon per credential. The comma-separated multiple-macaroon form allowed by the L402 specification is not supported.
An Authorization header is considered present even when its value is empty.
This prevents an empty Authorization value from bypassing validation and falling back to another macaroon header.
Testing
go test ./auth ./l402 -count=1go test ./...