Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support and test multiple TLS certificates per Gateway listener #1330

Open
bowei opened this issue Aug 15, 2022 · 18 comments
Open

Support and test multiple TLS certificates per Gateway listener #1330

bowei opened this issue Aug 15, 2022 · 18 comments
Assignees
Labels
area/conformance kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@bowei
Copy link
Contributor

bowei commented Aug 15, 2022

What would you like to be added:

We should clarify the API Spec to clarify what happens when multiple TLS Certificates are specified on a Listener. For example, what happens when those certificates overlap or conflict? Once we clarify the spec, we'll also need a conformance test to verify the behavior.

Why this is needed:

Attaching multiple TLS certificates to a Gateway listener should have extended support.

@bowei bowei added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 15, 2022
@shaneutt shaneutt added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. area/conformance labels Aug 15, 2022
@robscott
Copy link
Member

This is one of the bullet points of #1104 (although that doesn't explicitly require multiple certs), adding a link from there as well.

@ksankeerth
Copy link

I’m interested in working on this conformance test. I went through the documentation. From my understanding, the conformance test for Gateway HTTPS listener references multiple TLS certificates should include the below aspects

  1. The GatewayClass MUST use the longest matching SNI out of all available certificates for any TLS handshake.
  2. The SNIs of all the certificates MUST match with the listener's hostname

Did I miss anything here? @robscott @bowei

@youngnick
Copy link
Contributor

Sorry for the delay, @ksankeerth, that looks right to me.

@ksankeerth
Copy link

Thanks @youngnick. I'll send a PR to cover these tests. Let's get others' opinions after the PR

@ksankeerth
Copy link

Hi @youngnick,
I sent a PR with a conformance test. The test cases were written to check the longest SNI when multiple TLS certs are available. If possible, please review it.

@skriss
Copy link
Contributor

skriss commented Sep 6, 2022

My understanding was that multiple certificate refs per Listener was "custom" conformance, per:
https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1beta1/gateway_types.go#L312-L314
https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1beta1/gateway_types.go#L328-L330

Am I missing something here?

(Per @robscott's point in #1330 (comment), we do still need a test for a single TLS cert, though).

@rainest
Copy link
Contributor

rainest commented Sep 6, 2022

No, I think you're correct @skriss. It looks like the language is a bit confusing, where https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.Listener has

The GatewayClass MUST use the longest matching SNI out of all available certificates for any TLS handshake.
Support: Core

under the tls field, which I think meant to indicate that the tls field itself is core, and then also included some additional tips related to if. After, https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.GatewayTLSConfig, the type that goes in the tls field, indicates that multiple certificate support is expanded. The longest SNI bit should maybe reside in the GatewayTLSConfig docs instead of Listener, or the Listener doc should be amended to note that this only applies if you do implement the optional expanded feature (and presumably that should be "Gateway" instead of "GatewayClass").

The proposed change tests the longest SNI bit only (though I'm not sure there's anything more to test for multiple refs, and comments above seem to agree).

@ksankeerth
Copy link

@rainest Sorry for the confusion related to the proposed changes. I also looked at the same doc. The Longest Matching SNI was mentioned as support: core. The second proposed test seems unnecessary. As “Host Header” and “Hostname” should match, I thought the “SAN” or “CommonName” of certificates also match with “Hostname”. I think it’s not necessary for Support: core. Even in the PR, I didn’t include any tests for that. #1375

The proposed change tests the longest SNI bit only (though I'm not sure there's anything more to test for multiple refs, and comments above seem to agree).

@shaneutt shaneutt added this to the v0.6.1 milestone Nov 2, 2022
@youngnick
Copy link
Contributor

We discussed this one in a triage meeting, and although we have #1375 open, before we merge that one, we need another PR to move the multiple certs field to Extended support, and we can discuss there if we all agree (seems likely we do though).

@ksankeerth, any chance you are game to give that one a go?

@ksankeerth
Copy link

We discussed this one in a triage meeting, and although we have #1375 open, before we merge that one, we need another PR to move the multiple certs field to Extended support, and we can discuss there if we all agree (seems likely we do though).

@ksankeerth, any chance you are game to give that one a go?

@youngnick I submitted a PR for this. #1526

@shaneutt shaneutt modified the milestones: v0.6.1, v0.6.2 Jan 31, 2023
@shaneutt shaneutt added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Feb 21, 2023
@Xunzhuo
Copy link
Member

Xunzhuo commented Mar 8, 2023

Hey @ksankeerth, since you are working on it now. May I remove help and assign this to you?

@shaneutt
Copy link
Member

shaneutt commented Mar 8, 2023

It's marked to fix this, so I think we're good @Xunzhuo we just need to catch up on our PR review backlog 😅

/assign @ksankeerth

@shaneutt shaneutt moved this from Todo to In Progress in Gateway API: The Road to GA Mar 8, 2023
@shaneutt shaneutt modified the milestones: v0.6.2, v1.0.0 Mar 8, 2023
@robscott robscott changed the title Conformance test for multiple TLS certificates Support and test multiple TLS certificates per Gateway listener May 18, 2023
@shaneutt
Copy link
Member

@robscott and I talked about this one in grooming, we've kinda gotten stuck and need to rethink how we're going to move forward

/assign @robscott

@robscott
Copy link
Member

I think this issue has gotten stuck, and to unblock it we're going to need a clear understanding of how underlying dataplanes handle multiple certificates. Before we can call this extended, we need to:

  1. List out how underlying dataplanes handle multiple TLS certs, particularly when they're conflicting or overlapping. At a minimum this should cover NGINX, HAProxy, and Envoy.
  2. Determine if there is sufficiently overlapping behavior between implementations, if so, define what that is and build that into Gateway API spec.

Until we have this figured out, we can't move forward with graduating multiple TLS certs to "Extended" support.

@rainest
Copy link
Contributor

rainest commented Jul 24, 2023

Kong's implementation:

  1. Allows provisioning an a single alternate certificate, in order to offer different key types (e.g. RSA and ECDSA) depending on what a client supports.
  2. For overlapping certificates, will serve whichever certificate has the more specific hostname. Given a request for foo.example.com, Kong will serve a foo.example.com over a *.example.com certificate.
  3. Does not allow exact overlaps. Attempting to configure a second foo.example.com certificate outside the alternate system in (1) will fail.

We intended to add support for (1) to meet the spec as it is. The behavior around overlapping/duplicate certificates doesn't fall under the existing spec, as the additional certificates are configured as part of a Listener, which already specifies a hostname independent of the certificates attached to the Listener.

Our implementation does not automatically serve certificates based on their CN or SANs. Our SNI->selected certificate configuration is independent of the certificate contents, so we create a mapping from the Listener hostname to the certificates the Listener references.

I think the spec could reasonably specify rules for overlapping hostnames across different Listeners, as I wouldn't expect the "choose the most specific hostname you can" rule would be controversial.

I don't think we could easily specify conformance rules for handling of additional certificates configured within a Listener. I would not be surprised if additional certificates vary in purpose across vendors.

@shaneutt shaneutt removed this from the v1.0.0 milestone Sep 27, 2023
@shaneutt
Copy link
Member

I think we need to re-triage this and make sure we're aligned on what's next for this one.

/triage needs-information

@k8s-ci-robot k8s-ci-robot added the triage/needs-information Indicates an issue needs more information in order to work on it. label Sep 18, 2024
@shaneutt shaneutt removed the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Sep 18, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 17, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/conformance kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
Status: Triage
10 participants