security: translate OIDC claims to Biscuit facts dynamically, reorgan…

[aojea]

…ize token logic, and add specifications references

[gemini-code-assist]

Code Review

This pull request refactors the Biscuit token minting and verification logic from main.go into dedicated biscuit.go and biscuit_test.go files, introduces dynamic OIDC claims-to-facts mapping, and updates the policy documentation. Feedback on these changes highlights several improvement opportunities: preventing a potential nil pointer dereference on token in mintBiscuitToken, handling non-slice representations of roles and groups claims gracefully, parsing static Datalog rules outside of the public key verification loop to improve efficiency, and correcting outdated file paths and function names in the documentation comments of api/constants.go.

[gemini-code-assist]

The documentation comments refer to non-existent files and incorrect function names. Specifically, TranslateClaimsToFacts does not exist (it is translateClaimsToFacts), and there are no api/translation.go or api/translation_test.go files in the repository (the implementation and tests are in cmd/sam-hub/biscuit.go and cmd/sam-hub/biscuit_test.go respectively). Let's update the comments to match the actual codebase structure.

Suggested change

	// How to add a new translation:
	// 1. Define a constant for the Biscuit fact name in the "Biscuit fact names" block above
	// (e.g., FactMyNewClaim = "my_new_fact").
	// 2. Add an entry to the OIDCClaimToFact map below (e.g., "my_oidc_claim": FactMyNewClaim).
	// 3. Update TranslateClaimsToFacts in api/translation.go to handle parsing/type conversion
	// for the new fact if it uses a custom format (e.g. integer, date, list).
	// 4. Implement unit tests in api/translation_test.go covering the new mapping.
	// How to add a new translation:
	// 1. Define a constant for the Biscuit fact name in the "Biscuit fact names" block above
	// (e.g., FactMyNewClaim = "my_new_fact").
	// 2. Add an entry to the OIDCClaimToFact map below (e.g., "my_oidc_claim": FactMyNewClaim).
	// 3. Update translateClaimsToFacts in cmd/sam-hub/biscuit.go to handle parsing/type conversion
	// for the new fact if it uses a custom format (e.g. integer, date, list).
	// 4. Implement unit tests in cmd/sam-hub/biscuit_test.go covering the new mapping.

[aojea]

/gemini review again

[gemini-code-assist]

Code Review

This pull request refactors the Biscuit token minting and verification logic by moving it into dedicated files (biscuit.go and biscuit_test.go), introduces dynamic OIDC claims-to-facts mapping, and updates documentation and manifests to reflect port and flag changes. The review feedback focuses on improving robustness, determinism, and performance by suggesting a nil check on claims to prevent panics, sorting map keys (roles and claims) to ensure deterministic token serialization, and pre-parsing static Datalog rules to reduce CPU overhead during verification.

[aojea]

/gemini review again

[gemini-code-assist]

Code Review

This pull request refactors the Biscuit token minting and verification logic into a dedicated biscuit.go file, introduces benchmark and unit tests, and updates documentation and manifests to reflect port changes (from 8080/4002 to 9090) and command usage. Feedback focuses on addressing concurrency and security risks associated with the mutable global map OIDCClaimToFact in api/constants.go. Exposing this map directly can lead to race conditions during concurrent iterations in translateClaimsToFacts; it is recommended to make the map package-private and expose a thread-safe copy-accessor function instead.

[aojea]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors Biscuit token minting and verification from main.go into a dedicated biscuit.go file, introducing dynamic OIDC claims-to-facts mapping, comprehensive unit tests, benchmarks, and updated documentation. Feedback on these changes highlights two critical reliability issues: first, failures when adding MCP servers or network targets during token minting are logged but otherwise ignored, which can produce partially constructed tokens; second, errors from adding facts, checks, and policies during verification are unhandled, potentially leading to silent authorization failures or bypasses.

[gemini-code-assist]

If builder.AddAuthorityFact fails when adding allowed MCP servers or network targets, the error is logged but the token minting process continues. This results in a partially constructed Biscuit token that is missing critical authorization facts, which will cause downstream authorization failures for the client. We should return the error and fail the token minting process.

