-
Notifications
You must be signed in to change notification settings - Fork 6
fix(security/gcp): drop plaintext SCHEDULED_TASK_SECRET — rely on OIDC + Cloud Run IAM (closes #159) #161
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.
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
fix(security/gcp): drop plaintext SCHEDULED_TASK_SECRET — rely on OIDC + Cloud Run IAM (closes #159) #161
Changes from all commits
997f9a8
9caee8b
59efced
8ef51a5
db4925c
bb0f441
985cb8b
6f22b9d
97253fd
5193227
e993d67
b473ebe
171eea5
7dbf859
7935b6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ package server | |
|
|
||
| import ( | ||
| "context" | ||
| "crypto/subtle" | ||
| "encoding/base64" | ||
| "encoding/json" | ||
| "fmt" | ||
|
|
@@ -20,9 +19,13 @@ import ( | |
| func CreateHTTPServer(app *Application, port int) *http.Server { | ||
| mux := http.NewServeMux() | ||
|
|
||
| // Register health and scheduled task routes (always available) | ||
| // Register health and scheduled task routes (always available). | ||
| // /api/scheduled/* sits behind the scheduledauth middleware — see | ||
| // internal/server/scheduledauth. Mode is selected by | ||
| // SCHEDULED_TASK_AUTH_MODE: oidc on GCP, bearer on Azure, disabled | ||
| // for local dev. | ||
| mux.HandleFunc("/health", app.handleHealthCheck) | ||
| mux.HandleFunc("/api/scheduled/", app.handleScheduledHTTP) | ||
| mux.Handle("/api/scheduled/", app.scheduledAuthMiddleware(http.HandlerFunc(app.handleScheduledHTTP))) | ||
|
|
||
| // When STATIC_DIR is set, serve static files for non-API paths | ||
| // and route only /api/ to the API handler. | ||
|
|
@@ -105,24 +108,29 @@ func (app *Application) handleHTTPRequest(w http.ResponseWriter, r *http.Request | |
| lambdaResponseToHTTP(w, lambdaResp) | ||
| } | ||
|
|
||
| // scheduledAuthMiddleware returns the configured scheduledauth middleware, | ||
| // or a passthrough no-op if app.scheduledAuth is nil. Tests that build | ||
| // an Application directly (without NewApplicationFromDeps) leave it | ||
| // nil — they exercise the handler in isolation, with the middleware | ||
| // covered separately in scheduledauth's own tests. | ||
| func (app *Application) scheduledAuthMiddleware(next http.Handler) http.Handler { | ||
| if app.scheduledAuth == nil { | ||
| return next | ||
| } | ||
| return app.scheduledAuth.Middleware(next) | ||
|
Comment on lines
+116
to
+120
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fail closed when scheduled auth has not been initialized. Returning 🔒 Suggested fail-closed guardfunc (app *Application) scheduledAuthMiddleware(next http.Handler) http.Handler {
if app.scheduledAuth == nil {
- return next
+ return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ http.Error(w, "Scheduled task auth is not configured", http.StatusServiceUnavailable)
+ })
}
return app.scheduledAuth.Middleware(next)
}🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| // handleScheduledHTTP handles scheduled tasks via HTTP endpoint | ||
| // This is used by GCP Cloud Scheduler and Azure Logic Apps | ||
| // This is used by GCP Cloud Scheduler and Azure Logic Apps. Auth is | ||
| // enforced upstream by scheduledAuthMiddleware (see CreateHTTPServer); | ||
| // by the time we reach here the request is already authenticated for | ||
| // the configured mode. | ||
| func (app *Application) handleScheduledHTTP(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodPost { | ||
| http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) | ||
| return | ||
| } | ||
|
|
||
| // Verify shared secret for scheduled task authentication | ||
| if secret := app.appConfig.ScheduledTaskSecret; secret != "" { | ||
| provided := r.Header.Get("Authorization") | ||
| expected := "Bearer " + secret | ||
| if subtle.ConstantTimeCompare([]byte(provided), []byte(expected)) != 1 { | ||
| http.Error(w, "Unauthorized", http.StatusUnauthorized) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| ctx := r.Context() | ||
|
|
||
| // Ensure database connection is established (lazy initialization) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| package scheduledauth | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strings" | ||
| ) | ||
|
|
||
| // EnvSource is the minimal interface NewFromEnv depends on. The default | ||
| // implementation (envOS) reads from os.Getenv; tests inject a map. | ||
| type EnvSource interface { | ||
| Get(string) string | ||
| } | ||
|
|
||
| // EnvMap is an EnvSource backed by a map. Useful in tests. | ||
| type EnvMap map[string]string | ||
|
|
||
| // Get returns the value for key, or "" if absent. | ||
| func (m EnvMap) Get(key string) string { return m[key] } | ||
|
|
||
| // Environment variable names. Centralised so tests and Terraform stay | ||
| // aligned with the Go code. | ||
| const ( | ||
| EnvAuthMode = "SCHEDULED_TASK_AUTH_MODE" // "oidc" | "bearer" | "disabled" | ||
| EnvOIDCIssuer = "SCHEDULED_TASK_OIDC_ISSUER" // default GoogleIssuer | ||
| EnvOIDCJWKSURL = "SCHEDULED_TASK_OIDC_JWKS_URL" // default GoogleJWKSURL | ||
| EnvOIDCAudience = "SCHEDULED_TASK_OIDC_AUDIENCE" // comma-separated | ||
| EnvOIDCSubjects = "SCHEDULED_TASK_OIDC_SUBJECTS" // comma-separated; REQUIRED in oidc mode | ||
| EnvBearerSecret = "SCHEDULED_TASK_SECRET" // bearer mode only | ||
| ) | ||
|
|
||
| // LoadConfig parses Config from env. SCHEDULED_TASK_AUTH_MODE MUST be | ||
| // set explicitly to one of "oidc", "bearer", or "disabled" — an unset | ||
| // value returns ErrConfigInvalid so a missed Terraform/Kubernetes env | ||
| // wiring fails server startup instead of silently booting | ||
| // /api/scheduled/* unauthenticated. "disabled" is reserved for an | ||
| // explicit local-dev choice and must be opted into deliberately. | ||
| func LoadConfig(env EnvSource) (Config, error) { | ||
| mode := strings.ToLower(strings.TrimSpace(env.Get(EnvAuthMode))) | ||
| if mode == "" { | ||
| return Config{}, fmt.Errorf("%w: %s is unset (set to oidc, bearer, or disabled)", | ||
| ErrConfigInvalid, EnvAuthMode) | ||
| } | ||
|
|
||
| cfg := Config{Mode: Mode(mode)} | ||
|
|
||
| switch cfg.Mode { | ||
| case ModeOIDC: | ||
| cfg.Issuer = strings.TrimSpace(env.Get(EnvOIDCIssuer)) | ||
| cfg.JWKSURL = strings.TrimSpace(env.Get(EnvOIDCJWKSURL)) | ||
| cfg.Audiences = splitCSV(env.Get(EnvOIDCAudience)) | ||
| cfg.Subjects = splitCSV(env.Get(EnvOIDCSubjects)) | ||
|
|
||
| case ModeBearer: | ||
| cfg.Bearer = env.Get(EnvBearerSecret) | ||
| // Permit other env vars to be set without complaint — the | ||
| // terraform module ships SCHEDULED_TASK_OIDC_* across all | ||
| // platforms and they should be ignored on Azure. | ||
|
|
||
| case ModeDisabled: | ||
| // Nothing to parse. | ||
|
|
||
| default: | ||
| return Config{}, fmt.Errorf("%w: %s=%q (expected oidc, bearer, or disabled)", | ||
| ErrConfigInvalid, EnvAuthMode, mode) | ||
| } | ||
|
|
||
| return cfg, nil | ||
| } | ||
|
|
||
| // splitCSV splits a comma-separated value into a trimmed list, dropping | ||
| // empty entries (so trailing commas / whitespace are tolerated). | ||
| func splitCSV(raw string) []string { | ||
| if raw == "" { | ||
| return nil | ||
| } | ||
| parts := strings.Split(raw, ",") | ||
| out := make([]string, 0, len(parts)) | ||
| for _, p := range parts { | ||
| p = strings.TrimSpace(p) | ||
| if p != "" { | ||
| out = append(out, p) | ||
| } | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't fail open when
SCHEDULED_TASK_AUTH_MODEis missing.buildScheduledAuthgoes throughscheduledauth.LoadConfig, which currently maps an unset mode todisabled. In the production constructor, that turns a missed Terraform/env wiring into a live validator that allows every/api/scheduled/*request instead of failing startup, which is the exact regression this PR is trying to prevent.🔒 Proposed guard
func buildScheduledAuth(cfg ApplicationConfig) (*scheduledauth.Validator, error) { + if strings.TrimSpace(os.Getenv(scheduledauth.EnvAuthMode)) == "" { + return nil, fmt.Errorf( + "%w: %s must be explicitly set (use disabled only for local dev)", + scheduledauth.ErrConfigInvalid, + scheduledauth.EnvAuthMode, + ) + } saCfg, err := scheduledauth.LoadConfig(envSourceOS{}) if err != nil { return nil, err }📝 Committable suggestion
🤖 Prompt for AI Agents