Add serving_cert_source section to the OIDC Discovery Provider by KR-Ravindra · Pull Request #7285 · spiffe/spire

KR-Ravindra · 2026-09-13T06:51:23Z

Pull Request check list

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

Affected functionality

OIDC Discovery Provider: configuration of the certificate used to serve HTTPS.

Description of change

The provider can only obtain its serving certificate via ACME or from files on disk, so serving it with a SPIRE-issued X509-SVID requires a sidecar writing the SVID to disk (#5570).

This adds the serving_cert_source labeled section agreed on in #5570:

serving_cert_source "acme" { ... }
serving_cert_source "cert_file" { ... }
serving_cert_source "workload_api" { ... }

config.go: new section with exactly one source. "acme" and "cert_file" are aliased onto the existing ACME/ServingCertFile fields at parse time so the existing code paths are reused. Not combinable with the deprecated acme/serving_cert_file sections. Error messages name the section the user wrote.
main.go: newWorkloadAPIListener obtains the X509-SVID with workloadapi.NewX509Source (errors logged through the go-spiffe logger) and serves it through tlsconfig.TLSServerConfig, so the served certificate follows rotation; tls_config applies. The health checks server starts before the listener is built, with /ready not ready until the listener is up, and a shutdown request while waiting for the first SVID exits cleanly. Deprecation warnings for acme and serving_cert_file.
main_posix.go / main_windows.go: socket_path / named_pipe_name default to the workload_api JWKS section; mutually exclusive with insecure_addr and the local listener.
README: new section, DNS names and combination notes.

Tests: TestParseConfigServingCertSource, TestNewWorkloadAPIListener (fake Workload API, go-spiffe authenticated handshake, TLS 1.2 client rejected) and TestHealthCheckHandler readiness gating.

Which issue this PR fixes

fixes #5570

This change was prepared with an AI agent operated by KR-Ravindra, who reviewed and tested it.

Copilot

🟡 Changes recommended

Several operator-facing deprecation/logging and documentation/test issues should be addressed to match SPIRE conventions and avoid confusing behavior.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR extends the OIDC Discovery Provider to support serving HTTPS with a SPIRE-issued X509-SVID fetched from the SPIFFE Workload API, via a new labeled serving_cert_source configuration block while preserving legacy acme / serving_cert_file behavior through aliasing.

Changes:

Add serving_cert_source (acme, cert_file, workload_api) parsing/validation and alias legacy config fields for backward compatibility.
Implement a Workload API-backed TLS listener that serves a rotating X509-SVID using workloadapi.NewX509Source + tlsconfig.TLSServerConfig.
Update documentation and add tests covering parsing/validation and the Workload API TLS listener behavior.

File summaries

File	Description
support/oidc-discovery-provider/README.md	Documents `serving_cert_source` and adds Workload API serving-cert example.
support/oidc-discovery-provider/main.go	Adds Workload API serving-cert listener path and emits deprecation warnings for legacy config sections.
support/oidc-discovery-provider/main_windows.go	Adds serving-cert Workload API address support and OS-specific validation/defaulting for named pipes.
support/oidc-discovery-provider/main_posix.go	Adds serving-cert Workload API address support and OS-specific validation/defaulting for Unix sockets.
support/oidc-discovery-provider/main_posix_test.go	Adds a TLS handshake test for the Workload API serving-cert listener on non-Windows platforms.
support/oidc-discovery-provider/config.go	Introduces `ServingCertSource` config structs, parsing/validation, and workload_api addr defaulting.
support/oidc-discovery-provider/config_test.go	Adds table-driven tests for `serving_cert_source` parsing, defaults, and validation errors.

Review details

Suppressed comments (1)

support/oidc-discovery-provider/README.md:90

The footnote describing required listener configuration omits insecure_addr, but the provider can start in insecure mode with only insecure_addr configured. The documentation should match the actual validation behavior.

[1]: One of `serving_cert_source`, `acme`, `serving_cert_file` or `listen_named_pipe_name` must be defined.

Files reviewed: 7/7 changed files
Comments generated: 5
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

 #### 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.


+	if config.ServingCertSource == nil && config.ACME != nil {
+		log.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.Warn(`The serving_cert_file section is deprecated and will be removed in a future release; use serving_cert_source "cert_file" instead`)
+	}


+	case c.ACME == nil && c.ListenSocketPath == "" && c.ServingCertFile == nil && servingCertWorkloadAPI == nil && c.InsecureAddr == "":
+		return errors.New("a serving_cert_source section, or either acme, serving_cert_file, insecure_addr or listen_socket_path must be configured")


+	listener, err := newWorkloadAPIListener(ctx, log, config, tlsPolicy)
+	require.NoError(t, err)
+	defer listener.Close()
+


+	case c.ACME == nil && c.Experimental.ListenNamedPipeName == "" && c.ServingCertFile == nil && servingCertWorkloadAPI == nil && c.InsecureAddr == "":
+		return errors.New("a serving_cert_source section, or either acme, serving_cert_file, insecure_addr or listen_named_pipe_name must be configured")


MarcosDY

looks great! I added a couple comments

MarcosDY · 2026-09-26T18:20:42Z

+	}
+
+	logger.WithField(telemetry.Address, workloadAPIAddr.String()).Info("Waiting for an X509-SVID from the Workload API")
+	source, err := workloadapi.NewX509Source(ctx, workloadapi.WithClientOptions(clientOption))


