feat: implement OIDC PKCE redirect flow and deploy local Dex sandbox - #212
Conversation
![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 introduces the SAM Web Console, adding a frontend interface and a console server to manage the zero-trust mesh network. It integrates the console into the development environment with Kubernetes manifests, Dex OIDC configurations, and E2E tests, while extending the control plane and SQL storage to support user-scoped endpoints, OIDC authentication, and multi-tenancy. The review feedback highlights critical security and reliability improvements, including escaping user-supplied data in the frontend to prevent XSS, dynamically setting the Secure flag on cookies over HTTPS, using constant-time comparisons for admin token verification to mitigate timing attacks, and adding a timeout to HTTP client requests.
| function getAuthHeaders() { | ||
| return { | ||
| 'Authorization': 'Bearer ' + getAdminToken() | ||
| }; | ||
| } |
There was a problem hiding this comment.
The getAuthHeaders function currently returns Authorization: Bearer even when getAdminToken() is empty. This causes the reverse proxy in the console server to skip injecting the sam_session cookie because the Authorization header is technically present (as "Bearer "). This completely breaks the OIDC redirect flow, resulting in 401 Unauthorized errors.
Only include the Authorization header if a valid admin token is actually present.
function getAuthHeaders() {
const token = getAdminToken();
return token ? { 'Authorization': 'Bearer ' + token } : {};
}| tbody.innerHTML = users.map(user => ` | ||
| <tr> | ||
| <td><code>${user.ID || '-'}</code></td> | ||
| <td>${user.Role || '-'}</td> | ||
| <td>${user.Name || '-'}</td> | ||
| <td>${user.Email || '-'}</td> | ||
| </tr> | ||
| `).join(''); |
There was a problem hiding this comment.
Rendering user.Name and user.Email directly into innerHTML without escaping poses a stored Cross-Site Scripting (XSS) vulnerability. If an attacker registers with a malicious payload in their OIDC profile, it will execute in the administrator's browser when viewing the users table.
Please implement a helper function to escape HTML characters and use it when rendering user-supplied strings.
tbody.innerHTML = users.map(user => {
const escapeHTML = str => (str || '-').replace(/[&<>'" ]/g,
tag => ({ '&': '&', '<': '<', '>': '>', "'": ''', '"': '"' }[tag] || tag)
);
return `
<tr>
<td><code>${escapeHTML(user.ID)}</code></td>
<td>${escapeHTML(user.Role)}</td>
<td>${escapeHTML(user.Name)}</td>
<td>${escapeHTML(user.Email)}</td>
</tr>
`;
}).join('');| resp, err := http.Get(cfg.HubURL + "/info") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to query control-plane info for OIDC discovery: %w", err) | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() |
There was a problem hiding this comment.
Using http.Get without a timeout can cause the console server to block indefinitely during startup if the control plane is slow or unresponsive. Always use a custom http.Client with a configured timeout.
| resp, err := http.Get(cfg.HubURL + "/info") | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to query control-plane info for OIDC discovery: %w", err) | |
| } | |
| defer func() { _ = resp.Body.Close() }() | |
| client := &http.Client{Timeout: 10 * time.Second} | |
| resp, err := client.Get(cfg.HubURL + "/info") | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to query control-plane info for OIDC discovery: %w", err) | |
| } | |
| defer func() { _ = resp.Body.Close() }() |
| http.SetCookie(w, &http.Cookie{ | ||
| Name: "sam_oidc_state", | ||
| Value: state, | ||
| Path: "/", | ||
| MaxAge: 300, | ||
| HttpOnly: true, | ||
| Secure: false, | ||
| SameSite: http.SameSiteLaxMode, | ||
| }) | ||
|
|
||
| http.SetCookie(w, &http.Cookie{ | ||
| Name: "sam_oidc_verifier", | ||
| Value: verifier, | ||
| Path: "/", | ||
| MaxAge: 300, | ||
| HttpOnly: true, | ||
| Secure: false, | ||
| SameSite: http.SameSiteLaxMode, | ||
| }) |
There was a problem hiding this comment.
The sam_oidc_state and sam_oidc_verifier cookies are configured with Secure: false regardless of whether the connection is secure. If the console is accessed over HTTPS, these cookies should have Secure: true to prevent them from being transmitted over unencrypted connections.
| http.SetCookie(w, &http.Cookie{ | |
| Name: "sam_oidc_state", | |
| Value: state, | |
| Path: "/", | |
| MaxAge: 300, | |
| HttpOnly: true, | |
| Secure: false, | |
| SameSite: http.SameSiteLaxMode, | |
| }) | |
| http.SetCookie(w, &http.Cookie{ | |
| Name: "sam_oidc_verifier", | |
| Value: verifier, | |
| Path: "/", | |
| MaxAge: 300, | |
| HttpOnly: true, | |
| Secure: false, | |
| SameSite: http.SameSiteLaxMode, | |
| }) | |
| http.SetCookie(w, &http.Cookie{ | |
| Name: "sam_oidc_state", | |
| Value: state, | |
| Path: "/", | |
| MaxAge: 300, | |
| HttpOnly: true, | |
| Secure: scheme == "https", | |
| SameSite: http.SameSiteLaxMode, | |
| }) | |
| http.SetCookie(w, &http.Cookie{ | |
| Name: "sam_oidc_verifier", | |
| Value: verifier, | |
| Path: "/", | |
| MaxAge: 300, | |
| HttpOnly: true, | |
| Secure: scheme == "https", | |
| SameSite: http.SameSiteLaxMode, | |
| }) |
| http.SetCookie(w, &http.Cookie{ | ||
| Name: "sam_session", | ||
| Value: rawIDToken, | ||
| Path: "/", | ||
| MaxAge: 24 * 3600, | ||
| HttpOnly: true, | ||
| Secure: false, | ||
| SameSite: http.SameSiteLaxMode, | ||
| }) |
There was a problem hiding this comment.
The sam_session cookie is configured with Secure: false. If the console is accessed over HTTPS, this cookie should have Secure: true to prevent it from being transmitted over unencrypted connections.
| http.SetCookie(w, &http.Cookie{ | |
| Name: "sam_session", | |
| Value: rawIDToken, | |
| Path: "/", | |
| MaxAge: 24 * 3600, | |
| HttpOnly: true, | |
| Secure: false, | |
| SameSite: http.SameSiteLaxMode, | |
| }) | |
| http.SetCookie(w, &http.Cookie{ | |
| Name: "sam_session", | |
| Value: rawIDToken, | |
| Path: "/", | |
| MaxAge: 24 * 3600, | |
| HttpOnly: true, | |
| Secure: scheme == "https", | |
| SameSite: http.SameSiteLaxMode, | |
| }) |
| if s.config.AdminToken != "" && tokenStr == s.config.AdminToken { | ||
| return &storage.User{ | ||
| ID: "root-admin", | ||
| Email: "admin@sam-mesh.local", | ||
| Role: "admin", | ||
| CreatedAt: time.Now(), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Comparing the AdminToken using standard string comparison (tokenStr == s.config.AdminToken) is vulnerable to timing attacks. To prevent timing attacks, use a constant-time comparison helper like subtle.ConstantTimeCompare on the SHA256 hashes of the tokens.
Note: You will need to import "crypto/subtle" in this file.
// 1. Check root admin token backdoor
if s.config.AdminToken != "" {
tokenHash := sha256.Sum256([]byte(tokenStr))
adminHash := sha256.Sum256([]byte(s.config.AdminToken))
if subtle.ConstantTimeCompare(tokenHash[:], adminHash[:]) == 1 {
return &storage.User{
ID: "root-admin",
Email: "admin@sam-mesh.local",
Role: "admin",
CreatedAt: time.Now(),
}, nil
}
}…cks, and E2E assertions
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new web console for the SAM mesh, adding necessary infrastructure configurations, Docker support, and backend API endpoints for user authentication and resource management. The review feedback identifies several critical security issues, including the use of hardcoded secrets, potential XSS vulnerabilities in the frontend, and insecure token storage. Additionally, the reviewer suggests performance optimizations for database queries, improvements to the API design, and general code quality refinements to ensure the console is robust and maintainable.
| - "--allowed-audiences=${ALLOWED_AUDIENCES}" | ||
| - "--policy-file=/etc/sam/policies/policies.yaml" | ||
| - "--insecure-skip-tls-verify" | ||
| - "--admin-token=super-secret-admin-token" |
There was a problem hiding this comment.
A hardcoded admin token super-secret-admin-token is being used. Even for a development environment, this is a poor security practice as it can be easily overlooked and promoted to production environments. It's recommended to manage this secret using Kubernetes secrets and inject it as an environment variable.
| - "--hub=http://sam-control-plane:8080" | ||
| - "--bind-addr=:8081" | ||
| - "--static-dir=/app/public" | ||
| - "--admin-token=super-secret-admin-token" |
There was a problem hiding this comment.
A hardcoded admin token super-secret-admin-token is being used. This is a security risk. It's recommended to manage this secret using Kubernetes secrets and inject it as an environment variable.
| <td>${escapeHTML(node.OwnerID)}</td> | ||
| <td> | ||
| <div class="actions-cell"> | ||
| <button class="btn btn-sm btn-danger" onclick="revokeDevice('${escapeHTML(node.PeerID)}')">Revoke</button> |
There was a problem hiding this comment.
Using escapeHTML within an onclick attribute is not sufficient to prevent Cross-Site Scripting (XSS). escapeHTML is designed for HTML context, but here the value is being placed inside a JavaScript string literal. A malicious node.PeerID (e.g., ');alert(1);//) can break out of the string and execute arbitrary code.
The same vulnerability exists in renderEnrollmentsTable for approveEnrollment and rejectEnrollment.
To fix this, you should avoid building HTML strings with innerHTML. Instead, create DOM elements programmatically and attach event listeners. This separates data from code and is inherently safer.
For example, for the revoke button:
const revokeButton = document.createElement('button');
revokeButton.className = 'btn btn-sm btn-danger';
revokeButton.textContent = 'Revoke';
revokeButton.addEventListener('click', () => {
revokeDevice(node.PeerID);
});
// ... append button to the cell| @@ -0,0 +1,17 @@ | |||
| FROM golang:1.26.4-alpine AS builder | |||
There was a problem hiding this comment.
The Go version 1.26.4 specified in the base image does not exist. Please use a valid and recent version of Go, such as 1.22.4, to ensure the build environment is correct and secure.
FROM golang:1.22.4-alpine AS builder
| window.savePolicy = async function() { | ||
| const yamlContent = document.getElementById('policy-yaml').value; | ||
| const encoder = new TextEncoder(); | ||
| const yamlBytes = encoder.encode(yamlContent); | ||
|
|
||
| const lenBytes = writeVarint(yamlBytes.length); | ||
| const pbBytes = new Uint8Array(1 + lenBytes.length + yamlBytes.length); | ||
| pbBytes[0] = 0x0a; // field tag 1 (YamlContent) | ||
| pbBytes.set(lenBytes, 1); | ||
| pbBytes.set(yamlBytes, 1 + lenBytes.length); |
There was a problem hiding this comment.
Manually encoding the protobuf message for the policy update is brittle and hard to maintain. If the PolicyConfigUpdateRequest protobuf message definition changes in the backend, this client-side code will need to be updated manually, and it might fail silently or in non-obvious ways.
Consider using a JavaScript protobuf library (like protobuf.js) to generate the serialization code from your .proto files. This would ensure that the client and server are always in sync and make the code more robust.
| user = &storage.User{ | ||
| ID: sub, | ||
| Email: email, | ||
| Role: "user", |
There was a problem hiding this comment.
The role for auto-registered OIDC users is hardcoded to "user". This lacks flexibility. In many systems, it's desirable to assign roles (including admin roles) based on claims in the OIDC token, such as group membership. Consider making the default role configurable or implementing a mechanism to map OIDC groups to SAM roles during user provisioning.
| allNodes, err := s.store.ListNodes(ctx) | ||
| if err == nil { | ||
| for _, n := range allNodes { | ||
| if n.OwnerID == user.ID { | ||
| nodes = append(nodes, n) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
For non-admin users, this code fetches all nodes from the database and then filters them by OwnerID in the application layer. This is inefficient and will not scale well as the number of nodes grows.
It would be much more efficient to add a method to the storage layer to fetch nodes by owner directly, for example ListNodesByOwner(ctx, ownerID). This would push the filtering down to the database.
| allTokens, err := s.store.ListBootstrapTokens(ctx) | ||
| if err == nil { | ||
| for _, t := range allTokens { | ||
| if t.OwnerID == user.ID || user.Role == "admin" { | ||
| tokens = append(tokens, t) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the node listing, this code fetches all bootstrap tokens and filters them by OwnerID in memory for non-admin users. This is inefficient and will not scale well. A dedicated storage method like ListBootstrapTokensByOwner(ctx, ownerID) should be created to perform this filtering at the database level.
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | ||
| http.Error(w, "Invalid JSON body", http.StatusBadRequest) | ||
| return | ||
| } | ||
| defer func() { _ = r.Body.Close() }() |
There was a problem hiding this comment.
The defer to close the request body should be placed before the body is read. This ensures that the body is closed even if json.NewDecoder returns an error (e.g., on an empty body).
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | |
| http.Error(w, "Invalid JSON body", http.StatusBadRequest) | |
| return | |
| } | |
| defer func() { _ = r.Body.Close() }() | |
| defer func() { _ = r.Body.Close() }() | |
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | |
| http.Error(w, "Invalid JSON body", http.StatusBadRequest) | |
| return | |
| } |
| func (s *Server) HandleUserRevoke(w http.ResponseWriter, r *http.Request) { | ||
| user, err := s.authenticateUser(r) | ||
| if err != nil { | ||
| http.Error(w, "Unauthorized: "+err.Error(), http.StatusUnauthorized) | ||
| return | ||
| } | ||
|
|
||
| if r.Method != http.MethodPost { | ||
| http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) | ||
| return | ||
| } |
There was a problem hiding this comment.
The HandleUserRevoke endpoint uses POST with an id in the query string (/api/user/revoke?id=...). For a RESTful API, a DELETE request with the ID in the path, like DELETE /api/user/nodes/{id}, would be more idiomatic for a resource deletion/revocation operation.
1 participant
Now can we tested locally with
make kind-upThe console is exposed in
localhost:9092