Adding option to enable Back End HTTPS for Prow Ingress by NiJuFirenzia · Pull Request #573 · kubernetes-sigs/prow

NiJuFirenzia · 2025-12-10T19:16:14Z

Prow currently requires SSL connections to be terminated before connecting to the deck and hook pods. This does not enable users to run prow with backend https enabled on their ingress. This PR adds the option to enable backend https on ingresses by passing in specific flags to the hook and deck pods. The argument flags will look like as below:

hook:

- --enable-ssl=true
- --server-cert-file=/etc/prow/tls.crt
- --server-key-file=/etc/prow/tls.key

deck:

- --enable-ssl=true
- --server-cert-file=/etc/prow/tls.crt
- --server-key-file=/etc/prow/tls.key
- --client-cert-file=etc/prow/deck-client.crt

In the example above, a secret containing the cert file and key file was mounted to the deck and hook pods. Additionally this has been tested to continue working as is with ingress backend to be set to just HTTP if the flags are not passed in.

netlify · 2025-12-10T19:16:21Z

✅ Deploy Preview for k8s-prow ready!

Name	Link
🔨 Latest commit	`814fb94`
🔍 Latest deploy log	https://app.netlify.com/projects/k8s-prow/deploys/69a1b17b61d15300080e8e34
😎 Deploy Preview	https://deploy-preview-573--k8s-prow.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

linux-foundation-easycla · 2025-12-10T19:16:22Z

The committers listed above are authorized under a signed CLA.

✅ login: NiJuFirenzia / name: Nikhil Jiju (2ad6fa1, 814fb94)

k8s-ci-robot · 2025-12-10T19:16:23Z

Welcome @NiJuFirenzia!

It looks like this is your first PR to kubernetes-sigs/prow 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/prow has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-12-10T19:16:27Z

Hi @NiJuFirenzia. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

NiJuFirenzia · 2025-12-10T19:21:29Z

This PR addresses Issue #328

matthyx · 2025-12-10T20:21:11Z

@NiJuFirenzia thanks for your PR, can you check the CLA and sign it?

matthyx · 2025-12-10T21:38:35Z

/ok-to-test

petr-muller · 2025-12-17T15:27:46Z

+	enableSSL bool
+	certFile  string
+	keyFile   string


let's group this into a small struct that we can embed at both places, we can share validation etc. similarly to how the ThingOptions bits above

petr-muller · 2025-12-17T15:29:38Z

+		httpServer := &http.Server{
+			Addr:    ":" + strconv.Itoa(o.port),
+			Handler: hookMux,
+		}


this seems identical in both branches, so can be extracted before the condition

petr-muller · 2025-12-17T16:12:21Z

 // cacheLife is the time that we keep a pluginhelp.Help struct before considering it stale.
 // We consider help valid for a minute to prevent excessive calls to hook.
 const cacheLife = time.Minute
+const tlsEnabledScehma = "https"


Suggested change

const tlsEnabledScehma = "https"

const tlsEnabledSchema = "https"

petr-muller · 2025-12-17T16:16:15Z

+			transport.TLSClientConfig = &tls.Config{
+				RootCAs: caCertPool,
+			}
+			http.DefaultTransport = transport


helpAgent should get a custom client member with this transport instead of modifying the global default transport (this would affect anything else in deck that is a http client and relies on the default)

petr-muller · 2025-12-17T16:29:03Z

+		caCert, err := os.ReadFile(ha.cert)
+		if err != nil {
+			return nil, fmt.Errorf("error decoding cert file: %w", err)
+		}
+		caCertPool := x509.NewCertPool()
+		caCertPool.AppendCertsFromPEM(caCert)


I am not entirely sure I like this coupling. This essentially reuses Deck's server cert as a CA cert to establish trust with Hook's server cert for this behavior where Deck is a client to hook's pluginhelp endpoint? That's a bit hacky and suprising.

That would require having to add another option flag to take in a client cert right? I had been trying to limit the options that we would need