Can you add logs to workload api? go-spiffe provides a way to log errors from watcher, that logger is going to allow users to understand why source is failing (for example socket is not accessible) if not it is going to wait for ever without notification,
you can see how we are doing that one pkg/server/plugin/upstreamauthority/spire/spire_server_client.go.

Another thing here, NewX509Source blocks until the first X509-SVID arrives, and there's no timeout. The health checks server only starts after buildNetListener returns, so while we're waiting, /live and /ready don't respond at all.

In Kubernetes, if the registration entry is missing (or slow to show up), the liveness probe fails and the pod keeps restarting (and when we start server+provider on the same pod this can be an issue).
The only log line is "Waiting for an X509-SVID from the Workload API", so it's hard to tell what's wrong. Also, if the pod gets a SIGTERM during the wait, the provider exits with a "context canceled" error instead of shutting down cleanly.

Could we start the health server before building the listener, with /ready returning false until the listener is up?

MarcosDY · 2026-10-03T16:25:47Z

+	state := conn.(*tls.Conn).ConnectionState()
+	require.Equal(t, svid.Certificates[0].Raw, state.PeerCertificates[0].Raw)
+	// The TLS policy is applied to the listener.
+	require.Equal(t, uint16(tls.VersionTLS13), state.Version)


This check passes even if the TLS policy is never applied. A default Go client and server already pick TLS 1.3, so state.Version is 1.3 either way.

The simplest fix is to check the listener's config directly, since the test already reaches into workloadAPIListener for the source:

require.Equal(t, uint16(tls.VersionTLS13), listener.(*workloadAPIListener).conf.MinVersion)

Without the policy, MinVersion would be TLS 1.2 (applyTLSPolicy sets that as the base), so this would catch it.

Another option is to also dial with a client limited to TLS 1.2 (MaxVersion: tls.VersionTLS12) and expect the handshake to fail. That tests what users actually see.

MarcosDY · 2026-10-03T16:28:16Z


 [4]: SPIRE OIDC Discovery provider monitors and reloads the files provided in the `serving_cert_file` configuration at runtime.

+#### Serving Certificate Source Section


A few things that would help in this section:

DNS names: browsers and most HTTP clients check that the certificate's DNS names match the host they connect to. An X509-SVID only has DNS names if the registration entry sets them (-dns), so the entry needs DNS names that match domains. Without them, clients like the kube-apiserver fetching the JWKS will fail. Could we add a note about this next to the trust bundle note?

What it can't be combined with: serving_cert_source "workload_api" can't be used together with insecure_addr, listen_socket_path or listen_named_pipe_name. The provider fails to start in that case, so it's worth one sentence.

More than one SVID: if the workload gets more than one SVID, the first one is served. A short note would avoid surprises.

