Add certificates config keys in config-network by nak3 · Pull Request #648 · knative/networking

nak3 · 2022-04-06T09:01:31Z

This patch adds the following certificate variables:

activator-server-certs
queue-proxy-ca
queue-proxy-san
queue-proxy-server-certs

It is similar to #608.

knative/serving#12815 and knative/serving#12770
verified the change.

This patch adds the following certificate variables: - `activator-server-certs` - `queue-proxy-ca` - `queue-proxy-san` - `queue-proxy-server-certs` It is similar to #608. knative/serving#12815 and knative/serving#12770 verifeid the change.

knative-prow · 2022-04-06T09:01:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [nak3]

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

codecov · 2022-04-06T09:06:53Z

Codecov Report

Merging #648 (1e34dd9) into main (a1261cd) will increase coverage by 0.29%.
The diff coverage is 100.00%.

❗ Current head 1e34dd9 differs from pull request most recent head 169e8c0. Consider uploading reports for the commit 169e8c0 to get more accurate results

@@            Coverage Diff             @@
##             main     #648      +/-   ##
==========================================
+ Coverage   94.56%   94.85%   +0.29%     
==========================================
  Files          38       38              
  Lines         810     1245     +435     
==========================================
+ Hits          766     1181     +415     
- Misses         31       52      +21     
+ Partials       13       12       -1

Impacted Files	Coverage Δ
pkg/network.go	`89.04% <100.00%> (+3.12%)`	⬆️
pkg/prober/prober.go	`96.80% <0.00%> (-0.17%)`	⬇️
pkg/stats.go	`100.00% <0.00%> (ø)`
pkg/bufferpool.go	`100.00% <0.00%> (ø)`
pkg/probe_handler.go	`100.00% <0.00%> (ø)`
pkg/apis/config/store.go	`100.00% <0.00%> (ø)`
pkg/testing/status/fake.go	`100.00% <0.00%> (ø)`
pkg/apis/networking/ports.go	`100.00% <0.00%> (ø)`
pkg/apis/networking/register.go	`0.00% <0.00%> (ø)`
pkg/apis/networking/generic_types.go	`100.00% <0.00%> (ø)`
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dde40b0...169e8c0. Read the comment docs.

evankanderson

I think we want to have a pattern for queue-proxy SANs which incorporates at least the namespace name, and possibly also the revision name.

I'm less concerned about revision name, but it would be nice if we could use the SAN to prove that we don't "cross the streams" at the networking layer and dispatch a request to a queue-proxy at the wrong namespace.

evankanderson · 2022-04-06T13:51:37Z

+    # The SAN (Subject Alt Name) used to validate the activator TLS certificate.
+    # It must be set when "queue-proxy-ca" is specified.
+    # Use an empty value to disable the feature (default).
+    #
+    # NOTE: This flag is in an alpha state and is mostly here to enable internal testing
+    #       for now. Use with caution.
+    queue-proxy-san: ""
+
+    # The server certificates to serve the TLS traffic from activator to queue-proxy.
+    # It is specified by the secret name, which has the "tls.crt" and "tls.key" data field.
+    # Use an empty value to disable the feature (default).
+    #
+    # NOTE: This flag is in an alpha state and is mostly here to enable internal testing
+    #       for now. Use with caution.
+    queue-proxy-server-certs: ""


Shouldn't queue-proxies in different namespaces get different SANs and Certs?

Yes, it should get the different SANs and certs. But for the alpha release, I assumed that:

admin deploys secret (server certs) in each application namespace with the name specified in queue-proxy-server-certs.

queue-proxy mounts the secret via volume mount.

The different SANs and certs are very difficult to implement and we need to consider about another traffic ingress -> queue-proxy in the future.

It seems important that a user in one namespace not have access to the cert being used in a different namespace. SANs feel like the way to do that -- we can start with having the admins manually create the certs using something like Cert-Manager or by hand, and eventually enhance Knative to do this before GA.

I think we need to validate SAN/certs at service creation time. It should be done for GA. For now its ok to test with shared certs I guess.

evankanderson · 2022-04-06T13:52:36Z

+    # The server certificates to serve the TLS traffic from ingress to activator.
+    # It is specified by the secret name, which has the "tls.crt" and "tls.key" data field.
+    # Use an empty value to disable the feature (default).
+    #
+    # NOTE: This flag is in an alpha state and is mostly here to enable internal testing
+    #       for now. Use with caution.
+    activator-server-certs: ""


How about activator-cert-secret if we need this to be configurable? (I'm okay with it being configurable, though I'm not sure how useful that feature might be vs simply having a hard-coded name.)

I wanted make this configurable as activator can turn on the https serving or not based on the config as:

if networkConfig.ActivatorServerCert != "" { // activator starts serving https. }

For the name of activator-cert-secret vs activator-server-certs, we already have the activator ca as activator-ca (without -secert) so it should be consistent?

I'd thought the activator-ca was the actually CA certificate (which doesn't need to be kept private, because it's a certificate, not keys).

I'd prefer to have the -secret name here, because it provides a clearer clue that this is a value that needs to be kept private as well.

nak3 · 2022-04-07T03:59:01Z

For queue-proxy's SANs and certs, it needs to consider about the ingress -> queue-proxy, which we have a plan to implement in the future, right? I think it is difficult to determine/implement it with the namespace level in this alpha release...

nak3 · 2022-04-07T23:50:00Z

/cc @skonto @rhuss

evankanderson · 2022-04-11T05:44:52Z

For queue-proxy's SANs and certs, it needs to consider about the ingress -> queue-proxy, which we have a plan to implement in the future, right? I think it is difficult to determine/implement it with the namespace level in this alpha release...

I think the challenge for ingress -> queue-proxy is twofold:

Need some way to teach Ingresses about SANs and CAs associated with particular endpoints (i.e. a mix of activator and queue-proxy pods will have different SANs depending on the endpoint).
Need some way to teach individual Ingresses about per-SLS SANs, which means we probably need to put that information in KIngress, which means a "protocol feature bump" in a way that is harder than activator -> queue-proxy, where we control all the code (which is in Go).

I'd be happy with either hard-coding the SAN as equal to the internal cluster service name, or something like {{namespace}}.knative.

skonto · 2022-04-11T08:42:26Z

I think the challenge for ingress -> queue-proxy is twofold:
...

I would also vote for moving the san/cert info in the ingress part.

I'd be happy with either hard-coding the SAN as equal to the internal cluster service name, or something like {{namespace}}.knative.

SAN can be many things afaik. Could users use the service without the internal name (thinking out loud)?

skonto · 2022-04-11T08:59:17Z

/lgm

evankanderson · 2022-04-11T19:52:47Z

I think the challenge for ingress -> queue-proxy is twofold:
...

I would also vote for moving the san/cert info in the ingress part.

I'm not sure I understand the comment here. Do you mean move the SAN and Cert info in the KIngress, or into a different phase of the implementation.

I'd be happy with either hard-coding the SAN as equal to the internal cluster service name, or something like {{namespace}}.knative.

SAN can be many things afaik. Could users use the service without the internal name (thinking out loud)?

SAN is a field in the CA certificate; I don't think we need to align those values with specific resolvable DNS addresses. I think I'm suggesting using the dnsNames field here only because that seems to be supported by cert-manager, but I'm imagining that we'd have a custom validator and wouldn't want other systems using the internal name and attempting validation.

nak3 · 2022-04-12T00:52:26Z

@skonto @evankanderson Is it possible to merge this into 1.4 😅

evankanderson · 2022-04-12T01:01:24Z

@skonto @evankanderson Is it possible to merge this into 1.4 😅

I'd like to understand the plan for queue-proxy certs. Is it to copy a shared cert to each namespace for 1.4, and then to replace them (and the related config options) in 1.5?

nak3 · 2022-04-12T01:10:48Z

Is it to copy a shared cert to each namespace for 1.4, and then to replace them (and the related config options) in 1.5?

Yes, I assumed that.

evankanderson · 2022-04-12T02:49:20Z

I think this probably means that the feature will be pre-alpha for 1.4, but as long as we're fine with throwing out some or all of users configuration when we ship 1.5.

/lgtm

Add certificates keys in config-network

6711637

This patch adds the following certificate variables: - `activator-server-certs` - `queue-proxy-ca` - `queue-proxy-san` - `queue-proxy-server-certs` It is similar to #608. knative/serving#12815 and knative/serving#12770 verifeid the change.

knative-prow Bot requested a review from evankanderson April 6, 2022 09:01

knative-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 6, 2022

knative-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2022

This was referenced Apr 6, 2022

All Hops Encrypted: Activator uses queue-proxy TLS if available knative/serving#12503

Closed

All Hops Encrypted: Queue-Proxy serving TLS knative/serving#12502

Closed

All Hops Encrypted: Activator serving TLS knative/serving#12501

Closed

nak3 mentioned this pull request Apr 6, 2022

All Hops Encrypted: Activator serving TLS knative/serving#12770

Merged

7 tasks

evankanderson reviewed Apr 6, 2022

View reviewed changes

knative-prow Bot requested review from rhuss and skonto April 7, 2022 23:50

nak3 added 3 commits April 11, 2022 16:03

Rename -server-certs with -cert-secret

3e704c8

Bump checksum

1e34dd9

Fix lint

169e8c0

nak3 mentioned this pull request Apr 11, 2022

All Hops Encrypted: TLS between activator and queue-Proxy knative/serving#12815

Merged

4 tasks

knative-prow Bot assigned evankanderson Apr 12, 2022

knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2022

knative-prow Bot merged commit c173eed into knative:main Apr 12, 2022

psschwei mentioned this pull request Jun 14, 2022

Support internal-encryption to deploy internal certificates automatically #680

Merged

Conversation

nak3 commented Apr 6, 2022

Uh oh!

knative-prow Bot commented Apr 6, 2022

Uh oh!

codecov Bot commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

evankanderson 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!

Choose a reason for hiding this comment

Uh oh!

nak3 commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nak3 commented Apr 7, 2022

Uh oh!

evankanderson commented Apr 11, 2022

Uh oh!

skonto commented Apr 11, 2022

Uh oh!

skonto commented Apr 11, 2022

Uh oh!

evankanderson commented Apr 11, 2022

Uh oh!

nak3 commented Apr 12, 2022

Uh oh!

evankanderson commented Apr 12, 2022

Uh oh!

nak3 commented Apr 12, 2022

Uh oh!

evankanderson commented Apr 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Apr 6, 2022 •

edited

Loading

nak3 commented Apr 7, 2022 •

edited

Loading