From 3186df16f42eeb4a4170a5c27478d7097e7bae73 Mon Sep 17 00:00:00 2001 From: KR Ravindra <42912207+KR-Ravindra@users.noreply.github.com> Date: Sat, 3 Oct 2026 09:47:39 -0700 Subject: [PATCH] Add serving_cert_source section to the OIDC Discovery Provider Add a labeled `serving_cert_source` section that selects where the certificate used to serve HTTPS comes from, with the "acme", "cert_file" and "workload_api" variants. The "workload_api" variant obtains an X509-SVID from the SPIFFE Workload API and keeps it up to date as it is rotated, which removes the need for a sidecar to feed the provider a certificate. The existing `acme` and `serving_cert_file` sections keep working and log a deprecation warning at startup. They cannot be combined with the new section. The health checks server is started before the listener is built, with /ready reporting not ready until the listener is up, so that liveness and readiness probes answer while the provider waits for its first X509-SVID. Workload API errors are logged through the go-spiffe logger while waiting, and a shutdown request during the wait exits cleanly. Signed-off-by: KR Ravindra <42912207+KR-Ravindra@users.noreply.github.com> --- support/oidc-discovery-provider/README.md | 62 ++++++- support/oidc-discovery-provider/config.go | 140 ++++++++++++++- .../config_posix_test.go | 2 +- .../oidc-discovery-provider/config_test.go | 166 ++++++++++++++++++ .../config_windows_test.go | 2 +- .../healthchecks_handler.go | 13 +- .../healthchecks_handler_test.go | 31 +++- support/oidc-discovery-provider/main.go | 126 ++++++++++++- support/oidc-discovery-provider/main_posix.go | 28 ++- .../main_posix_test.go | 103 +++++++++++ .../oidc-discovery-provider/main_windows.go | 27 ++- 11 files changed, 661 insertions(+), 39 deletions(-) create mode 100644 support/oidc-discovery-provider/main_posix_test.go diff --git a/support/oidc-discovery-provider/README.md b/support/oidc-discovery-provider/README.md index 60141bc60f..c95bb25ed3 100644 --- a/support/oidc-discovery-provider/README.md +++ b/support/oidc-discovery-provider/README.md @@ -36,8 +36,9 @@ The configuration file is **required** by the provider. It contains | Key | Type | Required? | Description | Default | | ----------------------- | ------- | ------------------ | ---------------------------------------------------------------------------------------------- | -------- | -| `acme` | section | required[1] | Provides the ACME configuration. | | -| `serving_cert_file` | section | required\[1\]\[4\] | Provides the serving certificate configuration. | | +| `serving_cert_source` | section | required\[1\] | Provides the [serving certificate source](#serving-certificate-source-section) configuration. | | +| `acme` | section | required[1] | Provides the ACME configuration. Deprecated, use `serving_cert_source "acme"` instead. | | +| `serving_cert_file` | section | required\[1\]\[4\] | Serving certificate configuration. Deprecated, use `serving_cert_source "cert_file"` instead. | | | `allow_insecure_scheme` | bool | optional\[3\] | Serves OIDC configuration response with HTTP url. A warning is logged at startup when enabled. | `false` | | `domains` | strings | required | One or more domains the provider is being served from. | | | `experimental` | section | optional | The experimental options that are subject to change or removal. | | @@ -56,7 +57,7 @@ The configuration file is **required** by the provider. It contains | `jwt_issuer` | string | optional | Specifies the issuer for the OIDC provider configuration request | | | `jwks_uri` | string | optional | Specifies the JWKS URI returned in the discovery document | | | `server_path_prefix` | string | optional | If specified, all endpoints listened to will be prefixed by this value | `"/"` | -| `tls_config` | section | optional | TLS config for terminating HTTPS listeners (disk certificate and ACME modes). | | +| `tls_config` | section | optional | TLS config for terminating HTTPS listeners (disk certificate, ACME and Workload API modes). | | | experimental | Type | Required? | Description | Default | |--------------------------|--------|--------------------|------------------------------------------------------|---------| @@ -80,13 +81,13 @@ the only way to bound growth of `log_path`. #### Considerations for Unix platforms -[1]: One of `acme`, `serving_cert_file` or `listen_socket_path` must be defined. +[1]: One of `serving_cert_source`, `acme`, `serving_cert_file` or `listen_socket_path` must be defined. [3]: The `allow_insecure_scheme` should only be enabled when the network path to the provider is trusted end-to-end (for example, when TLS is terminated at a trusted reverse proxy or load balancer on a private network). It only works in conjunction with `insecure_addr` or `listen_socket_path`. #### Considerations for Windows platforms -[1]: One of `acme`, `serving_cert_file` or `listen_named_pipe_name` must be defined. +[1]: One of `serving_cert_source`, `acme`, `serving_cert_file` or `listen_named_pipe_name` must be defined. [3]: The `allow_insecure_scheme` should only be enabled when the network path to the provider is trusted end-to-end (for example, when TLS is terminated at a trusted reverse proxy or load balancer on a private network). It only works in conjunction with `insecure_addr` or `listen_named_pipe_name`. @@ -103,6 +104,41 @@ will terminate if another domain is requested. [4]: SPIRE OIDC Discovery provider monitors and reloads the files provided in the `serving_cert_file` configuration at runtime. +#### Serving Certificate Source Section + +The `serving_cert_source` section selects where the certificate used to serve +HTTPS comes from. The section label selects the source and the body holds its +configuration: `serving_cert_source "acme" {}` takes the options of the +[ACME Section](#acme-section), `serving_cert_source "cert_file" {}` takes the +options of the [Serving Certificate Section](#serving-certificate-section), and +`serving_cert_source "workload_api" {}` serves an X509-SVID obtained from the +SPIFFE Workload API, kept up to date as it is rotated, with the options below. +Only one `serving_cert_source` section can be configured, and it cannot be used +together with the deprecated `acme` and `serving_cert_file` sections. The +`workload_api` source cannot be combined with `insecure_addr`, +`listen_socket_path` or `listen_named_pipe_name`; the provider fails to start +in that case. + +Note that clients need the trust domain's X.509 bundle to authenticate an +X509-SVID, and that they check the certificate's DNS names against the host +they connect to. An X509-SVID only carries the DNS names set on its +registration entry (`-dns` on `spire-server entry create`), so the entry of the +provider must set DNS names matching the configured `domains`; otherwise +clients such as the Kubernetes API server fail to fetch the JWKS. If the +Workload API returns more than one X509-SVID, the first one is served. + +| Key | Type | Required? | Description | Default | +|----------------|---------|---------------|---------------------------------------------------------------------------|---------| +| `experimental` | section | optional | The experimental options that are subject to change or removal. | | +| `socket_path` | string | required\[5\] | Path on disk to the Workload API Unix Domain socket. Unix platforms only. | | +| `addr` | string | optional | Exposes the service on the given address. | `:443` | + +| experimental | Type | Required? | Description | Default | +|:------------------|--------|---------------|---------------------------------------------------------|---------| +| `named_pipe_name` | string | required\[5\] | Pipe name of the Workload API named pipe. Windows only. | | + +[5]: Optional when the `workload_api` section is configured, in which case it defaults to the value configured in that section. + #### ACME Section | Key | Type | Required? | Description | Default | @@ -123,7 +159,7 @@ will terminate if another domain is requested. #### TLS Config Section -Applied to **terminating** HTTPS listeners when using `serving_cert_file` or `acme`. +Applied to **terminating** HTTPS listeners when using `serving_cert_source`, `serving_cert_file` or `acme`. Not applied to `insecure_addr`, `listen_socket_path`, named pipe modes, or any outbound TLS client connections. Parsed once at startup; invalid values prevent the provider from starting. @@ -245,6 +281,20 @@ workload_api { } ``` +#### Workload API and Serving Certificate from the Workload API + +```hcl +log_level = "debug" +domains = ["mypublicdomain.test"] +serving_cert_source "workload_api" { + addr = ":8443" +} +workload_api { + socket_path = "/tmp/spire-agent/public/api.sock" + trust_domain = "domain.test" +} +``` + #### Listening on a Unix Socket The following configuration has the OIDC Discovery Provider listen for requests diff --git a/support/oidc-discovery-provider/config.go b/support/oidc-discovery-provider/config.go index dee75af101..b4f83a6618 100644 --- a/support/oidc-discovery-provider/config.go +++ b/support/oidc-discovery-provider/config.go @@ -9,6 +9,7 @@ import ( "time" "github.com/hashicorp/hcl" + "github.com/hashicorp/hcl/hcl/ast" "github.com/spiffe/spire/pkg/common/config" spirelog "github.com/spiffe/spire/pkg/common/log" "github.com/spiffe/spire/pkg/common/tlspolicy" @@ -64,12 +65,25 @@ type Config struct { // on, for when deployed behind another webserver or sidecar. ListenSocketPath string `hcl:"listen_socket_path"` + // ServingCertSource is the configuration for the source of the certificate + // used to serve HTTPS. It is a labeled section whose label selects the + // source, e.g. `serving_cert_source "workload_api" {}`. It is required + // unless InsecureAddr, ListenSocketPath (ListenNamedPipeName on Windows) + // or one of the deprecated ACME or ServingCertFile sections is set. + ServingCertSource *ServingCertSourceConfig `hcl:"serving_cert_source"` + // ACME is the ACME configuration. It is required unless InsecureAddr or // ListenSocketPath is set, or if ServingCertFile is used. + // + // Deprecated: use ServingCertSource with the "acme" label instead. It is + // populated from that section by ParseConfig when it is used. ACME *ACMEConfig `hcl:"acme"` // ServingCertFile is the configuration for using a serving certificate to serve HTTPS. // It is required unless InsecureAddr or ListenSocketPath is set, or if ACME configuration is used. + // + // Deprecated: use ServingCertSource with the "cert_file" label instead. It + // is populated from that section by ParseConfig when it is used. ServingCertFile *ServingCertFileConfig `hcl:"serving_cert_file"` // ServerAPI is the configuration for using the SPIRE Server API as the @@ -107,6 +121,30 @@ type Config struct { ServerPathPrefix string `hcl:"server_path_prefix"` } +// ServingCertSourceConfig holds the configuration of the serving certificate +// source. The label of the serving_cert_source section is decoded into the +// field of the same name. Exactly one source must be configured. +type ServingCertSourceConfig struct { + // ACME obtains the serving certificate via ACME. + ACME *ACMEConfig `hcl:"acme"` + // CertFile loads the serving certificate and key from disk. + CertFile *ServingCertFileConfig `hcl:"cert_file"` + // WorkloadAPI serves an X509-SVID obtained from the SPIFFE Workload API. + WorkloadAPI *ServingCertWorkloadAPIConfig `hcl:"workload_api"` +} + +type ServingCertWorkloadAPIConfig struct { + // SocketPath is the path to the Workload API Unix Domain socket. It defaults + // to the socket path of the workload_api section when that is configured. + SocketPath string `hcl:"socket_path"` + // Addr is the address to listen on. This is optional and defaults to ":443". + Addr *net.TCPAddr `hcl:"-"` + // RawAddr holds the string version of the Addr. Consumers should use Addr instead. + RawAddr string `hcl:"addr"` + // Experimental options that are subject to change or removal. + Experimental experimentalWorkloadAPIConfig `hcl:"experimental"` +} + type ServingCertFileConfig struct { // CertFilePath is the path to the certificate file. The provider will watch // this file for changes and reload the certificate when it changes. @@ -261,6 +299,33 @@ func ParseConfig(hclConfig string) (_ *Config, err error) { } c.Domains = dedupeList(c.Domains) + if err := validateServingCertSourceSections(hclConfig); err != nil { + return nil, err + } + if c.ServingCertSource != nil { + if c.ACME != nil || c.ServingCertFile != nil { + return nil, errors.New("the acme and serving_cert_file sections cannot be used together with the serving_cert_source section") + } + // The deprecated acme and serving_cert_file sections are the legacy + // spelling of the "acme" and "cert_file" sources. Alias them so the + // rest of the provider has a single place to look. + c.ACME = c.ServingCertSource.ACME + c.ServingCertFile = c.ServingCertSource.CertFile + + if workloadAPI := c.ServingCertSource.WorkloadAPI; workloadAPI != nil { + if workloadAPI.RawAddr == "" { + workloadAPI.RawAddr = defaultAddr + } + workloadAPI.Addr, err = net.ResolveTCPAddr("tcp", workloadAPI.RawAddr) + if err != nil { + return nil, fmt.Errorf(`invalid addr in the serving_cert_source "workload_api" configuration section: %w`, err) + } + } + } + + // Name the section the user actually wrote in the error messages. + acmeSection, certFileSection := c.acmeSectionName(), c.certFileSectionName() + if c.ACME != nil { c.ACME.CacheDir = defaultCacheDir if c.ACME.RawCacheDir != nil { @@ -268,20 +333,20 @@ func ParseConfig(hclConfig string) (_ *Config, err error) { } switch { case c.InsecureAddr != "": - return nil, errors.New("insecure_addr and the acme section are mutually exclusive") + return nil, fmt.Errorf("insecure_addr and the %s section are mutually exclusive", acmeSection) case !c.ACME.ToSAccepted: - return nil, errors.New("tos_accepted must be set to true in the acme configuration section") + return nil, fmt.Errorf("tos_accepted must be set to true in the %s configuration section", acmeSection) case c.ACME.Email == "": - return nil, errors.New("email must be configured in the acme configuration section") + return nil, fmt.Errorf("email must be configured in the %s configuration section", acmeSection) } } if c.ServingCertFile != nil { if c.ServingCertFile.CertFilePath == "" { - return nil, errors.New("cert_file_path must be configured in the serving_cert_file configuration section") + return nil, fmt.Errorf("cert_file_path must be configured in the %s configuration section", certFileSection) } if c.ServingCertFile.KeyFilePath == "" { - return nil, errors.New("key_file_path must be configured in the serving_cert_file configuration section") + return nil, fmt.Errorf("key_file_path must be configured in the %s configuration section", certFileSection) } if c.ServingCertFile.RawAddr == "" { @@ -290,13 +355,13 @@ func ParseConfig(hclConfig string) (_ *Config, err error) { addr, err := net.ResolveTCPAddr("tcp", c.ServingCertFile.RawAddr) if err != nil { - return nil, fmt.Errorf("invalid addr in the serving_cert_file configuration section: %w", err) + return nil, fmt.Errorf("invalid addr in the %s configuration section: %w", certFileSection, err) } c.ServingCertFile.Addr = addr c.ServingCertFile.FileSyncInterval, err = parseDurationField(c.ServingCertFile.RawFileSyncInterval, defaultFileSyncInterval) if err != nil { - return nil, fmt.Errorf("invalid file_sync_interval in the serving_cert_file configuration section: %w", err) + return nil, fmt.Errorf("invalid file_sync_interval in the %s configuration section: %w", certFileSection, err) } } @@ -376,6 +441,67 @@ func ParseConfig(hclConfig string) (_ *Config, err error) { return c, nil } +// validateServingCertSourceSections checks the serving_cert_source sections +// on the raw HCL. Decoding merges sections sharing a label and drops unknown +// labels, so duplicates and typos are only visible before decoding. +func validateServingCertSourceSections(hclConfig string) error { + root, err := hcl.Parse(hclConfig) + if err != nil { + return fmt.Errorf("unable to parse configuration: %w", err) + } + objectList, ok := root.Node.(*ast.ObjectList) + if !ok { + return errors.New("malformed configuration") + } + items := objectList.Filter("serving_cert_source").Items + switch len(items) { + case 0: + return nil + case 1: + default: + return errors.New("only one serving_cert_source section can be configured") + } + if len(items[0].Keys) != 1 { + return errors.New(`serving_cert_source must have exactly one label, e.g. serving_cert_source "workload_api" {}`) + } + label, ok := items[0].Keys[0].Token.Value().(string) + if !ok { + return errors.New("invalid serving_cert_source label") + } + switch label { + case "acme", "cert_file", "workload_api": + return nil + default: + return fmt.Errorf(`unknown serving_cert_source %q: must be one of "acme", "cert_file", or "workload_api"`, label) + } +} + +// servingCertWorkloadAPI returns the serving_cert_source "workload_api" +// configuration, or nil if it is not configured. +func (c *Config) servingCertWorkloadAPI() *ServingCertWorkloadAPIConfig { + if c.ServingCertSource == nil { + return nil + } + return c.ServingCertSource.WorkloadAPI +} + +// acmeSectionName and certFileSectionName name the section the user wrote +// in error messages: the "acme" and "cert_file" sources are aliased onto the +// deprecated acme and serving_cert_file sections. +func (c *Config) acmeSectionName() string { + if c.ServingCertSource != nil { + return `serving_cert_source "acme"` + } + return "acme" +} + +func (c *Config) certFileSectionName() string { + if c.ServingCertSource != nil { + return `serving_cert_source "cert_file"` + } + return "serving_cert_file" +} + func dedupeList(items []string) []string { keys := make(map[string]bool) var list []string diff --git a/support/oidc-discovery-provider/config_posix_test.go b/support/oidc-discovery-provider/config_posix_test.go index e1062a553d..44deb95575 100644 --- a/support/oidc-discovery-provider/config_posix_test.go +++ b/support/oidc-discovery-provider/config_posix_test.go @@ -58,7 +58,7 @@ func parseConfigCasesOS() []parseConfigCase { socket_path = "/other/socket/path" } `, - err: "either acme, serving_cert_file, insecure_addr or listen_socket_path must be configured", + err: "one of serving_cert_source, acme, serving_cert_file, insecure_addr or listen_socket_path must be configured", }, { name: "ACME ToS not accepted", diff --git a/support/oidc-discovery-provider/config_test.go b/support/oidc-discovery-provider/config_test.go index e4a4514a74..54f4799794 100644 --- a/support/oidc-discovery-provider/config_test.go +++ b/support/oidc-discovery-provider/config_test.go @@ -2,10 +2,12 @@ package main import ( "crypto/tls" + "net" "os" "path/filepath" "runtime" "testing" + "time" "github.com/spiffe/spire/pkg/common/tlspolicy" "github.com/spiffe/spire/test/spiretest" @@ -185,3 +187,167 @@ func TestApplyTLSPolicyWithInvalidServerTLSConfig(t *testing.T) { require.Contains(t, err.Error(), "invalid minTLSVersion") }) } + +func TestParseConfigServingCertSource(t *testing.T) { + serverAPI := "server_api {\n address = \"unix:///some/socket/path\"\n}" + workloadAPI := "workload_api {\n socket_path = \"/some/socket/path\"\n trust_domain = \"domain.test\"\n}" + if runtime.GOOS == "windows" { + serverAPI = "server_api {\n experimental {\n named_pipe_name = \"\\\\name\\\\for\\\\server\\\\api\"\n }\n}" + workloadAPI = "workload_api {\n experimental {\n named_pipe_name = \"\\\\name\\\\for\\\\workload\\\\api\"\n }\n trust_domain = \"domain.test\"\n}" + } + acme := "serving_cert_source \"acme\" {\n email = \"admin@domain.test\"\n tos_accepted = true\n}" + certFile := "serving_cert_source \"cert_file\" {\n cert_file_path = \"test.crt\"\n key_file_path = \"test.key\"\n}" + deprecatedCertFile := "serving_cert_file {\n cert_file_path = \"test.crt\"\n key_file_path = \"test.key\"\n}\n" + + // The local listener and the Workload API address are OS specific. + localListener, localListenerName := "listen_socket_path = \"/some/listen/path\"\n", "listen_socket_path" + workloadAPIWithOwnAddr := "serving_cert_source \"workload_api\" {\n socket_path = \"/other/socket/path\"\n}" + checkOwnAddr := func(t *testing.T, c *Config) { + require.Equal(t, "/other/socket/path", c.ServingCertSource.WorkloadAPI.SocketPath) + } + if runtime.GOOS == "windows" { + localListener, localListenerName = "experimental {\n listen_named_pipe_name = \"\\\\name\\\\for\\\\listener\"\n}\n", "listen_named_pipe_name" + workloadAPIWithOwnAddr = "serving_cert_source \"workload_api\" {\n experimental {\n named_pipe_name = \"\\\\other\\\\pipe\"\n }\n}" + checkOwnAddr = func(t *testing.T, c *Config) { + require.Equal(t, `\other\pipe`, c.ServingCertSource.WorkloadAPI.Experimental.NamedPipeName) + } + } + + for _, tt := range []struct { + name string + in string + err string + check func(t *testing.T, c *Config) + }{ + { + name: "acme source is aliased to the acme section", + in: acme + serverAPI, + check: func(t *testing.T, c *Config) { + require.Equal(t, &ACMEConfig{CacheDir: defaultCacheDir, Email: "admin@domain.test", ToSAccepted: true}, c.ServingCertSource.ACME) + require.Same(t, c.ServingCertSource.ACME, c.ACME) + }, + }, + { + name: "cert_file source is aliased to the serving_cert_file section", + in: "serving_cert_source \"cert_file\" {\n cert_file_path = \"test.crt\"\n key_file_path = \"test.key\"\n}" + serverAPI, + check: func(t *testing.T, c *Config) { + require.Same(t, c.ServingCertSource.CertFile, c.ServingCertFile) + require.Equal(t, defaultAddr, c.ServingCertFile.RawAddr) + require.Equal(t, time.Minute, c.ServingCertFile.FileSyncInterval) + }, + }, + { + name: "workload_api source with defaults", + in: "serving_cert_source \"workload_api\" {}" + workloadAPI, + check: func(t *testing.T, c *Config) { + require.Equal(t, &net.TCPAddr{Port: 443}, c.ServingCertSource.WorkloadAPI.Addr) + require.Equal(t, c.WorkloadAPI.SocketPath, c.ServingCertSource.WorkloadAPI.SocketPath) + require.Equal(t, c.WorkloadAPI.Experimental, c.ServingCertSource.WorkloadAPI.Experimental) + require.Nil(t, c.ACME) + require.Nil(t, c.ServingCertFile) + }, + }, + { + name: "workload_api source with addr", + in: "serving_cert_source \"workload_api\" {\n addr = \"127.0.0.1:9090\"\n}" + workloadAPI, + check: func(t *testing.T, c *Config) { + require.Equal(t, &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 9090}, c.ServingCertSource.WorkloadAPI.Addr) + }, + }, + { + name: "workload_api source without a Workload API address", + in: "serving_cert_source \"workload_api\" {}" + serverAPI, + err: `must be configured in the serving_cert_source "workload_api" configuration section`, + }, + { + name: "workload_api source keeps its own Workload API address", + in: workloadAPIWithOwnAddr + workloadAPI, + check: checkOwnAddr, + }, + { + name: "workload_api source with insecure_addr", + in: "insecure_addr = \":8080\"\nserving_cert_source \"workload_api\" {}" + workloadAPI, + err: `serving_cert_source "workload_api" is mutually exclusive with insecure_addr`, + }, + { + name: "workload_api source with a local listener", + in: localListener + "serving_cert_source \"workload_api\" {}" + workloadAPI, + err: `serving_cert_source "workload_api" is mutually exclusive with insecure_addr and ` + localListenerName, + }, + { + name: "acme source with insecure_addr", + in: "insecure_addr = \":8080\"\n" + acme + serverAPI, + err: `insecure_addr and the serving_cert_source "acme" section are mutually exclusive`, + }, + { + name: "acme source errors name the serving_cert_source section", + in: "serving_cert_source \"acme\" {\n tos_accepted = true\n}" + serverAPI, + err: `email must be configured in the serving_cert_source "acme" configuration section`, + }, + { + name: "cert_file source without cert_file_path", + in: "serving_cert_source \"cert_file\" {\n key_file_path = \"test.key\"\n}" + serverAPI, + err: `cert_file_path must be configured in the serving_cert_source "cert_file" configuration section`, + }, + { + name: "cert_file source without key_file_path", + in: "serving_cert_source \"cert_file\" {\n cert_file_path = \"test.crt\"\n}" + serverAPI, + err: `key_file_path must be configured in the serving_cert_source "cert_file" configuration section`, + }, + { + name: "cert_file source with a local listener", + in: localListener + certFile + serverAPI, + err: `serving_cert_source "cert_file" and ` + localListenerName + " are mutually exclusive", + }, + { + name: "unknown source", + in: "serving_cert_source \"unknown\" {}" + serverAPI, + err: `unknown serving_cert_source "unknown": must be one of "acme", "cert_file", or "workload_api"`, + }, + { + name: "unknown source next to a known one", + in: "serving_cert_source \"workload_api\" {}\nserving_cert_source \"unknown\" {}" + workloadAPI, + err: "only one serving_cert_source section can be configured", + }, + { + name: "missing label", + in: "serving_cert_source {}" + serverAPI, + err: "serving_cert_source must have exactly one label", + }, + { + name: "extra label", + in: "serving_cert_source \"workload_api\" \"extra\" {}" + workloadAPI, + err: "serving_cert_source must have exactly one label", + }, + { + name: "multiple sources", + in: acme + "serving_cert_source \"workload_api\" {}" + workloadAPI, + err: "only one serving_cert_source section can be configured", + }, + { + name: "duplicate sources", + in: "serving_cert_source \"workload_api\" {\n addr = \":1\"\n}\nserving_cert_source \"workload_api\" {\n addr = \":2\"\n}" + workloadAPI, + err: "only one serving_cert_source section can be configured", + }, + { + name: "mixed with the deprecated acme section", + in: "acme {\n email = \"admin@domain.test\"\n tos_accepted = true\n}\n" + acme + serverAPI, + err: "the acme and serving_cert_file sections cannot be used together with the serving_cert_source section", + }, + { + name: "mixed with the deprecated serving_cert_file section", + in: deprecatedCertFile + acme + serverAPI, + err: "the acme and serving_cert_file sections cannot be used together with the serving_cert_source section", + }, + } { + t.Run(tt.name, func(t *testing.T) { + c, err := ParseConfig("domains = [\"domain.test\"]\n" + tt.in) + if tt.err != "" { + require.ErrorContains(t, err, tt.err) + return + } + require.NoError(t, err) + tt.check(t, c) + }) + } +} diff --git a/support/oidc-discovery-provider/config_windows_test.go b/support/oidc-discovery-provider/config_windows_test.go index 211936445d..f4b3f914c4 100644 --- a/support/oidc-discovery-provider/config_windows_test.go +++ b/support/oidc-discovery-provider/config_windows_test.go @@ -68,7 +68,7 @@ func parseConfigCasesOS() []parseConfigCase { } } `, - err: "either acme, serving_cert_file, insecure_addr or listen_named_pipe_name must be configured", + err: "one of serving_cert_source, acme, serving_cert_file, insecure_addr or listen_named_pipe_name must be configured", }, { name: "ACME ToS not accepted", diff --git a/support/oidc-discovery-provider/healthchecks_handler.go b/support/oidc-discovery-provider/healthchecks_handler.go index 4fd0b5d031..24de816c87 100644 --- a/support/oidc-discovery-provider/healthchecks_handler.go +++ b/support/oidc-discovery-provider/healthchecks_handler.go @@ -2,6 +2,7 @@ package main import ( "net/http" + "sync/atomic" "time" ) @@ -15,6 +16,7 @@ type HealthChecksHandler struct { healthChecks HealthChecksConfig jwkThreshold time.Duration initTime time.Time + listening atomic.Bool http.Handler } @@ -35,6 +37,12 @@ func NewHealthChecksHandler(source JWKSSource, config *Config) *HealthChecksHand return h } +// SetListening marks the provider listener as up. Until then the ready check +// reports not ready, e.g. while waiting for the first X509-SVID. +func (h *HealthChecksHandler) SetListening() { + h.listening.Store(true) +} + // jwkThreshold determines the duration from the last successful poll before the server is considered unhealthy func jwkThreshold(config *Config) time.Duration { var duration time.Duration @@ -52,7 +60,8 @@ func jwkThreshold(config *Config) time.Duration { return duration } -// readyCheck is a health check that returns 200 if the server can successfully fetch a jwt keyset +// readyCheck is a health check that returns 200 if the listener is up and the +// server can successfully fetch a jwt keyset func (h *HealthChecksHandler) readyCheck(w http.ResponseWriter, r *http.Request) { if r.Method != "GET" { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) @@ -62,7 +71,7 @@ func (h *HealthChecksHandler) readyCheck(w http.ResponseWriter, r *http.Request) statusCode := http.StatusOK lastPoll := h.source.LastSuccessfulPoll() elapsed := time.Since(lastPoll) - isReady := !lastPoll.IsZero() && elapsed < h.jwkThreshold + isReady := h.listening.Load() && !lastPoll.IsZero() && elapsed < h.jwkThreshold if !isReady { statusCode = http.StatusInternalServerError diff --git a/support/oidc-discovery-provider/healthchecks_handler_test.go b/support/oidc-discovery-provider/healthchecks_handler_test.go index 233aa57195..cb8b2a86aa 100644 --- a/support/oidc-discovery-provider/healthchecks_handler_test.go +++ b/support/oidc-discovery-provider/healthchecks_handler_test.go @@ -23,7 +23,9 @@ func TestHealthCheckHandler(t *testing.T) { jwks *jose.JSONWebKeySet modTime time.Time pollTime time.Time - code int + // notListening leaves the provider listener marked as not up. + notListening bool + code int }{ { name: "Check Live State with no Keyset and valid threshold", @@ -102,6 +104,30 @@ func TestHealthCheckHandler(t *testing.T) { code: http.StatusInternalServerError, jwks: nil, }, + { + name: "Check Ready State with Keyset while the listener is not up", + method: "GET", + path: "/ready", + code: http.StatusInternalServerError, + jwks: &jose.JSONWebKeySet{ + Keys: []jose.JSONWebKey{ + { + Key: ec256Pubkey, + KeyID: "KEYID", + Algorithm: "ES256", + }, + }, + }, + pollTime: time.Now(), + notListening: true, + }, + { + name: "Check Live State without Keyset while the listener is not up", + method: "GET", + path: "/live", + code: http.StatusOK, + notListening: true, + }, } for _, testCase := range testCases { @@ -116,6 +142,9 @@ func TestHealthCheckHandler(t *testing.T) { ServerAPI: &ServerAPIConfig{}, HealthChecks: &HealthChecksConfig{BindPort: 8008, ReadyPath: "/ready", LivePath: "/live"}} h := NewHealthChecksHandler(source, &c) + if !testCase.notListening { + h.SetListening() + } h.ServeHTTP(w, r) t.Logf("HEADERS: %q", w.Header()) diff --git a/support/oidc-discovery-provider/main.go b/support/oidc-discovery-provider/main.go index 12d4ea5129..cc0fbf6e27 100644 --- a/support/oidc-discovery-provider/main.go +++ b/support/oidc-discovery-provider/main.go @@ -15,6 +15,8 @@ import ( "time" "github.com/sirupsen/logrus" + "github.com/spiffe/go-spiffe/v2/spiffetls/tlsconfig" + "github.com/spiffe/go-spiffe/v2/workloadapi" "golang.org/x/crypto/acme" "golang.org/x/crypto/acme/autocert" @@ -22,6 +24,7 @@ import ( spirelog "github.com/spiffe/spire/pkg/common/log" "github.com/spiffe/spire/pkg/common/telemetry" "github.com/spiffe/spire/pkg/common/tlspolicy" + "github.com/spiffe/spire/pkg/common/util" "github.com/spiffe/spire/pkg/common/version" ) @@ -80,6 +83,17 @@ func run(configPath string, expandEnv bool) error { log.Warn("log_file_rotation is configured with max_size_mb = 0 and the provider has no way to trigger a rotation, so the log file will not be rotated") } + deprecatedConfigFields := logrus.Fields{ + telemetry.Alert: true, + telemetry.AlertType: telemetry.DeprecatedConfigAlertType, + } + if config.ServingCertSource == nil && config.ACME != nil { + log.WithFields(deprecatedConfigFields).Warn(`The acme section is deprecated and will be removed in a future release; use serving_cert_source "acme" instead`) + } + if config.ServingCertSource == nil && config.ServingCertFile != nil { + log.WithFields(deprecatedConfigFields).Warn(`The serving_cert_file section is deprecated and will be removed in a future release; use serving_cert_source "cert_file" instead`) + } + if config.AllowInsecureScheme { log.Warn("allow_insecure_scheme is enabled. JWKS keys will be served over HTTP. Only enable this when the network path to the provider is trusted end-to-end (for example, when TLS is terminated at a trusted reverse proxy or load balancer on a private network)") } @@ -131,8 +145,31 @@ func run(configPath string, expandEnv bool) error { handler = logHandler(log, handler) } + // The health checks are served before the listener is built so that /live + // and /ready answer while the provider waits for its serving certificate + // (e.g. for the first X509-SVID from the Workload API). /ready reports not + // ready until the listener is up. + var healthChecks *HealthChecksHandler + if config.HealthChecks != nil { + healthChecks = NewHealthChecksHandler(source, config) + go func() { + server := &http.Server{ + Addr: fmt.Sprintf(":%d", config.HealthChecks.BindPort), + Handler: healthChecks, + ReadHeaderTimeout: 10 * time.Second, + } + log.Error(server.ListenAndServe()) + }() + } + listener, err := buildNetListener(ctx, config, log, tlsPolicy) if err != nil { + if ctx.Err() != nil { + // Shutdown was requested while waiting for the listener (e.g. for + // the first X509-SVID from the Workload API); not a failure. + log.Info("Shutdown requested before the listener was ready") + return nil + } return err } @@ -141,15 +178,8 @@ func run(configPath string, expandEnv bool) error { log.Error(err) }() - if config.HealthChecks != nil { - go func() { - server := &http.Server{ - Addr: fmt.Sprintf(":%d", config.HealthChecks.BindPort), - Handler: NewHealthChecksHandler(source, config), - ReadHeaderTimeout: 10 * time.Second, - } - log.Error(server.ListenAndServe()) - }() + if healthChecks != nil { + healthChecks.SetListening() } server := &http.Server{ @@ -194,6 +224,12 @@ func buildNetListener(ctx context.Context, config *Config, log *spirelog.Logger, telemetry.CertFilePath: config.ServingCertFile.CertFilePath, telemetry.Address: config.ServingCertFile.KeyFilePath, }).Info("Serving HTTPS using certificate loaded from disk") + case config.servingCertWorkloadAPI() != nil: + listener, err = newWorkloadAPIListener(ctx, log, config, tlsPolicy) + if err != nil { + return nil, err + } + log.WithField(telemetry.Address, listener.Addr().String()).Info("Serving HTTPS using an X509-SVID obtained from the Workload API") default: listener, err = newACMEListener(log, config, tlsPolicy) if err != nil { @@ -261,6 +297,40 @@ func newListenerWithServingCert(ctx context.Context, logger logrus.FieldLogger, return &tlsListener{TCPListener: tcpListener, conf: tlsConfig}, nil } +// newWorkloadAPIListener returns a TLS listener serving the X509-SVID obtained +// from the Workload API. It blocks until the first X509-SVID is received or +// the context is canceled; the SVID is then kept up to date as it is rotated. +func newWorkloadAPIListener(ctx context.Context, logger logrus.FieldLogger, config *Config, tlsPolicy tlspolicy.Policy) (net.Listener, error) { + workloadAPIAddr, err := config.getServingCertWorkloadAPIAddr() + if err != nil { + return nil, err + } + clientOption, err := util.GetWorkloadAPIClientOption(workloadAPIAddr) + if err != nil { + return nil, err + } + + logger.WithField(telemetry.Address, workloadAPIAddr.String()).Info("Waiting for an X509-SVID from the Workload API") + source, err := workloadapi.NewX509Source(ctx, workloadapi.WithClientOptions(clientOption, workloadapi.WithLogger(workloadAPILogger{log: logger}))) + if err != nil { + return nil, fmt.Errorf("failed to obtain an X509-SVID from the Workload API: %w", err) + } + + tlsConfig := tlsconfig.TLSServerConfig(source) + if err := applyTLSPolicy(tlsConfig, tlsPolicy); err != nil { + _ = source.Close() + return nil, err + } + + tcpListener, err := net.ListenTCP("tcp", config.servingCertWorkloadAPI().Addr) + if err != nil { + _ = source.Close() + return nil, fmt.Errorf("failed to create listener using X509-SVID from the Workload API: %w", err) + } + + return &workloadAPIListener{tlsListener: &tlsListener{TCPListener: tcpListener, conf: tlsConfig}, source: source}, nil +} + func newACMEListener(logger logrus.FieldLogger, config *Config, tlsPolicy tlspolicy.Policy) (net.Listener, error) { var cache autocert.Cache if config.ACME.CacheDir != "" { @@ -329,3 +399,41 @@ func (ln *tlsListener) Accept() (net.Conn, error) { _ = conn.SetKeepAlivePeriod(3 * time.Minute) return tls.Server(conn, ln.conf), nil } + +// workloadAPILogger adapts the provider logger to the go-spiffe logger so that +// Workload API errors (e.g. an unreachable socket) are reported while the +// provider waits for an X509-SVID instead of failing silently. +type workloadAPILogger struct { + log logrus.FieldLogger +} + +func (l workloadAPILogger) Debugf(format string, args ...any) { + l.log.Debugf(format, args...) +} + +func (l workloadAPILogger) Infof(format string, args ...any) { + l.log.Infof(format, args...) +} + +func (l workloadAPILogger) Warnf(format string, args ...any) { + l.log.Warnf(format, args...) +} + +func (l workloadAPILogger) Errorf(format string, args ...any) { + l.log.Errorf(format, args...) +} + +// workloadAPIListener is a tlsListener serving an X509-SVID obtained from the +// Workload API. Closing the listener also closes the Workload API source. +type workloadAPIListener struct { + *tlsListener + source *workloadapi.X509Source +} + +func (ln *workloadAPIListener) Close() error { + err := ln.tlsListener.Close() + if sourceErr := ln.source.Close(); err == nil { + err = sourceErr + } + return err +} diff --git a/support/oidc-discovery-provider/main_posix.go b/support/oidc-discovery-provider/main_posix.go index 4e6e75cee8..e273713927 100644 --- a/support/oidc-discovery-provider/main_posix.go +++ b/support/oidc-discovery-provider/main_posix.go @@ -4,6 +4,7 @@ package main import ( "errors" + "fmt" "net" "os" "strings" @@ -15,25 +16,32 @@ func (c *Config) getWorkloadAPIAddr() (net.Addr, error) { return util.GetUnixAddrWithAbsPath(c.WorkloadAPI.SocketPath) } +func (c *Config) getServingCertWorkloadAPIAddr() (net.Addr, error) { + return util.GetUnixAddrWithAbsPath(c.ServingCertSource.WorkloadAPI.SocketPath) +} + func (c *Config) getServerAPITargetName() string { return c.ServerAPI.Address } // validateOS performs os specific validations of the configuration func (c *Config) validateOS() (err error) { + servingCertWorkloadAPI := c.servingCertWorkloadAPI() switch { - case c.ACME == nil && c.ListenSocketPath == "" && c.ServingCertFile == nil && c.InsecureAddr == "": - return errors.New("either acme, serving_cert_file, insecure_addr or listen_socket_path must be configured") + case c.ACME == nil && c.ListenSocketPath == "" && c.ServingCertFile == nil && servingCertWorkloadAPI == nil && c.InsecureAddr == "": + return errors.New("one of serving_cert_source, acme, serving_cert_file, insecure_addr or listen_socket_path must be configured") + case servingCertWorkloadAPI != nil && (c.InsecureAddr != "" || c.ListenSocketPath != ""): + return errors.New(`serving_cert_source "workload_api" is mutually exclusive with insecure_addr and listen_socket_path`) case c.ACME != nil && c.ServingCertFile != nil: return errors.New("acme and serving_cert_file are mutually exclusive") case c.ACME != nil && c.ListenSocketPath != "": - return errors.New("listen_socket_path and the acme section are mutually exclusive") + return fmt.Errorf("listen_socket_path and the %s section are mutually exclusive", c.acmeSectionName()) case c.ServingCertFile != nil && c.InsecureAddr != "": - return errors.New("serving_cert_file and insecure_addr are mutually exclusive") + return fmt.Errorf("%s and insecure_addr are mutually exclusive", c.certFileSectionName()) case c.ServingCertFile != nil && c.ListenSocketPath != "": - return errors.New("serving_cert_file and listen_socket_path are mutually exclusive") + return fmt.Errorf("%s and listen_socket_path are mutually exclusive", c.certFileSectionName()) case c.ACME != nil && c.InsecureAddr != "": - return errors.New("acme and insecure_addr are mutually exclusive") + return fmt.Errorf("%s and insecure_addr are mutually exclusive", c.acmeSectionName()) case c.InsecureAddr != "" && c.ListenSocketPath != "": return errors.New("insecure_addr and listen_socket_path are mutually exclusive") } @@ -53,6 +61,14 @@ func (c *Config) validateOS() (err error) { } } + if servingCertWorkloadAPI != nil && servingCertWorkloadAPI.SocketPath == "" { + if c.WorkloadAPI == nil { + return errors.New(`socket_path must be configured in the serving_cert_source "workload_api" configuration section`) + } + // Default to the Workload API used as the JWKS source. + servingCertWorkloadAPI.SocketPath = c.WorkloadAPI.SocketPath + } + return nil } diff --git a/support/oidc-discovery-provider/main_posix_test.go b/support/oidc-discovery-provider/main_posix_test.go new file mode 100644 index 0000000000..d6d7a60471 --- /dev/null +++ b/support/oidc-discovery-provider/main_posix_test.go @@ -0,0 +1,103 @@ +//go:build !windows + +package main + +import ( + "crypto/tls" + "crypto/x509" + "net" + "testing" + + "github.com/sirupsen/logrus/hooks/test" + "github.com/spiffe/go-spiffe/v2/proto/spiffe/workload" + "github.com/spiffe/go-spiffe/v2/spiffeid" + "github.com/spiffe/go-spiffe/v2/spiffetls/tlsconfig" + "github.com/spiffe/spire/pkg/common/tlspolicy" + "github.com/spiffe/spire/pkg/common/x509util" + "github.com/spiffe/spire/test/fakes/fakeworkloadapi" + "github.com/spiffe/spire/test/testca" + "github.com/stretchr/testify/require" +) + +func TestNewWorkloadAPIListener(t *testing.T) { + td := spiffeid.RequireTrustDomainFromString("domain.test") + ca := testca.New(t, td) + svid := ca.CreateX509SVID(spiffeid.RequireFromPath(td, "/oidc-discovery-provider")) + keyDER, err := x509.MarshalPKCS8PrivateKey(svid.PrivateKey) + require.NoError(t, err) + + api := fakeworkloadapi.New(t, &fakeworkloadapi.FakeRequest{ + Req: &workload.X509SVIDRequest{}, + Resp: &workload.X509SVIDResponse{Svids: []*workload.X509SVID{{ + SpiffeId: svid.ID.String(), + X509Svid: x509util.DERFromCertificates(svid.Certificates), + X509SvidKey: keyDER, + Bundle: x509util.DERFromCertificates(ca.X509Authorities()), + }}}, + }) + + tlsPolicy, err := tlspolicy.NewPolicy(false, &tlspolicy.TLSConfig{MinTLSVersion: "VersionTLS13"}, nil) + require.NoError(t, err) + + config := &Config{ServingCertSource: &ServingCertSourceConfig{WorkloadAPI: &ServingCertWorkloadAPIConfig{ + SocketPath: api.Addr().(*net.UnixAddr).Name, + Addr: &net.TCPAddr{IP: net.IPv4(127, 0, 0, 1)}, + }}} + + ctx := t.Context() + log, _ := test.NewNullLogger() + listener, err := newWorkloadAPIListener(ctx, log, config, tlsPolicy) + require.NoError(t, err) + t.Cleanup(func() { _ = listener.Close() }) + + // acceptAndHandshake accepts a single connection and completes the TLS + // handshake on it, reporting the result on the returned channel. + acceptAndHandshake := func() <-chan error { + handshakeErr := make(chan error, 1) + go func() { + conn, err := listener.Accept() + if err != nil { + handshakeErr <- err + return + } + defer conn.Close() + handshakeErr <- conn.(*tls.Conn).HandshakeContext(ctx) + }() + return handshakeErr + } + clientConfig := func() *tls.Config { + return tlsconfig.TLSClientConfig(ca.X509Bundle(), tlsconfig.AuthorizeID(svid.ID)) + } + + // The certificate is always taken from the X509Source, so it follows + // rotation, rather than loaded once. + conf := listener.(*workloadAPIListener).conf + require.NotNil(t, conf.GetCertificate) + require.Empty(t, conf.Certificates) + + // The TLS policy is applied to the listener: a client limited to TLS 1.2 + // is rejected. + require.Equal(t, uint16(tls.VersionTLS13), conf.MinVersion) + handshakeErr := acceptAndHandshake() + tls12Config := clientConfig() + tls12Config.MaxVersion = tls.VersionTLS12 + _, err = (&tls.Dialer{Config: tls12Config}).DialContext(ctx, "tcp", listener.Addr().String()) + require.Error(t, err) + require.Error(t, <-handshakeErr) + + // The client authenticates the served certificate as the X509-SVID issued + // by the trust domain, using its bundle. + handshakeErr = acceptAndHandshake() + conn, err := (&tls.Dialer{Config: clientConfig()}).DialContext(ctx, "tcp", listener.Addr().String()) + require.NoError(t, err) + defer conn.Close() + require.NoError(t, <-handshakeErr) + + state := conn.(*tls.Conn).ConnectionState() + require.Equal(t, svid.Certificates[0].Raw, state.PeerCertificates[0].Raw) + + // Closing the listener releases the Workload API source. + require.NoError(t, listener.Close()) + _, err = listener.(*workloadAPIListener).source.GetX509SVID() + require.EqualError(t, err, "x509source: source is closed") +} diff --git a/support/oidc-discovery-provider/main_windows.go b/support/oidc-discovery-provider/main_windows.go index 55d24ebdb6..3dd966981e 100644 --- a/support/oidc-discovery-provider/main_windows.go +++ b/support/oidc-discovery-provider/main_windows.go @@ -17,25 +17,32 @@ func (c *Config) getWorkloadAPIAddr() (net.Addr, error) { return namedpipe.AddrFromName(c.WorkloadAPI.Experimental.NamedPipeName), nil } +func (c *Config) getServingCertWorkloadAPIAddr() (net.Addr, error) { + return namedpipe.AddrFromName(c.ServingCertSource.WorkloadAPI.Experimental.NamedPipeName), nil +} + func (c *Config) getServerAPITargetName() string { return fmt.Sprintf(`\\.\%s`, filepath.Join("pipe", c.ServerAPI.Experimental.NamedPipeName)) } // validateOS performs os specific validations of the configuration func (c *Config) validateOS() (err error) { + servingCertWorkloadAPI := c.servingCertWorkloadAPI() switch { - case c.ACME == nil && c.Experimental.ListenNamedPipeName == "" && c.ServingCertFile == nil && c.InsecureAddr == "": - return errors.New("either acme, serving_cert_file, insecure_addr or listen_named_pipe_name must be configured") + case c.ACME == nil && c.Experimental.ListenNamedPipeName == "" && c.ServingCertFile == nil && servingCertWorkloadAPI == nil && c.InsecureAddr == "": + return errors.New("one of serving_cert_source, acme, serving_cert_file, insecure_addr or listen_named_pipe_name must be configured") + case servingCertWorkloadAPI != nil && (c.InsecureAddr != "" || c.Experimental.ListenNamedPipeName != ""): + return errors.New(`serving_cert_source "workload_api" is mutually exclusive with insecure_addr and listen_named_pipe_name`) case c.ACME != nil && c.ServingCertFile != nil: return errors.New("acme and serving_cert_file are mutually exclusive") case c.ACME != nil && c.Experimental.ListenNamedPipeName != "": - return errors.New("listen_named_pipe_name and the acme section are mutually exclusive") + return fmt.Errorf("listen_named_pipe_name and the %s section are mutually exclusive", c.acmeSectionName()) case c.ACME != nil && c.InsecureAddr != "": - return errors.New("acme and insecure_addr are mutually exclusive") + return fmt.Errorf("%s and insecure_addr are mutually exclusive", c.acmeSectionName()) case c.ServingCertFile != nil && c.InsecureAddr != "": - return errors.New("serving_cert_file and insecure_addr are mutually exclusive") + return fmt.Errorf("%s and insecure_addr are mutually exclusive", c.certFileSectionName()) case c.ServingCertFile != nil && c.Experimental.ListenNamedPipeName != "": - return errors.New("serving_cert_file and listen_named_pipe_name are mutually exclusive") + return fmt.Errorf("%s and listen_named_pipe_name are mutually exclusive", c.certFileSectionName()) case c.InsecureAddr != "" && c.Experimental.ListenNamedPipeName != "": return errors.New("insecure_addr and listen_named_pipe_name are mutually exclusive") } @@ -51,6 +58,14 @@ func (c *Config) validateOS() (err error) { } } + if servingCertWorkloadAPI != nil && servingCertWorkloadAPI.Experimental.NamedPipeName == "" { + if c.WorkloadAPI == nil { + return errors.New(`named_pipe_name must be configured in the serving_cert_source "workload_api" configuration section`) + } + // Default to the Workload API used as the JWKS source. + servingCertWorkloadAPI.Experimental.NamedPipeName = c.WorkloadAPI.Experimental.NamedPipeName + } + return nil }