MarcosDY · 2026-10-03T16:29:30Z

 | `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 row still says "(disk certificate and ACME modes)", but tls_config also applies to the Workload API source. Maybe "(disk certificate, ACME and Workload API modes)".

MarcosDY · 2026-10-03T16:32:10Z

+		// 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


When the new syntax is used, these errors still point at the old section names. For example, serving_cert_source "cert_file" {} without cert_file_path fails with:

cert_file_path must be configured in the serving_cert_file configuration section

serving_cert_source "acme" {} does the same and mentions "the acme configuration section". The user never wrote those sections, so it can be confusing, especially since the old ones are now deprecated.

A simple fix would be to keep the section name in a variable and use it in these messages:

acmeSection, certFileSection := "acme", "serving_cert_file" if c.ServingCertSource != nil { acmeSection, certFileSection = `serving_cert_source "acme"`, `serving_cert_source "cert_file"` }

MarcosDY · 2026-10-03T16:35:43Z

+	// 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 or ListenSocketPath is set.


"required unless InsecureAddr or ListenSocketPath is set" doesn't match the code, can you update it?

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>

KR-Ravindra · 2026-10-03T16:48:18Z

@MarcosDY done in 376b937, thanks for the review:

Workload API errors are logged through the go-spiffe logger (workloadapi.WithLogger), same pattern as the spire upstream authority client.
The health checks server starts before the listener is built; /ready returns 500 until the listener is up (SetListening), /live keeps its current semantics. A SIGTERM while waiting for the first SVID now exits cleanly instead of returning "context canceled".
The test checks conf.MinVersion and that a TLS 1.2-capped client is rejected.
Errors name the section the user wrote (serving_cert_source "acme" / "cert_file"), with test cases. The ServingCertSource comment and the tls_config row are fixed, and the README has the DNS names, combination and multiple-SVID notes.

Could you take another look?

This comment was drafted with AI assistance and checked against the code.

KR-Ravindra marked this pull request as ready for review September 13, 2026 08:25

Copilot AI lite review requested due to automatic review settings September 13, 2026 08:25

KR-Ravindra requested review from MarcosDY, amartinezfayo, evan2645, rturner3 and sorindumitru as code owners September 13, 2026 08:25

github-actions Bot assigned MarcosDY Sep 13, 2026

Copilot started reviewing on behalf of KR-Ravindra September 13, 2026 08:26 View session

Copilot AI reviewed Sep 13, 2026

View reviewed changes

KR-Ravindra force-pushed the feat/oidc-provider-serving-cert-source branch from 059f9ae to f9e6ce7 Compare September 20, 2026 21:42

MarcosDY reviewed Oct 3, 2026

View reviewed changes

MarcosDY assigned KR-Ravindra and unassigned MarcosDY Oct 3, 2026

KR-Ravindra force-pushed the feat/oidc-provider-serving-cert-source branch from f9e6ce7 to 376b937 Compare October 3, 2026 16:47

github-actions Bot assigned MarcosDY and unassigned KR-Ravindra Oct 3, 2026

Navigation Menu

Add serving_cert_source section to the OIDC Discovery Provider - #7285

Add serving_cert_source section to the OIDC Discovery Provider#7285
KR-Ravindra wants to merge 1 commit into
spiffe:mainfrom
KR-Ravindra:feat/oidc-provider-serving-cert-source

KR-Ravindra commented Sep 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

MarcosDY left a comment

Uh oh!

MarcosDY Sep 26, 2026

Uh oh!

MarcosDY Oct 3, 2026

Uh oh!

MarcosDY Oct 3, 2026

Uh oh!

MarcosDY Oct 3, 2026

Uh oh!

MarcosDY Oct 3, 2026

Uh oh!

MarcosDY Oct 3, 2026

Uh oh!

MarcosDY Oct 3, 2026

Uh oh!

KR-Ravindra commented Oct 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		case c.ACME == nil && c.ListenSocketPath == "" && c.ServingCertFile == nil && servingCertWorkloadAPI == nil && c.InsecureAddr == "":
		return errors.New("a serving_cert_source section, or either acme, serving_cert_file, insecure_addr or listen_socket_path must be configured")

		case c.ACME == nil && c.Experimental.ListenNamedPipeName == "" && c.ServingCertFile == nil && servingCertWorkloadAPI == nil && c.InsecureAddr == "":
		return errors.New("a serving_cert_source section, or either acme, serving_cert_file, insecure_addr or listen_named_pipe_name must be configured")


		[4]: SPIRE OIDC Discovery provider monitors and reloads the files provided in the `serving_cert_file` configuration at runtime.

		#### Serving Certificate Source Section

Conversation

KR-Ravindra commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

MarcosDY left a comment

Choose a reason for hiding this comment

Uh oh!

MarcosDY Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

MarcosDY Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

MarcosDY Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

MarcosDY Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

MarcosDY Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

MarcosDY Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

MarcosDY Oct 3, 2026

Choose a reason for hiding this comment

Uh oh!

KR-Ravindra commented Oct 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KR-Ravindra commented Sep 13, 2026 •

edited

Loading