Decouple control plane and router from monolithic sam-hub - #191
Conversation
This refactors the monolithic sam-hub into two separate, decoupled services: 1. sam-control-plane: Manages node/router enrollment, keys database, and policy validation via standard HTTP APIs. 2. sam-router: libp2p host routing requests and acting as a proxy/relay in the mesh. Changes: - Replaced cmd/sam-hub with cmd/sam-control-plane and cmd/sam-router. - Split GKE/Kind deployments and templates in .github/k8s and development/kind to manage standalone control plane and router resources. - Adapted E2E tests (container_mesh.bash, key_rotation_test.bats, revocation_test.bats, relay.bats) to use decoupled control plane and router targets. - Introduced dual-advertisement (DNS and IP-based external addresses) in sam-router to support local container network resolution under isolated bridge environments. - Corrected SQLite data directory mounts in standalone Docker helper runs. - Resolved token audiences validation in OIDC testing setup.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restructures the architecture by splitting the monolithic sam-hub into a separate sam-control-plane and sam-router, updating the build configurations, Kubernetes templates, and E2E tests accordingly. The review feedback highlights critical persistence issues, specifically that both the control plane database and the router's private keys are configured with ephemeral emptyDir volumes. Additionally, the reviewer notes that the default public-mesh role is overly permissive, a potential panic exists in MintBiscuitToken due to a missing nil check on claims, and a non-existent Go version is specified in the Dockerfile. Other recommendations include refining the PeerID extraction regex and restoring the configurability of BiscuitTimeout and DefaultHubURL in the node options.
| # Get the PeerID by extracting the base58 string from the protobuf response | ||
| PEER_ID=$(curl -s http://${POD_IP}:9090/info | grep -a -o '/p2p/[1-9A-HJ-NP-Za-km-z]*' | head -n 1 | cut -d/ -f3 || true) | ||
| # Get the PeerID by extracting it from the router pod logs | ||
| PEER_ID=$(kubectl logs $pod -n ${NAMESPACE} | grep -o 'PeerID: [a-zA-Z0-9]*' | head -n 1 | cut -d' ' -f2 || true) |
There was a problem hiding this comment.
The regex [a-zA-Z0-9]* used to extract the PeerID is too broad and not specific to Base58 encoding. This could potentially match other alphanumeric strings in the logs. A more specific regex should be used to ensure only valid PeerIDs are matched. For example, [1-9A-HJ-NP-Za-km-z]+ is more accurate for Base58.
Also, fetching all logs with kubectl logs can be inefficient for pods with a large amount of log history. Consider using flags like --tail if the PeerID is always logged at the end, or other mechanisms to limit the amount of logs fetched.
PEER_ID=$(kubectl logs $pod -n ${NAMESPACE} | grep -o 'PeerID: [1-9A-HJ-NP-Za-km-z]\+' | head -n 1 | cut -d' ' -f2 || true)There was a problem hiding this comment.
it is prepended with PeerID so it seems safe to me
| logger: golog.Logger("sam-node"), | ||
| } | ||
| node.BiscuitTimeout = cfg.BiscuitTimeout | ||
| node.BiscuitTimeout = 2 * time.Second |
There was a problem hiding this comment.
The BiscuitTimeout is now hardcoded to 2 * time.Second. Previously, this was configurable via node.Options. Hardcoding this value removes flexibility and makes it harder to adjust for different network conditions or security requirements without changing the code.
Please consider re-introducing BiscuitTimeout to the Options struct to make it configurable from the outside.
…ication - Converted the GKE control plane template to deploy a dedicated PostgreSQL StatefulSet with persistent volume storage, and scaled the control plane Deployment to 2 stateless replicas. - Added a PostgreSQL database StatefulSet (with PVC) to the local Kind deployment configuration under development/kind/10-control-plane.yaml, and updated the control plane service to run 2 replicas. - Added wait readiness check for the postgres database in development/kind/run.sh. - Updated default production router lease TTL to 15 minutes in controlplane config and CLI flags. - Configured E2E test fixtures (sam-hub.yaml) and standalone tests (container_mesh.bash) to use aggressive 15s lease duration and 5s renewal intervals. - Applied cleanups to sam-node client options and biscuit token timeouts.
…timezone failures
…t datalog timeouts
|
/review again |
…ats to fast Go integration tests
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the monolithic sam-hub with a decoupled architecture consisting of a stateless sam-control-plane and a dedicated sam-router for GossipSub routing overlays, supported by a SQL-based storage layer. The review feedback identifies critical security and robustness issues, including unauthenticated access to the /policies endpoint, the omission of banned-node checks during enrollment, potential panics from unsafe type assertions on http.DefaultTransport, and indefinite hangs during startup due to missing database query timeouts. Additionally, the feedback addresses a key rotation bug where VerifyBiscuit must return the specific public key that successfully verified the token to ensure compatibility with rotated keys during the grace period.
| } | ||
|
|
||
| // HandlePolicies HTTP GET/POST/PUT `/policies` | ||
| func (s *Server) HandlePolicies(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
The /policies endpoint allows unauthenticated clients to read and overwrite the global mesh policies via GET, POST, and PUT requests. This is a critical security vulnerability as any attacker with network access to the control plane can modify the policies to grant themselves administrative privileges.
Additionally, the AdminToken field is defined as a CLI flag but is completely missing from controlplane.Options and is never passed or verified.
Please add AdminToken to controlplane.Options, pass it from main.go, and enforce token verification in HandlePolicies.
func (s *Server) HandlePolicies(w http.ResponseWriter, r *http.Request) {
if s.config.AdminToken != "" {
authHeader := r.Header.Get("Authorization")
if authHeader != "Bearer "+s.config.AdminToken {
http.Error(w, "Unauthorized", http.StatusUnauthorized)
return
}
}| PolicyPath string // Optional: path to bootstrap policy configuration | ||
| } |
There was a problem hiding this comment.
Add the AdminToken field to Options to support authenticating policy REST API requests.
| PolicyPath string // Optional: path to bootstrap policy configuration | |
| } | |
| PolicyPath string // Optional: path to bootstrap policy configuration | |
| AdminToken string // Token for authenticating admin REST API requests | |
| } |
| PolicyPath: policyFile, | ||
| BiscuitTimeout: 10 * time.Second, | ||
| } |
There was a problem hiding this comment.
Pass the parsed adminToken flag to the control plane server options.
| PolicyPath: policyFile, | |
| BiscuitTimeout: 10 * time.Second, | |
| } | |
| PolicyPath: policyFile, | |
| BiscuitTimeout: 10 * time.Second, | |
| AdminToken: adminToken, | |
| } |
| // 1. The token is cryptographically signed by one of the trustedPublicKeys. | ||
| // 2. The token is not expired. | ||
| // 3. The token is securely bound to the expected remotePeer. | ||
| func VerifyBiscuit(biscuitData []byte, expectedPeer peer.ID, trustedPublicKeys []ed25519.PublicKey, timeout time.Duration) (*biscuit.Biscuit, error) { |
There was a problem hiding this comment.
During key rotation, the control plane maintains historical but still valid public keys within the grace period. While VerifyBiscuit correctly loops through all trustedPublicKeys to find a match, it only returns (*biscuit.Biscuit, error).
This forces callers (such as HandleRouterLease in the control plane, performHubAuthHandshake in sam-node, and performMutualAuth in sam-router) to construct the Authorizer using trustedKeys[0] (the current active key). If a client presents a biscuit signed by a rotated but still valid key, constructing the authorizer with trustedKeys[0] will fail signature verification, breaking mutual authentication and lease renewals during the grace period.
To fix this, update VerifyBiscuit to return the specific ed25519.PublicKey that successfully verified the biscuit, and use that key to instantiate the Authorizer in the callers.
| func VerifyBiscuit(biscuitData []byte, expectedPeer peer.ID, trustedPublicKeys []ed25519.PublicKey, timeout time.Duration) (*biscuit.Biscuit, error) { | |
| func VerifyBiscuit(biscuitData []byte, expectedPeer peer.ID, trustedPublicKeys []ed25519.PublicKey, timeout time.Duration) (*biscuit.Biscuit, ed25519.PublicKey, error) { |
| var lastErr error | ||
| for _, pubKey := range keys { | ||
| var authorized bool | ||
| for _, pubKey := range trustedPublicKeys { |
There was a problem hiding this comment.
Initialize a verifiedKey variable to track which public key successfully authorized the biscuit.
| var lastErr error | |
| for _, pubKey := range keys { | |
| var authorized bool | |
| for _, pubKey := range trustedPublicKeys { | |
| var lastErr error | |
| var authorized bool | |
| var verifiedKey ed25519.PublicKey | |
| for _, pubKey := range trustedPublicKeys { |
| } | ||
|
|
||
| _, err = hub.verifyBiscuit(biscuitData, dummyPeer) | ||
| _, err = VerifyBiscuit(biscuitData, dummyPeer, []ed25519.PublicKey{pub}, 500*time.Millisecond) |
There was a problem hiding this comment.
Update the test call to handle the new return signature.
| _, err = VerifyBiscuit(biscuitData, dummyPeer, []ed25519.PublicKey{pub}, 500*time.Millisecond) | |
| _, _, err = VerifyBiscuit(biscuitData, dummyPeer, []ed25519.PublicKey{pub}, 500*time.Millisecond) |
| defer wg.Done() | ||
| for j := 0; j < 100; j++ { | ||
| _, err := hub.verifyBiscuit(biscuitData, dummyPeer) | ||
| _, err := VerifyBiscuit(biscuitData, dummyPeer, []ed25519.PublicKey{pub}, 500*time.Millisecond) |
There was a problem hiding this comment.
Update the test call to handle the new return signature.
| _, err := VerifyBiscuit(biscuitData, dummyPeer, []ed25519.PublicKey{pub}, 500*time.Millisecond) | |
| _, _, err := VerifyBiscuit(biscuitData, dummyPeer, []ed25519.PublicKey{pub}, 500*time.Millisecond) |
| pID, err := peer.Decode(req.PeerId) | ||
| if err != nil { | ||
| http.Error(w, "Invalid Peer ID", http.StatusBadRequest) | ||
| return | ||
| } |
There was a problem hiding this comment.
The HandleRegister endpoint does not check if the requesting node is banned before issuing a Biscuit token. Even if an administrator bans a node (setting banned = TRUE in the database), the banned node can still successfully call /register to enroll, obtain a brand new Biscuit token signed by the active key, and use it to authenticate with other nodes in the mesh.
Please add a check to verify if the node is banned using s.store.IsNodeBanned right after decoding the Peer ID, and reject the request with http.StatusForbidden if the node is banned.
pID, err := peer.Decode(req.PeerId)
if err != nil {
http.Error(w, "Invalid Peer ID", http.StatusBadRequest)
return
}
banned, err := s.store.IsNodeBanned(ctx, req.PeerId)
if err != nil {
logger.Errorf("Failed to check if node is banned: %v", err)
http.Error(w, "Internal server error", http.StatusInternalServerError)
return
}
if banned {
logger.Warnw("Banned node attempted enrollment", "peer_id", req.PeerId)
http.Error(w, "Forbidden: node is banned", http.StatusForbidden)
return
}| tr := http.DefaultTransport.(*http.Transport).Clone() | ||
| tr.TLSClientConfig = &tls.Config{InsecureSkipVerify: s.config.InsecureSkipTLSVerify} | ||
| client := &http.Client{ | ||
| Timeout: 30 * time.Second, | ||
| Transport: tr, | ||
| } |
There was a problem hiding this comment.
The type assertion http.DefaultTransport.(*http.Transport) can panic if http.DefaultTransport has been wrapped or replaced by a custom http.RoundTripper that is not of type *http.Transport (e.g., by another dependency or initialization function).
To prevent potential startup panics, perform a safe type assertion check and fall back to a newly constructed &http.Transport{} if the assertion fails.
| tr := http.DefaultTransport.(*http.Transport).Clone() | |
| tr.TLSClientConfig = &tls.Config{InsecureSkipVerify: s.config.InsecureSkipTLSVerify} | |
| client := &http.Client{ | |
| Timeout: 30 * time.Second, | |
| Transport: tr, | |
| } | |
| tr, ok := http.DefaultTransport.(*http.Transport) | |
| var transport *http.Transport | |
| if ok { | |
| transport = tr.Clone() | |
| } else { | |
| transport = &http.Transport{} | |
| } | |
| transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: s.config.InsecureSkipTLSVerify} | |
| client := &http.Client{ | |
| Timeout: 30 * time.Second, | |
| Transport: transport, | |
| } |
| ctx := context.Background() | ||
| _, _, err := s.store.GetCurrentKey(ctx) |
There was a problem hiding this comment.
During server startup, database queries (such as checking the keyring and bootstrapping policies) are executed using context.Background() without any timeout. If the database is locked, unresponsive, or slow to connect, the control plane server will hang indefinitely during startup.
Please use a timeout context (e.g., 15 seconds) for these initialization queries to ensure the server fails fast and restarts gracefully if the database is unhealthy.
// Initialize Keyring
ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
defer cancel()
_, _, err := s.store.GetCurrentKey(ctx)- Implemented BootstrapEnrollRequest logic in sam-router with public key payload and status polling. - Added support for --bootstrap-token-path and --bootstrap-token CLI flags in sam-router. - Updated control plane to auto-seed command line --bootstrap-token into storage database on start. - Fixed copylocks value copies and labeled loop break issues in node/router enrollment status polling. - Adapted E2E tests and Kubernetes fixtures to use static bootstrap tokens instead of OIDC token curls.
1 participant
This refactors the monolithic sam-hub into two separate, decoupled services:
Changes: