Skip to content

fix oauth callback and integrate istio gateway with cert-manager for prod #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sanjay7178 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix oauth callback and integrate istio gateway with cert-manager for prod #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5484f0e
feat: Enhance OAuth user handling with fallback email and certificate…
sanjay7178 Sep 6, 2025
028b7a8
feat: Add DCO check to commit message validation in Lefthook
sanjay7178 Sep 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,8 @@ COPY --from=builder /app/guac /app/guac
# Copy templates (if your application uses them from filesystem at runtime)
COPY --from=builder /app/templates /app/templates

# Create and copy certificates as before
# Create certificates directory (certificates will be generated by init container)
RUN mkdir -p /app/certs
# COPY --from=builder /app/certs/ /app/certs/ # This line might not be needed if certs are always generated
RUN echo "Generating self-signed certificates..." && \
openssl req -x509 -nodes -days 365 -newkey rsa:2048 \
-keyout /app/certs/private.key \
-out /app/certs/certificate.crt \
-subj "/C=US/ST=California/L=San Francisco/O=My Company/CN=mydomain.com"

ENV CERT_PATH=/app/certs/certificate.crt
ENV CERT_KEY_PATH=/app/certs/private.key
Expand Down
8 changes: 8 additions & 0 deletions Tiltfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,18 @@ docker_build_with_restart(
live_update=[
sync('./.tilt/guac', '/app/guac'),
sync('./templates', '/app/templates'),
sync('./certs', '/app/certs'),
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid syncing /app/certs; it can overwrite init-generated certs at runtime.

The initContainer is responsible for generating certs. Live-update syncing this path risks clobbering keys/certs and breaking TLS.

Apply this diff:

-        sync('./certs', '/app/certs'),
+        # Avoid syncing certs; init container owns /app/certs
+        # sync('./certs', '/app/certs'),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
sync('./certs', '/app/certs'),
# Avoid syncing certs; init container owns /app/certs
# sync('./certs', '/app/certs'),
🤖 Prompt for AI Agents 
In Tiltfile around line 28, remove the live-update sync of './certs' to
'/app/certs' because it can overwrite initContainer-generated certificates at
runtime; instead either delete this sync entry entirely or change it to sync
local certs to a different non-production path (e.g., '/app/dev-certs') and
update any mounts or code that reference the alternate path so production
init-generated certs are never clobbered.

run('chmod +x /app/guac') # Ensure binary is executable
]
)

k8s_resource(
'browser-sandbox-frontend',
port_forwards=['8080:80'],
labels=["frontend"],
auto_init=False
)

# Frontend (optional dev build) - uses Caddyfile.dev via build arg
# Build only when needed; does not deploy any k8s resources here
docker_build(
Expand Down
16 changes: 9 additions & 7 deletions deployments/isito.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,24 @@ spec:
- browser-sandbox-gateway
http:
- match:
- uri:
prefix: "/api"
- headers:
":authority":
exact: "api.kubebrowse1.ghcat.tech"
route:
Comment on lines +40 to 43
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Host/authority mismatch will prevent routing.

You match on authority api.kubebrowse1.ghcat.tech, but:

  • Gateway hosts only include kubebrowse1.ghcat.tech.
  • VirtualService hosts only include kubebrowse1.ghcat.tech.

Requests to api. will not match the Gateway/VS. Add api.kubebrowse1.ghcat.tech (and app. below) to both Gateway servers.hosts and VirtualService.spec.hosts, and ensure the cert covers all SANs.

Apply this diff (prefer the dedicated field over headers):

-        - headers:
-            ":authority":
-              exact: "api.kubebrowse1.ghcat.tech"
+        - authority:
+            exact: "api.kubebrowse1.ghcat.tech"

Additionally (outside the changed hunk), update hosts:

# Gateway.servers[*].hosts (both 80 and 443)
hosts:
  - kubebrowse1.ghcat.tech
  - api.kubebrowse1.ghcat.tech
  - app.kubebrowse1.ghcat.tech

# VirtualService.spec.hosts
hosts:
  - kubebrowse1.ghcat.tech
  - api.kubebrowse1.ghcat.tech
  - app.kubebrowse1.ghcat.tech
🤖 Prompt for AI Agents 
In deployments/isito.yml around lines 40 to 43, the route match uses a header
":authority" exact "api.kubebrowse1.ghcat.tech" but the Gateway.servers.hosts
and VirtualService.spec.hosts do not include the api.* and app.* hostnames,
causing request mismatches; replace the header-based authority match with the
dedicated host field (use route.hosts / VirtualService.spec.hosts) and add
api.kubebrowse1.ghcat.tech and app.kubebrowse1.ghcat.tech to both
Gateway.servers[*].hosts (for both 80 and 443) and to VirtualService.spec.hosts,
and ensure the TLS certificate includes all SANs (kubebrowse1.ghcat.tech,
api.kubebrowse1.ghcat.tech, app.kubebrowse1.ghcat.tech).

- destination:
host: browser-sandbox-api
port:
number: 4567
- match:
- uri:
prefix: "/websocket"
- headers:
":authority":
exact: "app.kubebrowse1.ghcat.tech"
route:
- destination:
host: browser-sandbox-api
host: browser-sandbox-frontend
port:
number: 4567
- route: # Default route for frontend
number: 80
- route: # Default route (fallback)
- destination:
host: browser-sandbox-frontend
port:
Expand Down
38 changes: 31 additions & 7 deletions deployments/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,24 @@ spec:
app: browser-sandbox-api
spec:
serviceAccountName: browser-sandbox-sa
initContainers:
- name: cert-generator
image: alpine:3.20
command: ['sh', '-c']
args:
- |
apk add --no-cache openssl
mkdir -p /app/certs
echo "Generating self-signed certificates..."
openssl req -x509 -nodes -days 365 -newkey rsa:2048 \
-keyout /app/certs/private.key \
-out /app/certs/certificate.crt \
-subj "/C=US/ST=California/L=San Francisco/O=My Company/CN=mydomain.com"
echo "Certificates generated successfully"
ls -la /app/certs/
volumeMounts:
- name: certs-volume
mountPath: /app/certs
containers:
Comment on lines +256 to 274
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Don’t generate self-signed TLS in initContainer for prod; use cert-manager/Issuers or terminate at Istio.

This contradicts the PR goal and creates ephemeral, untrusted certs on each pod restart.

If TLS terminates at the Istio Gateway, remove pod TLS entirely. Otherwise, mount a cert-manager-managed Secret.

Apply (remove initContainer and cert volume):

       serviceAccountName: browser-sandbox-sa
-      initContainers:
-        - name: cert-generator
-          image: alpine:3.20
-          command: ['sh', '-c']
-          args:
-            - |
-              apk add --no-cache openssl
-              mkdir -p /app/certs
-              echo "Generating self-signed certificates..."
-              openssl req -x509 -nodes -days 365 -newkey rsa:2048 \
-                -keyout /app/certs/private.key \
-                -out /app/certs/certificate.crt \
-                -subj "/C=US/ST=California/L=San Francisco/O=My Company/CN=mydomain.com"
-              echo "Certificates generated successfully"
-              ls -la /app/certs/
-          volumeMounts:
-            - name: certs-volume
-              mountPath: /app/certs

And also remove the related env, volumeMount, and volume below (see next comments).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
initContainers:
- name: cert-generator
image: alpine:3.20
command: ['sh', '-c']
args:
- |
apk add --no-cache openssl
mkdir -p /app/certs
echo "Generating self-signed certificates..."
openssl req -x509 -nodes -days 365 -newkey rsa:2048 \
-keyout /app/certs/private.key \
-out /app/certs/certificate.crt \
-subj "/C=US/ST=California/L=San Francisco/O=My Company/CN=mydomain.com"
echo "Certificates generated successfully"
ls -la /app/certs/
volumeMounts:
- name: certs-volume
mountPath: /app/certs
containers:
serviceAccountName: browser-sandbox-sa
containers:
🤖 Prompt for AI Agents 
In deployments/manifest.yml around lines 256-274, the initContainer generates
ephemeral self-signed TLS certs which is inappropriate for production and
contradicts the PR goal; remove the entire initContainer block and the
certs-volume definition/mounts, then remove any related env vars, container
volumeMount entries, and volumes that reference certs-volume elsewhere in the
file so the pod no longer manages TLS; if TLS is terminated by Istio ensure no
pod TLS config remains, otherwise replace these with a reference to a
cert-manager-managed Secret (mounted via a Secret volume) or document that TLS
is terminated at the Gateway.

- name: api
image: ghcr.io/browsersec/kubebrowse:sha-09dfa1f
Expand Down Expand Up @@ -320,15 +338,15 @@ spec:
- name: DISTRIBUTED_TRACING
value: "true"
- name: GITHUB_CLIENT_ID
value: "Ov23li974J8UoG1CH0HJ"
value: "Ov23liQQQ5lMyIrOULYx"
- name: GITHUB_CLIENT_SECRET
value: "f16b7dc2b85d4a7f817e273b4a2d64124153ce8f"
value: "47afea55e0e19231e1cc82c3a57d56aaeedb480e"
- name: GITHUB_CALLBACK_URL
value: "https://localhost:4567/auth/oauth/github/callback"
- name: SESSION_SECRET
value: "McX0xoYlJYpO0EGdUZC6aJxVh2rKYrXZM7SPMpVREeXMXoMBI20v90PICD-LfEn7jr07c5vH2CWcKzjl8hoNvQ"
- name: FRONTEND_URL
value: "http://localhost:5173"
value: "http://localhost:8080"
- name: ENVIRONMENT
value: "development"
Comment on lines 345 to 351
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix OAuth callback and environment for prod.

GITHUB_CALLBACK_URL=https://localhost:4567/... and ENVIRONMENT=development will not work in prod. Callback must be your public domain and environment should be production.

Apply:

-            - name: GITHUB_CALLBACK_URL
-              value: "https://localhost:4567/auth/oauth/github/callback"
+            - name: GITHUB_CALLBACK_URL
+              value: "https://<your-domain>/auth/oauth/github/callback"
...
-            - name: ENVIRONMENT
-              value: "development"
+            - name: ENVIRONMENT
+              value: "production"

Also ensure the GitHub OAuth app is updated with the same callback URL.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
value: "https://localhost:4567/auth/oauth/github/callback"
- name: SESSION_SECRET
value: "McX0xoYlJYpO0EGdUZC6aJxVh2rKYrXZM7SPMpVREeXMXoMBI20v90PICD-LfEn7jr07c5vH2CWcKzjl8hoNvQ"
- name: FRONTEND_URL
value: "http://localhost:5173"
value: "http://localhost:8080"
- name: ENVIRONMENT
value: "development"
env:
- name: GITHUB_CALLBACK_URL
value: "https://<your-domain>/auth/oauth/github/callback"
- name: SESSION_SECRET
value: "McX0xoYlJYpO0EGdUZC6aJxVh2rKYrXZM7SPMpVREeXMXoMBI20v90PICD-LfEn7jr07c5vH2CWcKzjl8hoNvQ"
- name: FRONTEND_URL
value: "http://localhost:8080"
- name: ENVIRONMENT
value: "production"
🤖 Prompt for AI Agents 
In deployments/manifest.yml around lines 345 to 351 the OAuth callback and
environment are set for local development; change the GITHUB_CALLBACK_URL value
to your public production callback URL
(https://your-production-domain/auth/oauth/github/callback) and set ENVIRONMENT
to "production", removing any hard-coded local URLs and development secrets;
after updating the manifest, ensure the GitHub OAuth app's callback/redirect URL
is updated to exactly match the production callback URL and rotate any secrets
if they were development-only.

- name: SMTP_HOST
Expand All @@ -341,11 +359,17 @@ spec:
value: "ulagdeliigobhdgr"
- name: FROM_EMAIL
value: "sanjay.obsidian@gmail.com"
# - name: CERT_PATH
# value: ""
# - name: CERT_KEY_PATH
# value: ""
- name: CERT_PATH
value: "/app/certs/certificate.crt"
- name: CERT_KEY_PATH
value: "/app/certs/private.key"
volumeMounts:
- name: certs-volume
mountPath: /app/certs
securityContext: {}
volumes:
- name: certs-volume
emptyDir: {}
---
Comment on lines +362 to 373
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove CERT_PATH/CERT_KEY and the cert volume if TLS terminates at the gateway. Or replace with cert-manager Secret.

Mounting ephemeral certs is brittle.

Option A (Gateway TLS termination, recommended):

-            - name: CERT_PATH
-              value: "/app/certs/certificate.crt"
-            - name: CERT_KEY_PATH
-              value: "/app/certs/private.key"
-          volumeMounts:
-            - name: certs-volume
-              mountPath: /app/certs
...
-      volumes:
-        - name: certs-volume
-          emptyDir: {}

Option B (pod TLS via cert-manager):

  • Create Certificate with secretName: api-tls.
  • Mount Secret:
+          volumeMounts:
+            - name: api-tls
+              mountPath: /app/certs
+              readOnly: true
...
+      volumes:
+        - name: api-tls
+          secret:
+            secretName: api-tls

And keep CERT_PATH=/app/certs/tls.crt, CERT_KEY_PATH=/app/certs/tls.key.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: CERT_PATH
value: "/app/certs/certificate.crt"
- name: CERT_KEY_PATH
value: "/app/certs/private.key"
volumeMounts:
- name: certs-volume
mountPath: /app/certs
securityContext: {}
volumes:
- name: certs-volume
emptyDir: {}
---
# (removed CERT_PATH/CERT_KEY and certs-volume since TLS now terminates at the gateway)
securityContext: {}
---
🧰 Tools
🪛 Checkov (3.2.334)

[MEDIUM] 240-373: Containers should not run with allowPrivilegeEscalation

(CKV_K8S_20)

[MEDIUM] 240-373: Minimize the admission of root containers

(CKV_K8S_23)

# API Service
apiVersion: v1
Expand Down
56 changes: 50 additions & 6 deletions docs/CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ order: 800

# Contributing to KubeBrowse

Thank you for your interest in contributing to GUAC! This document provides guidelines and instructions for contributing to this project.
Thank you for your interest in contributing to KubeBrowse! This document provides guidelines and instructions for contributing to this project.

## Table of Contents

Expand All @@ -16,10 +16,14 @@ Thank you for your interest in contributing to GUAC! This document provides guid
- [Getting Started](#getting-started)
- [Development Environment Setup](#development-environment-setup)
- [Troubleshooting Common Issues](#troubleshooting-common-issues)
- [Go Version Mismatch](#go-version-mismatch)
- [Project Structure](#project-structure)
- [Development Workflow](#development-workflow)
- [Branching Strategy](#branching-strategy)
- [Commit Guidelines](#commit-guidelines)
- [Conventional Commits Format](#conventional-commits-format)
- [DCO Sign-off Requirement](#dco-sign-off-requirement)
- [Example Commit](#example-commit)
- [Pre-commit Hooks](#pre-commit-hooks)
- [Pull Requests](#pull-requests)
- [PR Process](#pr-process)
Expand Down Expand Up @@ -146,9 +150,11 @@ If you encounter errors like `compile: version "go1.24.0" does not match go tool

### Commit Guidelines

We follow [Conventional Commits](https://www.conventionalcommits.org/) for commit messages:
We follow [Conventional Commits](https://www.conventionalcommits.org/) for commit messages and require [Developer Certificate of Origin (DCO)](https://developercertificate.org/) sign-off for all contributions.

```
#### Conventional Commits Format

```text
<type>(<scope>): <description>

<body>
Expand All @@ -157,6 +163,7 @@ We follow [Conventional Commits](https://www.conventionalcommits.org/) for commi
```

Types include:

- `feat`: A new feature
- `fix`: A bug fix
- `docs`: Documentation changes
Expand All @@ -165,14 +172,50 @@ Types include:
- `test`: Adding or modifying tests
- `chore`: Changes to the build process or auxiliary tools

Example:
#### DCO Sign-off Requirement

All commits must include a `Signed-off-by` line to certify that you have the right to submit the code under the project's license. This is required by the [Developer Certificate of Origin](https://developercertificate.org/).

**To sign off your commits automatically:**

```bash
# Configure git to always sign off commits (recommended)
git config --global format.signoff true

# Or use the -s flag for individual commits
git commit -s -m "feat(api): add new authentication endpoint"
```

**To sign off existing commits:**

```bash
# Sign off the last commit
git commit --amend --signoff

# Sign off multiple commits using interactive rebase
git rebase --signoff HEAD~<number-of-commits>

# Force push to update the PR
git push --force-with-lease
```

The sign-off line should look like:

```text
Signed-off-by: Your Name <your.email@example.com>
```

#### Example Commit

```text
feat(api): add endpoint for user authentication

- Implement JWT token generation
- Add middleware for token validation

Fixes #123

Signed-off-by: John Doe <john.doe@example.com>
```

### Pre-commit Hooks
Expand All @@ -186,11 +229,13 @@ This project uses [Lefthook](https://github.com/evilmartians/lefthook) for manag
- Potential secrets

Install the hooks with:

```bash
make hooks
```

You can manually run the pre-commit checks:

```bash
# Check only staged files
make lint
Expand Down Expand Up @@ -251,5 +296,4 @@ If Lefthook is not found, you'll be prompted to install it. Lefthook hooks are a
5. Tag the release with the version number
6. Update documentation with release notes

Thank you for contributing to GUAC!
*
Thank you for contributing to KubeBrowse!
48 changes: 42 additions & 6 deletions frontend/Caddyfile.dev
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,50 @@
root * /srv
file_server

# Handle OAuth callback redirects
handle /auth/success {
file_server
header Cache-Control "no-cache, no-store, must-revalidate"
# Authentication endpoints - proxy to backend API (excluding frontend routes)
handle /auth/oauth/* {
reverse_proxy {$CADDY_GUAC_CLIENT_URL} {
transport http {
tls
tls_insecure_skip_verify
}
header_up Host {http.reverse_proxy.upstream.hostport}
header_up Connection {header.Connection}
header_up Upgrade {header.Upgrade}

# Cookie handling for session management
}
}

# Authentication endpoints - proxy to backend API
handle /auth/* {
handle /auth/login {
reverse_proxy {$CADDY_GUAC_CLIENT_URL} {
transport http {
tls
tls_insecure_skip_verify
}
header_up Host {http.reverse_proxy.upstream.hostport}
header_up Connection {header.Connection}
header_up Upgrade {header.Upgrade}

# Cookie handling for session management
}
}

handle /auth/me {
reverse_proxy {$CADDY_GUAC_CLIENT_URL} {
transport http {
tls
tls_insecure_skip_verify
}
header_up Host {http.reverse_proxy.upstream.hostport}
header_up Connection {header.Connection}
header_up Upgrade {header.Upgrade}

# Cookie handling for session management
}
}

handle /auth/logout {
reverse_proxy {$CADDY_GUAC_CLIENT_URL} {
transport http {
tls
Expand Down
17 changes: 15 additions & 2 deletions frontend/src/components/auth/LoginForm.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { Label } from '../ui/label';
import { Card } from '../ui/card';
import { Alert } from '../ui/alert';
import EmailVerification from './EmailVerification';
import toast from 'react-hot-toast';

export function LoginForm({ onSwitchToRegister }) {
const [formData, setFormData] = useState({
Expand All @@ -32,13 +33,25 @@ export function LoginForm({ onSwitchToRegister }) {

const result = await login(formData.email, formData.password);

if (!result.success) {
if (result.success) {
// Show success toast similar to OAuth flow
toast.success(`Welcome back, ${result.user?.name || result.user?.email || 'User'}!`, {
duration: 3000,
position: "top-right",
style: {
background: "#10B981",
color: "#fff",
fontWeight: "600",
},
iconTheme: { primary: "#fff", secondary: "#10B981" },
});
setIsSubmitting(false);
} else {
if (result.needsVerification) {
setShowEmailVerification(true);
}
setIsSubmitting(false);
}
// If successful, the auth context will handle the state update
};

const handleGitHubLogin = () => {
Expand Down
Loading