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") + } +}