Allow HTTPProxy to specify multiple SubjectNames for Upstream TLS Validation

[KauzClay]

Please describe the problem you have I am trying to secure the data path for a request when using Contour with Knative. The implementation for securing the data path in Knative is still evolving, but one of difficulties comes from Knative's ways of managing scale-to-zero. For a given Knative service, the endpoints of the corresponding K8s service to either point to the user-container pod, or the Knative Activator pod (when the user-container pod is scaled to zero). There is no indication on the K8s service whether it is in one state or the other.

This means that in order to secure and validate traffic to the backend (a la Upstream TLS), we would need Contour to accept either the subjectName for the user-container pod OR the subjectName for the Activator.

We currently get around this by keeping the Activator in the request path always, so that we only need to provide one subjectName. But that is not an ideal end state for the feature.

The current API for this is:

apiVersion: projectcontour.io/v1
kind: HTTPProxy
metadata:
  name: example
spec:
  virtualhost:
    fqdn: www.example.com
  routes:
  - services:
    - name: secure-backend
      port: 8443
      validation:
        caSecret: my-certificate-authority
        subjectName: backend.example.com

It would be nice if we could expand it to something like:

apiVersion: projectcontour.io/v1
kind: HTTPProxy
metadata:
  name: example
spec:
  virtualhost:
    fqdn: www.example.com
  routes:
  - services:
    - name: secure-backend
      port: 8443
      validation:
        caSecret: my-certificate-authority
        subjectName: backend.example.com
        subjectNames:
        - backend.example.com
        - activator.knative.internal

Envoy itself has this capability, but I realize the HTTPProxy spec is in v1, so it may not be so easy to add.

Either way, I'm curious how others feel about this. I'm happy to work on a contribution if we are able to come to agreement on a way forward.

[github-actions[bot]]

Hey @KauzClay! Thanks for opening your first issue. We appreciate your contribution and welcome you to our community! We are glad to have you here and to have your input on Contour. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace

[KauzClay]

As a first pitch, there is the hacky way around. Perhaps we could change subjectName to accept a comma separated list of subject names?

so something like:

subjectName: "name1.example.com,name2.example.com"

I admit that this isn't ideal. And it deviates from the standard ways of interacting with a k8s crd spec.

[KauzClay]

Another option could be to just add a new optional field called SubjectNames, which is a list of strings, in addition to SubjectName.

We could take the values of both and combine/dedupe them. That way, users with the old version of the spec that just have SubjectName wouldn't be broken.

subjectName: name1.example.com
subjectNames:
- name1.example.com
- name2.example.com

Would become

[]string{"name1.example.com", "name2.example.com"}

Combining the input of two different fields might not be great either, but it at least lets us have a list of options.

A variation of this would to only allow setting one or the other. So users who have subjectName wouldn't be affected, but those who want to switch would intentionally switch to subjectNames. But idk if the crds are flexible enough to do this.

[skriss]

Thanks for writing this up in detail @KauzClay, I agree that those seem like our two main options. Let me look around a bit for some prior art and see if we can make a decision on which way to go.

[sunjayBhatia]

theres an upstream reference on this sort of idea: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#making-a-singular-field-plural

its a little more involved to do things the "right" way since for forward/backward compatibility we would have to populate the pluralized form of the field on read of an object that was specified with only the singular etc. (more detail in the link)

[skriss]

I think https://github.com/projectcontour/contour/issues/5520#issuecomment-1602800374 is reasonable to proceed with; we can mark the existing field as deprecated and plan to remove it if we ever have a v2 API (https://projectcontour.io/resources/deprecation-policy/ says we can remove a field with a new API version). Probably the simplest thing to do is add an error condition to the HTTPProxy if both fields are set simultaneously -- avoids having to deal with any complex merging/de-duping.

@sunjayBhatia @tsaarni that sound like a reasonable approach?

[tsaarni]

I wonder is it possible to skip implementing the pluralization rules that @sunjayBhatia linked https://github.com/projectcontour/contour/issues/5520#issuecomment-1603414729? Wouldn't adding an error condition when both fields are set imply that current subjectName must be changed to optional? After that change, would downgrade fail on resources that have no subjectName?

[sunjayBhatia]

Given we don't plan on upping the version on the CRDs seems like we need to support "older" clients that don't know about the new plural field

To enumerate the cases where this solution would/not work for an older client:

• Old client would send only the singular, which is fine since the field still exists in the new version
• Old client does not set the singular in a create, would be marked as invalid in status since neither singular or plural is set
• In an update (as opposed to a create or patch), if an old client reads a resource created by a newer client that only set the plural, they would miss that there was any SAN set, so they would effectively be trying to unset the singular field and the plural field (since they are not aware of the plural one)? Seems like this would then be marked as invalid in status but allowed to exist by the API server since both fields would be optional
  • Could be somewhat mitigated further if we don't set singular field as optional in the resource created by a new client, you always have to set singular field and singular must equal plural[0] if plural nonempty, would set as invalid if not true, however an invalid resource would still be allowed to exist (with invalid status) so not perfect
  • Re: the rule above, maybe this is a place where we can start using CEL validations on the CRD so the API server rejects invalid resources rather than requiring the contour controller to set them as invalid, this way the resource would never exist without the singular field set
  • Could be fully mitigated with a conversion webhook that populates the singular field on read if plural is set, but we don't have that today

An updated client would send singular or plural, but not both

Maybe worth checking the assumptions here with a spike to see if the UX is acceptable for an older client

We don't really test/guarantee anywhere about downgrading CRDs at the moment so maybe that case is ok to ignore? We don't implement a conversion webhook at the moment either but that is a more heavyweight solution to get full compatibility forwards and backwards

[sunjayBhatia]

messing around with CEL for a bit, this diff seems to work, maybe needs more bounds checking etc. but it rejects things like

subjectName: foo
subjectNames: [bar] // or empty list

but accepts when the plural field is omitted

diff --git a/apis/projectcontour/v1/httpproxy.go b/apis/projectcontour/v1/httpproxy.go
index db777345..9c4c48c3 100644
--- a/apis/projectcontour/v1/httpproxy.go
+++ b/apis/projectcontour/v1/httpproxy.go
@@ -1294,6 +1294,7 @@ type HeaderValue struct {
 }

 // UpstreamValidation defines how to verify the backend service's certificate
+// +kubebuilder:validation:XValidation:message="subjectNames[0] must equal subjectName if set",rule="has(self.subjectNames) ? self.subjectNames[0] == self.subjectName : true"
 type UpstreamValidation struct {
        // Name or namespaced name of the Kubernetes secret used to validate the certificate presented by the backend.
        // The secret must contain key named ca.crt.
@@ -1301,7 +1302,16 @@ type UpstreamValidation struct {
        // When cross-namespace reference is used, TLSCertificateDelegation resource must exist in the namespace to grant access to the secret.
        CACertificate string `json:"caSecret"`
        // Key which is expected to be present in the 'subjectAltName' of the presented certificate.
+       // Deprecated, migrate to using the plural field subjectNames.
+       // +kubebuilder:validation:MinLength=1
+       // +kubebuilder:validation:MaxLength=256
        SubjectName string `json:"subjectName"`
+       // List of keys, of which at least one is expected to be present in the 'subjectAltName of the
+       // presented certificate.
+       // +optional
+       // +kubebuilder:validation:MinItems=1
+       // +kubebuilder:validation:MaxItems=8
+       SubjectNames []string `json:"subjectNames"`
 }

 // DownstreamValidation defines how to verify the client certificate.