Fix missing expiration checks

[aojea]

Thanks also to @kaisoz for digging deeper, there were several paths were the biscuit expiration time was not checked , hence allowing attacks based on that.

Fixes #296

[gemini-code-assist]

Code Review

This pull request introduces a configurable Biscuit TTL (--biscuit-ttl) and ensures that a Biscuit's expiration is capped by the OIDC token's expiry or the OIDC session's end. It fixes a security issue where expired tokens were accepted on the node dataplane by implementing and enforcing expiration checks across all verification paths. Additionally, it drops prior admissions when a peer is banned to prevent revoked peers from retaining relay rights. Feedback on the changes highlights a security concern where expired peers might retain relay rights indefinitely in authPeers due to a lack of cleanup mechanisms, and suggests storing expiration times in authPeers to evict expired entries. It also recommends handling errors from AddFact and AddCheck within EnforceExpiration to prevent potential silent security bypasses.

[gemini-code-assist]

Security Issue: Expired peers retain relay rights indefinitely in authPeers

Currently, when a peer successfully authenticates via HandleAuthHandshake, it is stored in n.authPeers with a value of true (on line 1431, which is outside this diff). However, there is no mechanism to clean up or re-verify these entries when the peer's Biscuit token expires. As a result, once a peer has authenticated once, it retains relay rights indefinitely in nodeRelayACL (which only checks authPeers), completely bypassing the short-lived token expiration security model.

Suggested Fix:

1. Modify verifyBiscuit to extract and return the Biscuit's expiration time.
2. Store the expiration time in n.authPeers instead of true.
3. Update nodeRelayACL (AllowReserve and AllowConnect) to check and delete expired entries.

Here is how you can implement this:

In verifyBiscuit (this function):

func (n *SamNode) verifyBiscuit(biscuitData []byte, remotePeer peer.ID) (*biscuit.Biscuit, time.Time, error) {
    // ... (unmarshal and loop)
        identity.EnforceExpiration(authorizer)
        authorizer.AddPolicy(api.AllowIfTruePolicy)

        if err := authorizer.Authorize(); err == nil {
            var exp time.Time
            rule := biscuit.Rule{
                Head: biscuit.Predicate{Name: "get_exp", IDs: []biscuit.Term{biscuit.Variable("e")}},
                Body: []biscuit.Predicate{{Name: api.FactExpiration, IDs: []biscuit.Term{biscuit.Variable("e")}}},
            }
            if facts, err := authorizer.Query(rule); err == nil && len(facts) > 0 && len(facts[0].IDs) > 0 {
                if date, ok := facts[0].IDs[0].(biscuit.Date); ok {
                    exp = time.Time(date)
                }
            }
            return b, exp, nil
        }
    // ...
}

In HandleAuthHandshake:

    b, exp, err := n.verifyBiscuit(exchange.Biscuit, remotePeer)
    // ...
    n.authPeers.Store(remotePeer, exp)

In nodeRelayACL (around line 118):

func (a *nodeRelayACL) AllowReserve(p peer.ID, addr multiaddr.Multiaddr) bool {
    val, ok := a.node.authPeers.Load(p)
    if !ok {
        return false
    }
    if exp, ok := val.(time.Time); ok && !exp.IsZero() && time.Now().After(exp) {
        a.node.authPeers.Delete(p)
        return false
    }
    return true
}

[aojea]

Good catch, and this is the same bug one layer up: enforcing expiry at admission is worth little if the admission itself never expires. Fixed in 61d2fc7.

Took the shape you suggested, with two changes:

• verifyBiscuit returns an error rather than a zero time when it cannot extract an expiration. EnforceExpiration already requires an expiration() fact for the token to authorize at all, so a token that authorizes but carries no such fact is a contradiction, not a peer to admit with an open-ended entry.
• The AllowReserve sketch falls open on a type mismatch: if exp, ok := val.(time.Time); ok && ... returns true when the assertion fails. That is not hypothetical here, since the old code stored true and the tests stored struct{}{}. The version I landed treats any non-time.Time value as a bug and denies, with a regression test for it.