Suggested change

	for _, tool := range rolePolicy.MCP.AllowedServers {
	if err := builder.AddAuthorityFact(biscuit.Fact{Predicate: biscuit.Predicate{
	Name: api.FactMCPServer,
	IDs: []biscuit.Term{biscuit.String(tool)},
	}}); err != nil {
	logger.Errorw("Failed to add MCP tool fact to biscuit", "peer_id", remotePeer, "tool", tool, "error", err)
	}
	}
	for _, target := range rolePolicy.Network.AllowedTargets {
	if err := builder.AddAuthorityFact(biscuit.Fact{Predicate: biscuit.Predicate{
	Name: api.FactNetworkTarget,
	IDs: []biscuit.Term{biscuit.String(target)},
	}}); err != nil {
	logger.Errorw("Failed to add network target fact to biscuit", "peer_id", remotePeer, "target", target, "error", err)
	}
	}
	for _, tool := range rolePolicy.MCP.AllowedServers {
	if err := builder.AddAuthorityFact(biscuit.Fact{Predicate: biscuit.Predicate{
	Name: api.FactMCPServer,
	IDs: []biscuit.Term{biscuit.String(tool)},
	}}); err != nil {
	return nil, fmt.Errorf("failed to add MCP tool fact: %w", err)
	}
	}
	for _, target := range rolePolicy.Network.AllowedTargets {
	if err := builder.AddAuthorityFact(biscuit.Fact{Predicate: biscuit.Predicate{
	Name: api.FactNetworkTarget,
	IDs: []biscuit.Term{biscuit.String(target)},
	}}); err != nil {
	return nil, fmt.Errorf("failed to add network target fact: %w", err)
	}
	}

[gemini-code-assist]

The errors returned by authorizer.AddFact, authorizer.AddCheck, and authorizer.AddPolicy are ignored. If any of these operations fail, the authorizer will be configured incorrectly, which could lead to silent authorization bypasses or failures. We should check and handle these errors.

		if err := authorizer.AddFact(biscuit.Fact{
			Predicate: biscuit.Predicate{
				Name: "time",
				IDs:  []biscuit.Term{biscuit.Date(time.Now())},
			},
		}); err != nil {
			lastErr = err
			continue
		}

		if err := authorizer.AddCheck(staticTimeCheck); err != nil {
			lastErr = err
			continue
		}
		if err := authorizer.AddPolicy(staticAllowPolicy); err != nil {
			lastErr = err
			continue
		}

[aojea]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the Biscuit token minting and verification logic by moving it from cmd/sam-hub/main.go into a new dedicated file cmd/sam-hub/biscuit.go, along with comprehensive unit tests and benchmarks. It also introduces dynamic OIDC claim-to-fact mapping, defines a central CatalogTarget constant, and updates the documentation to align with these changes. The review feedback highlights opportunities to prevent minting invalid empty roles, improve error handling when no public keys are found in the keyring, and correct a misleading comment in the test suite.

[gemini-code-assist]

If b.Role is empty in the policy bindings, an empty role string "" will be added to resolvedRoles, resulting in an empty role fact role("") being added to the Biscuit token. We should filter out empty roles to prevent minting tokens with invalid empty roles.

Suggested change

	roles := make([]string, 0, len(resolvedRoles))
	for role := range resolvedRoles {
	roles = append(roles, role)
	}
	roles := make([]string, 0, len(resolvedRoles))
	for role := range resolvedRoles {
	if role != "" {
	roles = append(roles, role)
	}
	}

[gemini-code-assist]

If keys is empty (i.e., no valid public keys are found in the keyring), lastErr will remain nil, and the function will return a confusing error message: "no valid key found for verification: <nil>". We should explicitly check if keys is empty first to provide a clearer diagnostic error.

	keys := h.KeyRing.GetAllValidPublicKeys()
	if len(keys) == 0 {
		return nil, fmt.Errorf("no valid public keys found in keyring")
	}
	var lastErr error

[gemini-code-assist]

The comment says // Verify that it mapped the derived group "system:serviceaccounts:sam-canary" to "canary-role", but in Case 4, the mapping actually happens via the direct user binding User: "system:serviceaccount:sam-canary:sam-node-sa". The comment is misleading and should be corrected to avoid confusion for future maintainers.

Suggested change

	// Verify that it mapped the derived group "system:serviceaccounts:sam-canary" to "canary-role"
	// Verify that it mapped the user "system:serviceaccount:sam-canary:sam-node-sa" to "canary-role"