aperture: support third-party caveat discharge macaroons#232
Conversation
|
@claude review this |
lightninglabs-deploy
left a comment
lightninglabs-deploy
left a comment
There was a problem hiding this comment.
Solid PR. The approach of using macaroon.Slice as the serialization format is clean and the backwards compatibility story is well thought out.
I verified that Slice.MarshalBinary for a single macaroon produces identical bytes to Macaroon.MarshalBinary (both use appendBinary under the hood), and Slice.UnmarshalBinary is indeed a strict superset of Macaroon.UnmarshalBinary — so existing single-macaroon clients are truly unaffected. 👍
A few observations/minor nits:
1. tokenFromContext discards discharges
In l402/server_interceptor.go, the new _ blank identifier at the call site means that if anyone later needs the token ID from a request that also carries discharges, they'd have to refactor. This is fine for now since tokenFromContext only needs the token ID, but worth noting.
2. TestSetHeaderBackwardsCompatible — consider also checking Slice.MarshalBinary directly
The test currently round-trips through SetHeader/header parsing to check the base64 matches. It might be slightly more robust to also assert:
sliceBytes, _ := macaroon.Slice{mac}.MarshalBinary()
require.Equal(t, macBytes, sliceBytes)
This would make the invariant the test relies on explicit, rather than testing it indirectly through the header format string.
3. HasCaveat only checks root macaroon caveats — correct
Just confirming: HasCaveat iterates m.Caveats() which returns first-party caveats only on the root macaroon, so the preimage extraction in the hex-header path correctly ignores discharge macaroons. Good.
4. Tests are thorough
Nice coverage of:
- Auth header with/without discharges
- Hex header with/without discharges
- Multiple discharge round-trip
- Backwards compatibility
- Mint-level verification with/without discharges
The mint test correctly validates both the positive case (discharge present → verification succeeds) and the negative case (no discharge → "cannot find discharge" error).
Overall this is a clean, well-scoped change. LGTM.
| "unmarshal macaroon: %v", err) | ||
|
|
||
| // Use Slice to unmarshal so we can extract discharge | ||
| // macaroons if present. By convention the first macaroon |
There was a problem hiding this comment.
Interesting, so now there'll be multiple macaroons, space delimited as part of the header? Do we need to update the spec to account for this?
| mac := slice[0] | ||
| var discharges []*macaroon.Macaroon | ||
| if len(slice) > 1 { | ||
| discharges = slice[1:] |
There was a problem hiding this comment.
Re spec again, right now depending on an implicit ordering here.
| mac := slice[0] | ||
| var discharges []*macaroon.Macaroon | ||
| if len(slice) > 1 { | ||
| discharges = slice[1:] |
There was a problem hiding this comment.
Same logic in a few areas, can extract into a helper functio.
| // serialized alongside the root macaroon using the macaroon.Slice convention. | ||
| func SetHeader(header *http.Header, mac *macaroon.Macaroon, | ||
| preimage fmt.Stringer) error { | ||
| preimage fmt.Stringer, discharges []*macaroon.Macaroon) error { |
There was a problem hiding this comment.
Let's make the discharges ...*macaroon.Macaroon`, then we won't break the API.
| if len(matches) != 4 { | ||
| return nil, lntypes.Preimage{}, fmt.Errorf("invalid "+ | ||
| "auth header format: %s", authHeader) | ||
| return nil, lntypes.Preimage{}, nil, |
There was a problem hiding this comment.
Slightly out of scope, but I realize there's a bug here: https://github.com/djkazic/aperture/blob/4e3194e169b577cc63b887331415a533043bab39/l402/header.go#L58-L69
That should be a break.
| return nil, lntypes.Preimage{}, nil, fmt.Errorf("unable to "+ | ||
| "unmarshal macaroon: %v", err) | ||
| } | ||
| if len(slice) == 0 { |
There was a problem hiding this comment.
Should add a test case for this.
|
Will address review comments in the AM. |
3 participants
When a macaroon contains a third-party caveat, the macaroon library's
VerifySignaturewalks the caveat chain and callsfindDischargeto locate the corresponding discharge macaroon. Withnildischarges, this search always fails, returning"cannot find discharge macaroon for caveat ..."which is propagated as a verification error. The library treats a missing discharge as a verification failure, not a skippable condition, so there was no way to verify such macaroons.The fix uses
macaroon.Slice(where the first element is the root macaroon and the rest are discharges) as the serialization format in both the base64Authorizationheader and the hex-encodedGrpc-Metadata-Macaroon/Macaroonheaders.Slice.UnmarshalBinaryis a strict superset ofMacaroon.UnmarshalBinaryfor single-macaroon inputs, so existing clients are unaffected.