Extraction lives in identity.ExpirationOf, next to EnforceExpiration, so expiry logic stays in one place, and it takes the earliest expiration() fact in case an attenuated token narrows it.

[gemini-code-assist]

Defensive Programming: Unhandled errors from AddFact and AddCheck

The AddFact and AddCheck methods return an error. While they are highly unlikely to fail with programmatic inputs, ignoring these errors could lead to silent security bypasses if the authorizer is in an invalid state or if internal limits are exceeded.

Consider handling these errors or panicking to fail-closed:

func EnforceExpiration(authorizer biscuit.Authorizer) {
	if err := authorizer.AddFact(biscuit.Fact{
		Predicate: biscuit.Predicate{
			Name: api.FactTime,
			IDs:  []biscuit.Term{biscuit.Date(time.Now())},
		},
	}); err != nil {
		panic(fmt.Sprintf("failed to add time fact: %v", err))
	}
	if err := authorizer.AddCheck(api.ControlPlaneStaticTimeCheck); err != nil {
		panic(fmt.Sprintf("failed to add static time check: %v", err))
	}
}

[aojea]

This one is a false positive: Authorizer.AddFact and AddCheck return nothing, so there is no error to handle.

From biscuit-go/v2@v2.2.0/authorizer.go:

AddFact(fact Fact)
AddRule(rule Rule)
AddCheck(check Check)
AddPolicy(policy Policy)

The suggestion looks like a mix-up with Builder.AddAuthorityFact, which does return an error and which we do check at every call site. errcheck is one of the active linters, so an unchecked error here would not have got past make lint either.

Leaving EnforceExpiration as is. The suggested panic would compile only after inventing an error that cannot occur.

