input
{
"command": "gh pr diff 53 --repo wtvamp/loginid-takehome"
}result
diff --git a/cmd/api-service/main.go b/cmd/api-service/main.go
index db69913..b705696 100644
--- a/cmd/api-service/main.go
+++ b/cmd/api-service/main.go
@@ -9,6 +9,7 @@ import (
"fmt"
"log"
"net/http"
+ "os"
"loginid-takehome/internal/api"
"loginid-takehome/internal/app"
@@ -42,33 +43,33 @@ func sqlDriverNameFor(daoDriver string) string {
}
}
-// validateAuthConfig catches exactly the live-review finding that
-// motivated it: AUTH_JWT_ISSUER/AUTH_JWT_AUDIENCE were never set in the
-// deployed manifests, so the issuer minted iss-less tokens and the
-// verifier compared aud against "" — every token "verified" against an
-// empty audience check, which is a silent authorization hole (any
-// signed token, for any audience, would have passed), not a visible
-// failure. A verifier that quietly 401s everything, or an issuer that
-// mints structurally-incomplete tokens, is worse than a crash-loop that
-// names the exact missing variable — this is api-service's own
-// mode-aware check, kept out of internal/config's shared Load() because
-// AUTH_JWT_AUDIENCE is meaningless to cmd/idp-connector (which has its
-// own ConnectorJWTAudience instead) and a blanket check in the shared
-// loader would wrongly fail that binary's startup too.
+// validateAuthConfig catches exactly the two live-review findings that
+// motivated it — first AUTH_JWT_ISSUER/AUTH_JWT_AUDIENCE, then
+// AUTH_JWKS_URL, silently absent from the deployed manifests, each time
+// letting a verifier or issuer come up cleanly and fail in a way that
+// looked like an ordinary token-rejection rather than a configuration
+// gap. A verifier that quietly 401s everything, or an issuer that mints
+// structurally-incomplete tokens, is worse than a crash-loop that names
+// the exact missing variable.
//
-// AUTH_JWT_ISSUER is required in BOTH modes: the verifier checks it on
-// every inbound token, and the issuer stamps it into every token it
-// mints. AUTH_JWT_AUDIENCE is required only in verifier mode — the
-// issuer never reads its own audience config; it mints each token's
-// `aud` from the per-client oauth_client.audience column instead
-// (tokenhandler.go), so requiring it here would reject a valid issuer
-// configuration that simply has no use for the variable.
-func validateAuthConfig(cfg config.Config, mode app.Mode) error {
- if cfg.AuthJWTIssuer == "" {
- return fmt.Errorf("AUTH_JWT_ISSUER must be set in %s mode", mode)
- }
- if mode == app.ModeVerifier && cfg.AuthJWTAudience == "" {
- return fmt.Errorf("AUTH_JWT_AUDIENCE must be set in verifier mode")
+// Reads config.RequiredAuthEnvVars(service, mode) — the single source of
+// truth also read by internal/config's own manifest test — rather than
+// hardcoding its own list, so this check and that test can never drift
+// apart from each other. Checks the raw environment directly (not cfg's
+// already-loaded fields): the manifest test checks for the same env var
+// NAMES in the manifest text, so both sides operate on the identical
+// primitive.
+//
+// Kept out of internal/config's shared Load() because AUTH_JWT_AUDIENCE
+// is meaningless to cmd/idp-connector (which has its own
+// ConnectorJWTAudience instead) and a blanket check in the shared loader
+// would wrongly fail that binary's startup too — idp-connector runs the
+// equivalent check itself, against its own required list.
+func validateAuthConfig(mode app.Mode) error {
+ for _, name := range config.RequiredAuthEnvVars("api-service", string(mode)) {
+ if os.Getenv(name) == "" {
+ return fmt.Errorf("%s must be set in %s mode", name, mode)
+ }
}
return nil
}
@@ -78,7 +79,7 @@ func main() {
if err != nil {
log.Fatalf("api-service: loading config: %v", err)
}
- if err := validateAuthConfig(cfg, app.Mode(cfg.AppMode)); err != nil {
+ if err := validateAuthConfig(app.Mode(cfg.AppMode)); err != nil {
log.Fatalf("api-service: %v", err)
}
diff --git a/cmd/api-service/main_test.go b/cmd/api-service/main_test.go
index ef07df0..17b6ecc 100644
--- a/cmd/api-service/main_test.go
+++ b/cmd/api-service/main_test.go
@@ -5,50 +5,43 @@ import (
"testing"
"loginid-takehome/internal/app"
- "loginid-takehome/internal/config"
)
// TestValidateAuthConfig_VerifierMode is the live-review finding's own
-// enforcement site: AUTH_JWT_ISSUER and AUTH_JWT_AUDIENCE both missing
-// from the deployed manifests let the verifier silently compare every
-// token's aud claim against "" — this must fail startup loudly instead,
-// naming exactly which variable is missing.
+// enforcement site: AUTH_JWT_ISSUER, AUTH_JWT_AUDIENCE, and AUTH_JWKS_URL
+// all missing from the deployed manifests (in two separate incidents)
+// let the verifier come up cleanly and fail in a way that looked like an
+// ordinary token rejection — this must fail startup loudly instead,
+// naming exactly which variable is missing. validateAuthConfig reads the
+// process environment directly (via config.RequiredAuthEnvVars), so
+// these tests set/unset real env vars rather than constructing a
+// config.Config.
func TestValidateAuthConfig_VerifierMode(t *testing.T) {
+ allSet := func(t *testing.T) {
+ t.Helper()
+ t.Setenv("AUTH_JWT_ISSUER", "https://issuer.example")
+ t.Setenv("AUTH_JWT_AUDIENCE", "loginid-api-service")
+ t.Setenv("AUTH_JWKS_URL", "http://api-service-issuer.loginid-takehome.svc.cluster.local:443/.well-known/jwks.json")
+ }
+
cases := []struct {
name string
- cfg config.Config
+ unset string // one var to blank out after allSet, "" for the all-set case
wantErr bool
wantErrSubstr string
}{
- {
- name: "both set: ok",
- cfg: config.Config{AuthJWTIssuer: "https://issuer.example", AuthJWTAudience: "loginid-api-service"},
- wantErr: false,
- },
- {
- name: "issuer missing",
- cfg: config.Config{AuthJWTIssuer: "", AuthJWTAudience: "loginid-api-service"},
- wantErr: true,
- wantErrSubstr: "AUTH_JWT_ISSUER",
- },
- {
- name: "audience missing",
- cfg: config.Config{AuthJWTIssuer: "https://issuer.example", AuthJWTAudience: ""},
- wantErr: true,
- wantErrSubstr: "AUTH_JWT_AUDIENCE",
- },
- {
- name: "both missing",
- cfg: config.Config{},
- wantErr: true,
- // Either name is acceptable — the point is a loud, named
- // failure, not a specific ordering between the two checks.
- wantErrSubstr: "AUTH_JWT_ISSUER",
- },
+ {name: "all set: ok", wantErr: false},
+ {name: "issuer missing", unset: "AUTH_JWT_ISSUER", wantErr: true, wantErrSubstr: "AUTH_JWT_ISSUER"},
+ {name: "audience missing", unset: "AUTH_JWT_AUDIENCE", wantErr: true, wantErrSubstr: "AUTH_JWT_AUDIENCE"},
+ {name: "jwks url missing", unset: "AUTH_JWKS_URL", wantErr: true, wantErrSubstr: "AUTH_JWKS_URL"},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
- err := validateAuthConfig(c.cfg, app.ModeVerifier)
+ allSet(t)
+ if c.unset != "" {
+ t.Setenv(c.unset, "")
+ }
+ err := validateAuthConfig(app.ModeVerifier)
if c.wantErr && err == nil {
t.Fatalf("validateAuthConfig() = nil, want an error")
}
@@ -63,30 +56,29 @@ func TestValidateAuthConfig_VerifierMode(t *testing.T) {
}
// TestValidateAuthConfig_IssuerMode confirms issuer mode only requires
-// AUTH_JWT_ISSUER — AUTH_JWT_AUDIENCE is meaningless here (the issuer
-// mints each token's aud from the per-client oauth_client.audience
-// column, not from global config), so an issuer Deployment with no
-// AUTH_JWT_AUDIENCE set at all must NOT be rejected.
+// AUTH_JWT_ISSUER — AUTH_JWT_AUDIENCE and AUTH_JWKS_URL are meaningless
+// here (the issuer mints each token's aud from the per-client
+// oauth_client.audience column, not from global config, and never
+// verifies inbound tokens so it has no JWKS to fetch), so an issuer
+// Deployment with neither set at all must NOT be rejected.
func TestValidateAuthConfig_IssuerMode(t *testing.T) {
cases := []struct {
- name string
- cfg config.Config
- wantErr bool
+ name string
+ setIssuer bool
+ wantErr bool
}{
- {
- name: "issuer set, audience empty: ok",
- cfg: config.Config{AuthJWTIssuer: "https://issuer.example", AuthJWTAudience: ""},
- wantErr: false,
- },
- {
- name: "issuer missing",
- cfg: config.Config{AuthJWTIssuer: ""},
- wantErr: true,
- },
+ {name: "issuer set, audience/jwks unset: ok", setIssuer: true, wantErr: false},
+ {name: "issuer missing", setIssuer: false, wantErr: true},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
- err := validateAuthConfig(c.cfg, app.ModeIssuer)
+ t.Setenv("AUTH_JWT_ISSUER", "")
+ t.Setenv("AUTH_JWT_AUDIENCE", "")
+ t.Setenv("AUTH_JWKS_URL", "")
+ if c.setIssuer {
+ t.Setenv("AUTH_JWT_ISSUER", "https://issuer.example")
+ }
+ err := validateAuthConfig(app.ModeIssuer)
if c.wantErr && err == nil {
t.Fatalf("validateAuthConfig() = nil, want an error")
}
diff --git a/cmd/idp-connector/main.go b/cmd/idp-connector/main.go
index 5f26a0a..6f28baf 100644
--- a/cmd/idp-connector/main.go
+++ b/cmd/idp-connector/main.go
@@ -5,19 +5,44 @@
package main
import (
+ "fmt"
"log"
"net/http"
+ "os"
"loginid-takehome/internal/app"
"loginid-takehome/internal/config"
"loginid-takehome/internal/connector"
)
+// validateAuthConfig catches the live-review finding this binary's
+// manifest actually hit: AUTH_JWKS_URL and AUTH_JWT_ISSUER were both
+// silently absent from idp-connector's Deployment, so every request
+// 401'd in a way indistinguishable from an ordinary rejected token — the
+// PM's own postmortem note was that cmd/api-service's equivalent check
+// (added after ITS OWN identical incident) only protects that binary,
+// not this one, since nothing shared the requirement between them until
+// now. Reads config.RequiredAuthEnvVars("idp-connector", "") — the same
+// single source of truth internal/config's own manifest test reads —
+// rather than a hardcoded list of its own, so this check and that test
+// can never drift apart.
+func validateAuthConfig() error {
+ for _, name := range config.RequiredAuthEnvVars("idp-connector", "") {
+ if os.Getenv(name) == "" {
+ return fmt.Errorf("%s must be set", name)
+ }
+ }
+ return nil
+}
+
func main() {
cfg, err := config.Load()
if err != nil {
log.Fatalf("idp-connector: loading config: %v", err)
}
+ if err := validateAuthConfig(); err != nil {
+ log.Fatalf("idp-connector: %v", err)
+ }
addr := cfg.HTTPAddr
if addr == "" {
diff --git a/cmd/idp-connector/main_test.go b/cmd/idp-connector/main_test.go
new file mode 100644
index 0000000..13cf774
--- /dev/null
+++ b/cmd/idp-connector/main_test.go
@@ -0,0 +1,50 @@
+package main
+
+import (
+ "strings"
+ "testing"
+)
+
+// TestValidateAuthConfig is the live-review finding's own enforcement
+// site: AUTH_JWKS_URL and AUTH_JWT_ISSUER were both silently absent from
+// idp-connector's Deployment manifest, so every request 401'd in a way
+// indistinguishable from an ordinary rejected token — this must fail
+// startup loudly instead, naming exactly which variable is missing.
+func TestValidateAuthConfig(t *testing.T) {
+ allSet := func(t *testing.T) {
+ t.Helper()
+ t.Setenv("AUTH_JWT_ISSUER", "https://issuer.example")
+ t.Setenv("AUTH_JWKS_URL", "http://api-service-issuer.loginid-takehome.svc.cluster.local:443/.well-known/jwks.json")
+ t.Setenv("CONNECTOR_JWT_AUDIENCE", "idp-connector-service")
+ }
+
+ cases := []struct {
+ name string
+ unset string
+ wantErr bool
+ wantErrSubstr string
+ }{
+ {name: "all set: ok", wantErr: false},
+ {name: "issuer missing", unset: "AUTH_JWT_ISSUER", wantErr: true, wantErrSubstr: "AUTH_JWT_ISSUER"},
+ {name: "jwks url missing", unset: "AUTH_JWKS_URL", wantErr: true, wantErrSubstr: "AUTH_JWKS_URL"},
+ {name: "connector audience missing", unset: "CONNECTOR_JWT_AUDIENCE", wantErr: true, wantErrSubstr: "CONNECTOR_JWT_AUDIENCE"},
+ }
+ for _, c := range cases {
+ t.Run(c.name, func(t *testing.T) {
+ allSet(t)
+ if c.unset != "" {
+ t.Setenv(c.unset, "")
+ }
+ err := validateAuthConfig()
+ if c.wantErr && err == nil {
+ t.Fatalf("validateAuthConfig() = nil, want an error")
+ }
+ if !c.wantErr && err != nil {
+ t.Fatalf("validateAuthConfig() = %v, want nil", err)
+ }
+ if c.wantErr && !strings.Contains(err.Error(), c.wantErrSubstr) {
+ t.Errorf("error = %q, want it to name %q", err, c.wantErrSubstr)
+ }
+ })
+ }
+}
diff --git a/go.mod b/go.mod
index 4c0cdcb..c456c7b 100644
--- a/go.mod
+++ b/go.mod
@@ -8,6 +8,7 @@ require (
github.com/jackc/pgx/v5 v5.11.0
github.com/pressly/goose/v3 v3.28.0
golang.org/x/crypto v0.57.0
+ gopkg.in/yaml.v3 v3.0.1
modernc.org/sqlite v1.58.0
)
@@ -16,10 +17,12 @@ require (
github.com/jackc/pgpassfile v1.0.0 // indirect
github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect
github.com/jackc/puddle/v2 v2.2.2 // indirect
+ github.com/kr/text v0.2.0 // indirect
github.com/mattn/go-isatty v0.0.24 // indirect
github.com/mfridman/interpolate v0.0.2 // indirect
github.com/ncruces/go-strftime v1.0.0 // indirect
github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect
+ github.com/rogpeppe/go-internal v1.16.0 // indirect
github.com/sethvargo/go-retry v0.4.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
golang.org/x/sync v0.23.0 // indirect
diff --git a/go.sum b/go.sum
index 56a5ed3..91f7a49 100644
--- a/go.sum
+++ b/go.sum
@@ -1,3 +1,4 @@
+github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY=
github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto=
@@ -17,6 +18,10 @@ github.com/jackc/pgx/v5 v5.11.0 h1:IzBBtyK9AHqf98cctWFifYSci2hgQR/cd56wB4p+ogg=
github.com/jackc/pgx/v5 v5.11.0/go.mod h1:mal1tBGAFfLHvZzaYh77YS/eC6IX9OWbRV1QIIM0Jn4=
github.com/jackc/puddle/v2 v2.2.2 h1:PR8nw+E/1w0GLuRFSmiioY6UooMp6KJv0/61nB7icHo=
github.com/jackc/puddle/v2 v2.2.2/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4=
+github.com/kr/pretty v0.3.0 h1:WgNl7dwNpEZ6jJ9k1snq4pZsg7DOEN8hP9Xw0Tsjwk0=
+github.com/kr/pretty v0.3.0/go.mod h1:640gp4NfQd8pI5XOwp5fnNeVWj67G7CFk/SaSQn7NBk=
+github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
+github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
github.com/mattn/go-isatty v0.0.24 h1:tGZZoVgT/KiqK1c8ocVLeDS8BSWMRd47J3Lbz7vsReI=
github.com/mattn/go-isatty v0.0.24/go.mod h1:nMCL3Zebbrt45jsMDgnfIwz6ydEQApk5oEI3HqDio6A=
github.com/mfridman/interpolate v0.0.2 h1:pnuTK7MQIxxFz1Gr+rjSIx9u7qVjf5VOoM/u6BbAxPY=
@@ -28,6 +33,8 @@ github.com/pressly/goose/v3 v3.28.0 h1:D2M+iL31GmpZxSHOhX8mqyqAT3CXnokUmm0eKoSP+
github.com/pressly/goose/v3 v3.28.0/go.mod h1:v26MOuB8bL3kzzrt3Vqhb3R0PRVsl8hFQKdrht/L6Rk=
github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec h1:W09IVJc94icq4NjY3clb7Lk8O1qJ8BdBEF8z0ibU0rE=
github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo=
+github.com/rogpeppe/go-internal v1.16.0 h1:O9DK+vNMDVGLr2BeZqmpLeMjiMNkuXfcqntWbZV6S5g=
+github.com/rogpeppe/go-internal v1.16.0/go.mod h1:DrUVZyrJU+txYW5/1kwtXQSMFio52ZOxX7yM1VHvnxs=
github.com/sethvargo/go-retry v0.4.0 h1:9qy1OoIAxBL+gBYnkTnTnWle5wlfsXQlwRzIbbpdqPw=
github.com/sethvargo/go-retry v0.4.0/go.mod h1:tvsjdKG6xfiCx4LSiUZ06kcv38xvdVQwv8R6/VnnVWg=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
@@ -52,7 +59,11 @@ golang.org/x/text v0.42.0/go.mod h1:ojzP1Z+2QtioaF8DTtO8K5q7JWVVYwZKenzujK0Zd0E=
golang.org/x/tools v0.49.0 h1:3NI7VXzL9+1WZD52Dx2ttoPwD5DWrFGpl9mFZDlmisI=
golang.org/x/tools v0.49.0/go.mod h1:SJNXV9DBKT0UbdttsQjbfJlAE/q+y36++zo3uL3N0Oo=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
+gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
+gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
+gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
modernc.org/cc/v4 v4.29.2 h1:h6+9ciCnPKutf4I03CvheAvDLX7+IHlqR6Iy6J+cgd8=
modernc.org/cc/v4 v4.29.2/go.mod h1:OnovgIhbbMXMu1aISnJ0wvVD1KnW+cAUJkIrAWh+kVI=
modernc.org/ccgo/v4 v4.35.0 h1:F+TUsmw09QxLzmi3aeYYGxjAXarmZaKgj3mKQHNaA8w=
diff --git a/internal/config/config.go b/internal/config/config.go
index 55e9409..8c31f4a 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -218,3 +218,51 @@ func resolveDSNFrom(fileEnv, plainEnv string) (string, error) {
}
return os.Getenv(plainEnv), nil
}
+
+// RequiredAuthEnvVars is the single source of truth for which auth-related
+// env vars a given (service, mode) combination MUST have set to function
+// correctly — read by both this process's own startup validation
+// (cmd/api-service/main.go's validateAuthConfig, cmd/idp-connector/main.go's
+// equivalent) and, independently, by internal/config's own
+// TestManifestsSetRequiredAuthEnvVars, which parses the actual deployed
+// manifests (deploy/api-service.yaml, deploy/manifests.yaml) and asserts
+// every Deployment sets everything its own (service, mode) requires.
+//
+// Exists because of two separate live-review incidents in one day, both
+// the same shape: a real, correctly-signed token failed verification
+// because a required auth env var was silently absent from a
+// Deployment's manifest, and nothing caught it before a human noticed
+// the symptom in production. cmd/api-service/main.go's own
+// validateAuthConfig (added after the first incident) only checks the
+// binary it lives in — it has no way to know cmd/idp-connector's
+// Deployment was missing AUTH_JWKS_URL/AUTH_JWT_ISSUER entirely (the
+// second incident), because idp-connector never runs that check at all
+// until this same list is wired into its own main() too. And neither
+// check, however complete, can catch a manifest that's simply missing
+// the variable outright before a real request exercises the gap — only
+// a test that reads the manifest text itself can do that ahead of time,
+// which is what this declaration exists to make possible.
+//
+// service is "api-service" or "idp-connector" — the binary, not the
+// Deployment name (api-service and api-service-issuer are the SAME
+// binary in different modes). mode is "verifier"/"issuer" for
+// api-service (matching AppMode's own two values), or "" for
+// idp-connector, which has no mode concept.
+func RequiredAuthEnvVars(service, mode string) []string {
+ switch {
+ case service == "api-service" && mode == "verifier":
+ return []string{"AUTH_JWT_ISSUER", "AUTH_JWT_AUDIENCE", "AUTH_JWKS_URL"}
+ case service == "api-service" && mode == "issuer":
+ // AUTH_JWT_AUDIENCE deliberately absent here — the issuer mints
+ // each token's aud from the per-client oauth_client.audience
+ // column (tokenhandler.go), never from its own config, so
+ // requiring it would reject a valid issuer configuration for a
+ // variable it has no use for (validateAuthConfig's own doc
+ // comment states this same reasoning).
+ return []string{"AUTH_JWT_ISSUER"}
+ case service == "idp-connector":
+ return []string{"AUTH_JWT_ISSUER", "AUTH_JWKS_URL", "CONNECTOR_JWT_AUDIENCE"}
+ default:
+ return nil
+ }
+}
diff --git a/internal/config/manifest_test.go b/internal/config/manifest_test.go
new file mode 100644
index 0000000..dab49c8
--- /dev/null
+++ b/internal/config/manifest_tes
… truncated (20 KB cap)