From 80d2092f1b26279ccb5f81d9699fb6376a929b53 Mon Sep 17 00:00:00 2001 From: mrambossek Date: Sat, 1 Aug 2026 17:31:33 +0200 Subject: [PATCH] oidc: accept both the app's public client and the server's confidential one Explaining public vs confidential clients surfaced a gap in my own design: I had assumed a single client id, but there are two clients here with genuinely different properties. the Android app public + PKCE, because an APK cannot keep a secret the admin UI confidential, because the server can keep one in /etc/echolot-server.env and weakening it to public buys nothing So the audience check now accepts either registered client id - and only those two. "Any client of this issuer" would let every other application registered with the same IdP authenticate here, which is the entire reason the check exists. Either id alone is enough to enable sign-in, since an operator may register only the app or only the admin UI. The profile advertises the *app's* client id, since that is what a phone should authorize as. Co-Authored-By: Claude Fable 5 --- server/cmd/echolot-server/main.go | 12 +++++---- server/internal/config/config.go | 10 +++++--- server/internal/control/control.go | 7 +++--- server/internal/oidc/oidc.go | 37 +++++++++++++++++++++++++--- server/internal/oidc/oidc_test.go | 39 +++++++++++++++++++++++++++++- 5 files changed, 88 insertions(+), 17 deletions(-) diff --git a/server/cmd/echolot-server/main.go b/server/cmd/echolot-server/main.go index 6f859cd..fe11270 100644 --- a/server/cmd/echolot-server/main.go +++ b/server/cmd/echolot-server/main.go @@ -159,14 +159,16 @@ func serve(cfg *config.Config) error { // uploads=account can never be satisfied — which is the honest outcome, not a silent // downgrade to anonymous. var idp *oidc.Verifier - if cfg.OIDCIssuer != "" && cfg.OIDCClientID != "" { + if cfg.OIDCIssuer != "" && (cfg.OIDCClientID != "" || cfg.OIDCAppClientID != "") { idp = oidc.New(oidc.Config{ - Issuer: cfg.OIDCIssuer, - ClientID: cfg.OIDCClientID, - AdminGroup: cfg.OIDCAdminGroup, + Issuer: cfg.OIDCIssuer, + ClientID: cfg.OIDCClientID, + AppClientID: cfg.OIDCAppClientID, + AdminGroup: cfg.OIDCAdminGroup, }, nil) slog.Info("identity provider configured", "issuer", cfg.OIDCIssuer, - "client_id", cfg.OIDCClientID, "admin_group", cfg.OIDCAdminGroup) + "admin_client_id", cfg.OIDCClientID, "app_client_id", cfg.OIDCAppClientID, + "admin_group", cfg.OIDCAdminGroup) if cfg.OIDCAdminGroup == "" { slog.Warn("no admin group set: nobody will be an admin via OIDC " + "(set ECHOLOT_OIDC_ADMIN_GROUP)") diff --git a/server/internal/config/config.go b/server/internal/config/config.go index c2aee05..6779475 100644 --- a/server/internal/config/config.go +++ b/server/internal/config/config.go @@ -69,9 +69,10 @@ type Config struct { // Identity provider. Empty issuer disables sign-in entirely; the server is a relying // party and never stores passwords. - OIDCIssuer string // ECHOLOT_OIDC_ISSUER / --oidc-issuer - OIDCClientID string // ECHOLOT_OIDC_CLIENT_ID / --oidc-client-id - OIDCAdminGroup string // ECHOLOT_OIDC_ADMIN_GROUP / --oidc-admin-group + OIDCIssuer string // ECHOLOT_OIDC_ISSUER / --oidc-issuer + OIDCClientID string // ECHOLOT_OIDC_CLIENT_ID / --oidc-client-id (confidential, admin UI) + OIDCAppClientID string // ECHOLOT_OIDC_APP_CLIENT_ID / --oidc-app-client-id (public, the phone app) + OIDCAdminGroup string // ECHOLOT_OIDC_ADMIN_GROUP / --oidc-admin-group // Break-glass admin username; the password lives hashed in the state store. AdminUser string // ECHOLOT_ADMIN_USER / --admin-user @@ -125,7 +126,8 @@ func Load(args []string) (*Config, *Actions, error) { fs.IntVar(&c.UploadMaxRuns, "upload-max-runs", envInt("UPLOAD_MAX_RUNS", 200), "keep at most this many runs per device; 0 disables") fs.StringVar(&c.UploadMinAnon, "upload-min-anonymization", envOr("UPLOAD_MIN_ANONYMIZATION", "full"), "least anonymization accepted: full|balanced|strict") fs.StringVar(&c.OIDCIssuer, "oidc-issuer", envOr("OIDC_ISSUER", ""), "OpenID Connect issuer URL; empty disables sign-in") - fs.StringVar(&c.OIDCClientID, "oidc-client-id", envOr("OIDC_CLIENT_ID", ""), "OpenID Connect client id for this server") + fs.StringVar(&c.OIDCClientID, "oidc-client-id", envOr("OIDC_CLIENT_ID", ""), "confidential OIDC client id for the admin UI") + fs.StringVar(&c.OIDCAppClientID, "oidc-app-client-id", envOr("OIDC_APP_CLIENT_ID", ""), "public OIDC client id used by the Android app (PKCE)") fs.StringVar(&c.OIDCAdminGroup, "oidc-admin-group", envOr("OIDC_ADMIN_GROUP", ""), "group claim required for admin access; empty means nobody is an admin via OIDC") fs.StringVar(&c.PublicControlURL, "public-url", envOr("PUBLIC_URL", ""), "public control-plane URL for enrollment links, e.g. https://probe.example.net:8443") fs.StringVar(&c.MinAppVersion, "min-app-version", envOr("MIN_APP_VERSION", "0.2.0"), "oldest app version this server will serve (SemVer, inclusive)") diff --git a/server/internal/control/control.go b/server/internal/control/control.go index 9053fe2..4ec3b14 100644 --- a/server/internal/control/control.go +++ b/server/internal/control/control.go @@ -828,9 +828,10 @@ func (s *Server) authInfo(ctx context.Context) map[string]any { } cfg := s.OIDC.Config() out := map[string]any{ - "enabled": true, - "issuer": cfg.Issuer, - "client_id": cfg.ClientID, + "enabled": true, + "issuer": cfg.Issuer, + // The app's client, not the server's: this is what a phone should authorize as. + "client_id": cfg.AppClientID, // The app is a public client on a phone: no secret can be kept, so PKCE is what // protects the code exchange (RFC 7636), and the redirect comes back through the // scheme the app already registers for enrollment links. diff --git a/server/internal/oidc/oidc.go b/server/internal/oidc/oidc.go index 608b9f9..e8f30bb 100644 --- a/server/internal/oidc/oidc.go +++ b/server/internal/oidc/oidc.go @@ -97,8 +97,16 @@ func (a audience) contains(s string) bool { type Config struct { // Issuer is the IdP's base URL, e.g. https://auth.example.net/application/o/echolot/ Issuer string - // ClientID is this server's registered client. Tokens must be addressed to it. + // ClientID is this server's own registered client — confidential, used for the admin UI's + // browser login, where a secret can genuinely be kept in the host's config. ClientID string + // AppClientID is the mobile app's registered client. It is a separate, *public* client + // because an APK cannot keep a secret, so it uses PKCE instead. + // + // Both are accepted as audiences, and they must be listed rather than merged: a token is + // addressed to a specific client, and accepting "any client of this issuer" would let every + // other application registered with the same IdP authenticate here. + AppClientID string // AdminGroup, when set, is the group claim a person must hold to reach the admin UI. // Empty means no one is an admin via OIDC, which is the safe default: an operator who has // not said who may administer the server has not said "everyone". @@ -107,7 +115,27 @@ type Config struct { Skew time.Duration } -func (c Config) Enabled() bool { return c.Issuer != "" && c.ClientID != "" } +func (c Config) Enabled() bool { return c.Issuer != "" && (c.ClientID != "" || c.AppClientID != "") } + +// acceptedAudiences is every client id this server answers for. +func (v *Verifier) acceptedAudiences() []string { + out := make([]string, 0, 2) + for _, id := range []string{v.cfg.ClientID, v.cfg.AppClientID} { + if id != "" { + out = append(out, id) + } + } + return out +} + +func (v *Verifier) audienceAccepted(aud audience) bool { + for _, id := range v.acceptedAudiences() { + if aud.contains(id) { + return true + } + } + return false +} // Discovery is the subset of the provider metadata document that is used. type Discovery struct { @@ -362,8 +390,9 @@ func (v *Verifier) checkClaims(c Claims) error { } // A token addressed to a different client is a valid token that was not meant for us — // accepting it lets any other client of the same IdP authenticate here. - if !c.Audience.contains(v.cfg.ClientID) { - return fmt.Errorf("%w: addressed to %v, not to %q", ErrClaims, []string(c.Audience), v.cfg.ClientID) + if !v.audienceAccepted(c.Audience) { + return fmt.Errorf("%w: addressed to %v, not to %v", ErrClaims, + []string(c.Audience), v.acceptedAudiences()) } if c.Subject == "" { return fmt.Errorf("%w: no subject", ErrClaims) diff --git a/server/internal/oidc/oidc_test.go b/server/internal/oidc/oidc_test.go index 661fa33..22c929f 100644 --- a/server/internal/oidc/oidc_test.go +++ b/server/internal/oidc/oidc_test.go @@ -115,7 +115,9 @@ func (i *testIdP) claims(extra map[string]any) map[string]any { } func verifier(i *testIdP, adminGroup string) *Verifier { - return New(Config{Issuer: i.URL, ClientID: "echolot", AdminGroup: adminGroup}, i.Client()) + return New(Config{ + Issuer: i.URL, ClientID: "echolot", AppClientID: "echolot-app", AdminGroup: adminGroup, + }, i.Client()) } func TestAcceptsAGenuineToken(t *testing.T) { @@ -286,3 +288,38 @@ func TestDisabledWithoutConfiguration(t *testing.T) { t.Fatalf("want ErrDisabled, got %v", err) } } + +// Two clients, because the phone and the admin UI have different properties: an APK cannot keep a +// secret (public + PKCE) while the server can (confidential). Both must be accepted — but only +// those two. "Any client of this issuer" would let every other application registered with the +// same IdP authenticate here, which is the whole reason the audience check exists. +func TestBothRegisteredClientsAreAccepted(t *testing.T) { + idp := newIdP(t) + v := verifier(idp, "") + + for _, aud := range []any{"echolot", "echolot-app", []string{"echolot-app", "other"}} { + tok := idp.sign(t, "RS256", "rsa-1", idp.claims(map[string]any{"aud": aud})) + if _, err := v.Verify(context.Background(), tok); err != nil { + t.Errorf("aud %v was refused: %v", aud, err) + } + } + // A third application at the same issuer is still not us. + tok := idp.sign(t, "RS256", "rsa-1", idp.claims(map[string]any{"aud": "someone-elses-app"})) + if _, err := v.Verify(context.Background(), tok); !errors.Is(err, ErrClaims) { + t.Fatalf("a third client's token was accepted: %v", err) + } +} + +// Either client id alone is enough to make sign-in usable: an operator may register only the app +// (no admin UI login) or only the server. +func TestEitherClientIDAloneEnablesSignIn(t *testing.T) { + if !(Config{Issuer: "https://i", ClientID: "a"}).Enabled() { + t.Error("a server-only configuration was reported disabled") + } + if !(Config{Issuer: "https://i", AppClientID: "b"}).Enabled() { + t.Error("an app-only configuration was reported disabled") + } + if (Config{Issuer: "https://i"}).Enabled() { + t.Error("an issuer with no client at all was reported enabled") + } +}