[aojea]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a configurable Biscuit TTL and resolves a security vulnerability (#296) by ensuring that Biscuit token expiration is strictly enforced on the node dataplane. It adds the EnforceExpiration helper to inject the current time and static time checks into the authorizer. The review feedback highlights critical compilation errors due to incorrect field access on biscuit.Fact in biscuit-go v2, recommends handling errors returned by EnforceExpiration and AddPolicy to prevent silent failures, and suggests adding a nil check for the token before accessing its expiry to avoid potential panics.

[gemini-code-assist]

In biscuit-go v2, biscuit.Fact has a named Predicate field of type Predicate. It does not embed Predicate anonymously, so accessing fact.IDs directly will result in a compilation error. You must access IDs via fact.Predicate.IDs.

Suggested change

	for _, fact := range facts {
	if len(fact.IDs) != 1 {
	continue
	}
	date, ok := fact.IDs[0].(biscuit.Date)
	if !ok {
	continue
	}
	for _, fact := range facts {
	if len(fact.Predicate.IDs) != 1 {
	continue
	}
	date, ok := fact.Predicate.IDs[0].(biscuit.Date)
	if !ok {
	continue
	}

[aojea]

biscuit.Fact embeds Predicate anonymously, so fact.IDs is a promoted field selector and compiles fine.

biscuit-go/v2@v2.2.0/types.go:116:

type Fact struct {
	Predicate
}

Field promotion applies. go build ./... and go vet ./... are green on this branch.

Applying this suggestion would actually break the build here, because staticcheck is an active linter and QF1008 rejects the longer form. That rule is what made me write it this way in the first place:

internal/controlplane/biscuit_ttl_test.go:64:23: QF1008: could remove embedded field "Predicate" from selector (staticcheck)

QF1008 only fires on embedded fields, so the diagnostic is itself proof that the field is embedded. No change.

[gemini-code-assist]

Similar to the issue in internal/identity/biscuit.go, biscuit.Fact has a named Predicate field. Accessing facts[0].IDs directly will cause a compilation error. Please access it via facts[0].Predicate.IDs.

Suggested change

	if len(facts) != 1 || len(facts[0].IDs) != 1 {
	t.Fatalf("expected exactly one expiration fact, got %v", facts)
	}
	date, ok := facts[0].IDs[0].(biscuit.Date)
	if len(facts) != 1 || len(facts[0].Predicate.IDs) != 1 {
	t.Fatalf("expected exactly one expiration fact, got %v", facts)
	}
	date, ok := facts[0].Predicate.IDs[0].(biscuit.Date)

[aojea]

biscuit.Fact embeds Predicate anonymously, so fact.IDs is a promoted field selector and compiles fine.

biscuit-go/v2@v2.2.0/types.go:116:

type Fact struct {
	Predicate
}

Field promotion applies. go build ./... and go vet ./... are green on this branch.

Applying this suggestion would actually break the build here, because staticcheck is an active linter and QF1008 rejects the longer form. That rule is what made me write it this way in the first place:

internal/controlplane/biscuit_ttl_test.go:64:23: QF1008: could remove embedded field "Predicate" from selector (staticcheck)

QF1008 only fires on embedded fields, so the diagnostic is itself proof that the field is embedded. No change.

[gemini-code-assist]

In biscuit-go, AddFact and AddCheck return an error. Ignoring these errors can lead to silent failures where expiration checks are not properly enforced. Update EnforceExpiration to return an error so that callers can handle failures appropriately.

Suggested change

	func EnforceExpiration(authorizer biscuit.Authorizer) {
	authorizer.AddFact(biscuit.Fact{
	Predicate: biscuit.Predicate{
	Name: api.FactTime,
	IDs: []biscuit.Term{biscuit.Date(time.Now())},
	},
	})
	authorizer.AddCheck(api.ControlPlaneStaticTimeCheck)
	}
	func EnforceExpiration(authorizer biscuit.Authorizer) error {
	if err := authorizer.AddFact(biscuit.Fact{
	Predicate: biscuit.Predicate{
	Name: api.FactTime,
	IDs: []biscuit.Term{biscuit.Date(time.Now())},
	},
	}); err != nil {
	return err
	}
	return authorizer.AddCheck(api.ControlPlaneStaticTimeCheck)
	}

[aojea]

Authorizer.AddFact, AddCheck and AddPolicy all return nothing, so there is no error to propagate and EnforceExpiration has nothing to return.

biscuit-go/v2@v2.2.0/authorizer.go:22:

AddFact(fact Fact)
AddRule(rule Rule)
AddCheck(check Check)
AddPolicy(policy Policy)
Authorize() error
Query(rule Rule) (FactSet, error)

Authorize and Query are the two that return errors, and both are checked at every call site.

This looks like a mix-up with Builder.AddAuthorityFact, which does return an error and which we check everywhere. errcheck is also among the active linters, so an unchecked error could not have survived make lint.

These suggestions do not compile. No change.

[gemini-code-assist]

Handle the errors returned by EnforceExpiration and AddPolicy to ensure that failures during policy and expiration enforcement are not silently ignored.

		if err := EnforceExpiration(authorizer); err != nil {
			lastErr = err
			continue
		}
		if err := authorizer.AddPolicy(api.AllowIfTruePolicy); err != nil {
			lastErr = err
			continue
		}

[aojea]

Authorizer.AddFact, AddCheck and AddPolicy all return nothing, so there is no error to propagate and EnforceExpiration has nothing to return.

biscuit-go/v2@v2.2.0/authorizer.go:22:

AddFact(fact Fact)
AddRule(rule Rule)
AddCheck(check Check)
AddPolicy(policy Policy)
Authorize() error
Query(rule Rule) (FactSet, error)

Authorize and Query are the two that return errors, and both are checked at every call site.

This looks like a mix-up with Builder.AddAuthorityFact, which does return an error and which we check everywhere. errcheck is also among the active linters, so an unchecked error could not have survived make lint.

These suggestions do not compile. No change.

[gemini-code-assist]

Handle the error returned by EnforceExpiration to prevent proceeding with verification if the expiration check fails to be added.

Suggested change

	if enforceExpiry {
	EnforceExpiration(auth)
	}
	if enforceExpiry {
	if err := EnforceExpiration(auth); err != nil {
	lastErr = err
	continue
	}
	}

[aojea]

Authorizer.AddFact, AddCheck and AddPolicy all return nothing, so there is no error to propagate and EnforceExpiration has nothing to return.

biscuit-go/v2@v2.2.0/authorizer.go:22:

AddFact(fact Fact)
AddRule(rule Rule)
AddCheck(check Check)
AddPolicy(policy Policy)
Authorize() error
Query(rule Rule) (FactSet, error)

Authorize and Query are the two that return errors, and both are checked at every call site.

This looks like a mix-up with Builder.AddAuthorityFact, which does return an error and which we check everywhere. errcheck is also among the active linters, so an unchecked error could not have survived make lint.

These suggestions do not compile. No change.

[gemini-code-assist]

Handle the error returned by EnforceExpiration to ensure that any failure to inject the expiration check is caught and handled.

Suggested change

	identity.EnforceExpiration(authorizer)
	if err := identity.EnforceExpiration(authorizer); err != nil {
	return fmt.Errorf("failed to enforce expiration: %w", err)
	}

[aojea]

Authorizer.AddFact, AddCheck and AddPolicy all return nothing, so there is no error to propagate and EnforceExpiration has nothing to return.

biscuit-go/v2@v2.2.0/authorizer.go:22:

AddFact(fact Fact)
AddRule(rule Rule)
AddCheck(check Check)
AddPolicy(policy Policy)
Authorize() error
Query(rule Rule) (FactSet, error)

Authorize and Query are the two that return errors, and both are checked at every call site.

This looks like a mix-up with Builder.AddAuthorityFact, which does return an error and which we check everywhere. errcheck is also among the active linters, so an unchecked error could not have survived make lint.

These suggestions do not compile. No change.

[gemini-code-assist]

Handle the errors returned by EnforceExpiration and AddPolicy to ensure that failures during policy and expiration enforcement are not silently ignored.

		if err := identity.EnforceExpiration(authorizer); err != nil {
			lastErr = err
			continue
		}
		if err := authorizer.AddPolicy(api.AllowIfTruePolicy); err != nil {
			lastErr = err
			continue
		}

[aojea]

Authorizer.AddFact, AddCheck and AddPolicy all return nothing, so there is no error to propagate and EnforceExpiration has nothing to return.

biscuit-go/v2@v2.2.0/authorizer.go:22:

AddFact(fact Fact)
AddRule(rule Rule)
AddCheck(check Check)
AddPolicy(policy Policy)
Authorize() error
Query(rule Rule) (FactSet, error)

Authorize and Query are the two that return errors, and both are checked at every call site.

This looks like a mix-up with Builder.AddAuthorityFact, which does return an error and which we check everywhere. errcheck is also among the active linters, so an unchecked error could not have survived make lint.

These suggestions do not compile. No change.

[gemini-code-assist]

To prevent potential nil pointer dereference panics, verify that token is not nil before accessing token.Expiry.

Suggested change

	if token.Expiry.Before(biscuitExpiry) {
	biscuitExpiry = token.Expiry
	}
	if token != nil && token.Expiry.Before(biscuitExpiry) {
	biscuitExpiry = token.Expiry
	}

[aojea]

token cannot be nil at this point.

identity.VerifyJWT returns nil, nil, err on every failure path, and on success returns the non-nil *oidc.IDToken that verifier.Verify produced (internal/identity/oidc.go:94-99). HandleRegister returns immediately when it errors (server.go:440-444), so reaching this line means verification succeeded and token is non-nil.

The guard would be unreachable. Repo guidance is to validate at real boundaries rather than defend against states that cannot occur, and an unreachable nil check here would also suggest to a reader that a nil token is expected, which would be misleading. If VerifyJWT ever grew a claims, nil, nil path, the fix would belong there, where the invariant lives.

No change.