k8s-ci-robot · 2026-01-15T13:31:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: NiJuFirenzia
Once this PR has been reviewed and has the lgtm label, please ask for approval from petr-muller. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Nikhil Jiju <Nikhil.Jiju@fmr.com>

NiJuFirenzia · 2026-01-16T14:32:44Z

All change requests have been addressed and this PR is ready for a re-review

petr-muller

There's a flag name mismatch between error message and actual names. I have pointed out some opportunities to reduce stuttering (sslEnablement.EnableSSL and verbosity sslEnablement vs ssl) but treat them as non-blocking nits.

petr-muller · 2026-01-24T18:17:26Z

+func (o *SSLEnablementOptions) Validate(_ bool) error {
+	if o.EnableSSL {
+		if o.ServerCertFile == "" {
+			return errors.New("flag --enable-ssl was set to true but required flag --cert-file was not set")


Suggested change

return errors.New("flag --enable-ssl was set to true but required flag --cert-file was not set")

return errors.New("flag --enable-ssl was set to true but required flag --server-cert-file was not set")

actual flags are different

petr-muller · 2026-01-24T18:18:16Z

+			return errors.New("flag --enable-ssl was set to true but required flag --cert-file was not set")
+		}
+		if o.ServerKeyFile == "" {
+			return errors.New("flag --enable-ssl was set to true but required flag --key-file was not set")


Suggested change

return errors.New("flag --enable-ssl was set to true but required flag --key-file was not set")

return errors.New("flag --enable-ssl was set to true but required flag --server-key-file was not set")

petr-muller · 2026-01-24T18:21:54Z

+}
+
+func (o *SSLEnablementOptions) Validate(_ bool) error {
+	if o.EnableSSL {


We should either check also the opposite case (enabled is false but cert was passed, possibly confusing the admin they are using SSL when they are not), or make this an implied field from the presence of at least one file flags (if at least one is set, assume the user wants to have it enabled and validate both are set).

petr-muller · 2026-01-24T18:23:24Z

 	bugzilla               prowflagutil.BugzillaOptions
 	instrumentationOptions prowflagutil.InstrumentationOptions
 	jira                   prowflagutil.JiraOptions
+	sslEnablement          prowflagutil.SSLEnablementOptions


Suggested change

sslEnablement prowflagutil.SSLEnablementOptions

ssl prowflagutil.SSLEnablementOptions

petr-muller · 2026-01-24T18:25:34Z

+// HTTPS backend. If enableSSL is true, both certFile and keyFile must be set
+// to the location of the cert and key files respectively.
+
+type SSLEnablementOptions struct {


tbh I'd call this just SSLOptions or SSLServerOptions and the package flagutil/ssl

petr-muller · 2026-01-24T18:27:41Z

+	EnableSSL      bool
+	ServerCertFile string
+	ServerKeyFile  string


Suggested change

EnableSSL bool

ServerCertFile string

ServerKeyFile string

Enabled bool

CertFile string

KeyFile string

the instance of this struct will typically called ssl or something so meaning is typically provided by that context

I agree with the name changes. I'll have updates to this PR out next week

Signed-off-by: Nikhil Jiju <Nikhil.Jiju@fmr.com>

ivankatliarchuk

I'm not too sure. Maybe missing something. What is definately missing is a documentation, like operator docs how to, then it could be easier to understand the intent.

Misleading flag name suggests mTLS was intended but not delivered, aka --client-cert-file name -> it's a CA cert, not a client cert from first look.

I'll try to explain

CA certificate (what the code actually uses) When deck calls hook over HTTPS, it needs to verify that hook's server certificate is trustworthy. If hook uses a self-signed or internal CA cert, the OS trust store won't know about it. So you load that CA cert into a x509.CertPool and put it in RootCAs:

  caCertPool.AppendCertsFromPEM(caCert)
  client = &http.Client{
      Transport: &http.Transport{
          TLSClientConfig: &tls.Config{
              RootCAs: caCertPool,  // "trust this CA when verifying the server"
          },
      },
  }

This is one-way TLS: deck verifies hook's identity, but hook doesn't verify deck's identity.

Client certificate (what the name implies)

A client cert is deck's own certificate that it presents to hook during the handshake, so hook can verify deck's identity. That's mutual TLS (mTLS). It would look like:

  clientCert, _ := tls.LoadX509KeyPair(certFile, keyFile)
  TLSClientConfig: &tls.Config{
      Certificates: []tls.Certificate{clientCert},  // "here's who I am"
      RootCAs: caCertPool,
  }

The problem

An operator reading --client-cert-file will naturally think "this is the cert deck presents to authenticate itself to hook." But the code uses it purely as a CA cert to trust hook's server cert. These are completely different files with different formats and different roles.

A better name would be --hook-ca-cert-file or --hook-tls-ca-file, which makes it clear: "the CA cert used to verify hook's TLS certificate."

Another problem/concert from security perspective

The solution seems like half implemented.

One-way TLS only (deck verifies hook, hook doesn't verify deck)
No cert rotation -> not too clear, missing documentation describing that

ivankatliarchuk · 2026-03-26T07:29:29Z

+	}
+
+	if o.ssl.CertFile != "" && o.clientCertFile == "" {
+		return errors.New("flag --server-cert-file and --server-key-file was set to true but required flag --client-cert-file was not set")


"flag --server-cert-file and --server-key-file was set to true" — these are string flags, not booleans. Should be "were set" (plural) and drop "to true"

ivankatliarchuk · 2026-03-26T07:31:25Z

 type helpAgent struct {
-	path string
+	path   string
+	cert   string


cert field is a dead code. The field is set in newHelpAgent but never read after construction — the TLS client is fully initialized at construction time

ivankatliarchuk · 2026-03-26T07:32:35Z

 // cacheLife is the time that we keep a pluginhelp.Help struct before considering it stale.
 // We consider help valid for a minute to prevent excessive calls to hook.
 const cacheLife = time.Minute
+const sslEnabledSchema = "https"


Suggested change

const sslEnabledSchema = "https"

const sslEnabledScheme = "https"

should be sslEnabledScheme ("scheme" not "schema")

ivankatliarchuk · 2026-03-26T07:39:54Z

+		}
+	}
+
+	if o.ssl.CertFile != "" && o.clientCertFile == "" {


I'm not sure about this requirement. Requiring --client-cert-file whenever --server-cert-file is set does not sound correct - these are independent. Deck can serve HTTPS to the ingress without needing to call hook over HTTPS. Maybe the check should only trigger if --hook-url starts with https://

ivankatliarchuk · 2026-03-26T07:44:11Z

+}
+
+func (o *SSLOptions) Validate(_ bool) error {
+	if o.CertFile != "" || o.KeyFile != "" {


Worth to check for path exist and where or not file is missing.

Both kubernetes_cluster_clients.goand k8s_client.go use os.Stat inside their Validate methods to check kubeconfig paths.

Probably something like

if _, err := os.Stat(o.CertFile); err != nil { return fmt.Errorf("--server-cert-file: %w", err) } if _, err := os.Stat(o.KeyFile); err != nil { return fmt.Errorf("--server-key-file: %w", err) }

The benefit is that you get a clear error at startup rather than a cryptic TLS failure when ListenAndServeTLS is called.

ivankatliarchuk · 2026-03-26T08:05:12Z

For this PR, I would strongly recommend to create a documentation page describing this setup for operators and having in place describing alternatives. The honest answer without offence is: this in Application TLS is probably not worth the complexity it adds, this is opinionated answer, but some security best practice should be documented, and use cases with tradeoffs.

Something like a page to consider other approaches, before diving in APP tls setup and certificate lifecycle world. As an operator, I would like to know why I'm now managing certs, handling rotation, and adding TLS overhead on connections that are already encrypted at the network layer(as example).

Alternative approaches and tradeoff:

Just use this PR

Makes sense if: no service mesh, small cluster, you control the CA, Prow is the only thing needing this
Low complexity, self-contained
The manual cert rotation problem is real though -> nothing in this PR handles cert rotation, and no description, no metric emitted with cert expiry

Service mesh (Istio, Linkerd, etc)

mTLS between pods is automatic and transparent — no code changes, no cert management in the app
Identity is based on SPIFFE/SPIRE workload identity, not manually provisioned certs
You get observability, retries, circuit breaking for free
Downside: operational complexity to run the mesh, but it worth depends on client/use-case
If you already have a mesh, this APP TLS is unnecessary overhead

Zero trust / SPIFFE

Similar to service mesh but more explicit — each workload gets a cryptographic identity
Works well across clusters and cloud boundaries
Downside: operational complexity to run the mesh, but it worth depends on client/use-case. More portable then mesh
Better long-term if you're building toward zero trust across multiple environments

Prow running within a private network / network perimeter

All traffic between nodes/pods travels over an encrypted tunnel
The ingress→pod HTTP gap this PR is trying to close is already encrypted at the network layer
Gap in: application-layer identity (any pod on the VPN can talk to any other pod), Compromised node and Compliance requirements

k8s-ci-robot added area/deck Issues or PRs related to prow's deck component area/hook Issues or PRs related to prow's hook component labels Dec 10, 2025

k8s-ci-robot requested review from cjwagner and smg247 December 10, 2025 19:16

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 10, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 10, 2025

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 10, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 10, 2025

kayhern force-pushed the add-option-for-ssl branch 2 times, most recently from b32381d to 2cfbae6 Compare December 16, 2025 15:12

petr-muller requested changes Dec 17, 2025

View reviewed changes

k8s-ci-robot assigned petr-muller Dec 17, 2025

kayhern force-pushed the add-option-for-ssl branch from 2cfbae6 to 9865cdd Compare January 15, 2026 13:31

Adding option to enable Back End HTTPS for Prow Ingress

2ad6fa1

Signed-off-by: Nikhil Jiju <Nikhil.Jiju@fmr.com>

kayhern force-pushed the add-option-for-ssl branch from 9865cdd to 2ad6fa1 Compare January 16, 2026 13:26

NiJuFirenzia requested a review from petr-muller January 16, 2026 14:51

petr-muller reviewed Jan 24, 2026

View reviewed changes

Updating ssl flagutil and removing unnecessary option

814fb94

Signed-off-by: Nikhil Jiju <Nikhil.Jiju@fmr.com>

ivankatliarchuk suggested changes Mar 26, 2026

View reviewed changes

k8s-ci-robot assigned ivankatliarchuk Mar 26, 2026

	const tlsEnabledScehma = "https"
	const tlsEnabledSchema = "https"

	return errors.New("flag --enable-ssl was set to true but required flag --cert-file was not set")
	return errors.New("flag --enable-ssl was set to true but required flag --server-cert-file was not set")

	sslEnablement prowflagutil.SSLEnablementOptions
	ssl prowflagutil.SSLEnablementOptions

	const sslEnabledSchema = "https"
	const sslEnabledScheme = "https"

Conversation

NiJuFirenzia commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify Bot commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for k8s-prow ready!

Uh oh!

linux-foundation-easycla Bot commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Dec 10, 2025

Uh oh!

k8s-ci-robot commented Dec 10, 2025

Uh oh!

NiJuFirenzia commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthyx commented Dec 10, 2025

Uh oh!

matthyx commented Dec 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jan 15, 2026

Uh oh!

NiJuFirenzia commented Jan 16, 2026

Uh oh!

petr-muller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivankatliarchuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivankatliarchuk commented Mar 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NiJuFirenzia commented Dec 10, 2025 •

edited

Loading

netlify Bot commented Dec 10, 2025 •

edited

Loading

linux-foundation-easycla Bot commented Dec 10, 2025 •

edited

Loading

NiJuFirenzia commented Dec 10, 2025 •

edited

Loading