Add upstream TLS trust from CM bundles #1171

ReToCode · 2023-12-06T09:03:10Z

Changes

Adding support to trust multiple CAs (when a label is present) for upstream TLS connections
Checks if CAs are parsable before adding them to envoy
Additional to reading the ca.crt in activators cert secret, net-kourier now also adds additional trust bundles to the trust pool
Dropped the fallback implementation of reading service port names. With the above config read fix, this is no longer necessary
Tested the full set with CA rotation here

Release Note

net-kourier now allows to add additional CA trust bundles in ConfigMaps to verify upstream TLS connections

Docs

Will be added once the full encryption feature is completed.

knative-prow · 2023-12-06T09:03:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ReToCode

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ReToCode]

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

ReToCode · 2023-12-06T09:05:37Z

@skonto @dprotaso what do you think of that approach for knative/serving#14609.

With this approach, this is not tied to trust-manager. ConfigMaps seem to be a very common interface for CA bundles, see https://github.com/ReToCode/knative-encryption/tree/main/8-trust-sources.

We can also extend this to knative/serving#14609 (comment) once this has landed. But I'd vote for reading it using the K8s clients instead of mounting it to the filesystem, as this solves the reloading concern.

codecov · 2023-12-06T09:05:59Z

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (23848ce) 81.04% compared to head (2a72a55) 80.94%.
Report is 1 commits behind head on main.

❗ Current head 2a72a55 differs from pull request most recent head 9529993. Consider uploading reports for the commit 9529993 to get more accurate results

Files	Patch %	Lines
pkg/generator/ingress_translator.go	75.00%	12 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1171      +/-   ##
==========================================
- Coverage   81.04%   80.94%   -0.10%     
==========================================
  Files          18       18              
  Lines        1456     1496      +40     
==========================================
+ Hits         1180     1211      +31     
- Misses        219      223       +4     
- Partials       57       62       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

skonto · 2023-12-07T14:22:08Z

ConfigMaps seem to be a very common interface for CA bundles,

In general I see secrets being used as well. For trust manager it seems cm are first option. I don't have strong preference.

pkg/generator/ingress_translator.go

vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/configmap.go

pkg/generator/ingress_translator_test.go

ReToCode · 2023-12-08T09:02:37Z

In general I see secrets being used as well. For trust manager it seems cm are first option. I don't have strong preference.

Hm where do you mean? I don't think we have a current solution (other than ca.crt) that would put the bundles in a Secret. But we can always extend if necessary.

skonto · 2023-12-08T10:36:53Z

Hm where do you mean? I

I was referring to the docs: https://cert-manager.io/docs/trust/trust-manager

Bundle resources currently support several source types:

configMap - a ConfigMap resource in the trust-manager namespace
secret - a Secret resource in the trust-manager namespace
inLine - a manually specified string containing at least one certificate
useDefaultCAs - usually, a bundle of publicly trusted certificates

ReToCode · 2023-12-08T10:38:51Z

I was referring to the docs: https://cert-manager.io/docs/trust/trust-manager

That is just for sources when I'm reading it right. The result to mount in workloads is always a Configmap?

skonto · 2023-12-08T11:43:42Z

It seems it supports: sources/targets.

ConfigMap is the default target type, but as of v0.7.0 trust-manager also supports Secret resources as targets.

ReToCode · 2023-12-14T13:22:20Z

@skonto this is ready now, please take another look.

/unhold

pkg/reconciler/ingress/controller.go

pkg/generator/ingress_translator.go

pkg/reconciler/ingress/controller.go

skonto · 2023-12-14T15:47:42Z

pkg/generator/ingress_translator.go

@@ -466,3 +517,24 @@ func domainsForRule(rule v1alpha1.IngressRule) []string {
 	}
 	return domains
 }
+
+func checkCertBundle(certs []byte) error {


As a side note it seems that this error is not being caught by decode (probably not considered an error):

var rsaSecretCertPEM = `-----BEGIN CERTIFICATE----- -----BEGIN CERTIFICATE----- MIIB0zCCAX2gAwIBAgIJAI/M7BYjwB+uMA0GCSqGSIb3DQEBBQUAMEUxCzAJBgNV BAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5ldCBX aWRnaXRzIFB0eSBMdGQwHhcNMTIwOTEyMjE1MjAyWhcNMTUwOTEyMjE1MjAyWjBF MQswCQYDVQQGEwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UECgwYSW50 ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANLJ hPHhITqQbPklG3ibCVxwGMRfp/v4XqhfdQHdcVfHap6NQ5Wok/4xIA+ui35/MmNa rtNuC+BdZ1tMuVCPFZcCAwEAAaNQME4wHQYDVR0OBBYEFJvKs8RfJaXTH08W+SGv zQyKn0H8MB8GA1UdIwQYMBaAFJvKs8RfJaXTH08W+SGvzQyKn0H8MAwGA1UdEwQF MAMBAf8wDQYJKoZIhvcNAQEFBQADQQBJlffJHybjDGxRMqaRmDhX0+6v02TUKZsW r5QuVbpQhH6u+0UgcW0jp9QwpxoPTLTWGXEWBBBurxFwiCBhkQ+V -----END CERTIFICATE----- `

I played around with decode a bit.
The way it is written:

func Decode(data []byte) (p *Block, rest []byte) { // pemStart begins with a newline. However, at the very beginning of // the byte array, we'll accept the start string without it. rest = data for { if bytes.HasPrefix(rest, pemStart[1:]) { rest = rest[len(pemStart)-1:] } else if _, after, ok := bytes.Cut(rest, pemStart); ok { rest = after } else { return nil, data } var typeLine []byte typeLine, rest = getLine(rest) if !bytes.HasSuffix(typeLine, pemEndOfLine) { continue } typeLine = typeLine[0 : len(typeLine)-len(pemEndOfLine)] p = &Block{ Headers: make(map[string]string), Type: string(typeLine), } ...

It seems it is always going to consume the input.
I am wondering if there is a scenario where block== nil but we have len(rest) >0.

Yes I had one case with that, that's why I added

|| len(rest) > 0, this way we error out in checkCertBundle when there is still a rest that is not consumed.

Yeah I think it parses from comment to comment, so having multiple -----BEGIN CERTIFICATE----- is just ignored, as long as the cert within is valid.

skonto · 2023-12-14T16:08:17Z

pkg/generator/ingress_translator.go

+		if block != nil {
+			switch block.Type {
+			case "CERTIFICATE":
+				_, err := x509.ParseCertificate(block.Bytes)


Are we planning to do any verification at some point like with this function: https://github.com/istio/istio/blob/master/security/pkg/pki/util/keycertbundle.go#L284 or as soon as we get the cert bundle we assume an external entity is responsible to create valid combinations of ca certs/tls certs etc?

Yes, we give the CA to envoy, envoy will then use it to verify the upstream TLS server cert. I don't think we need to pre-validate. Otherwise we'd also need to reconcile on a number of upstream certs which we actually don't really want to read from each ingress.

Yeah just wondering if Istio is doing a pre-validation tep.

pkg/generator/ingress_translator.go

skonto · 2023-12-15T10:44:10Z

pkg/generator/ingress_translator_test.go

+						false,
+						&envoycorev3.TransportSocket{
+							Name:       wellknown.TransportSocketTls,
+							ConfigType: typedConfig(false, nil),


Passing nil here (also tested with a missing cm) means that we end up having no validation since this is passed to

net-kourier/pkg/generator/ingress_translator_test.go

Line 2078 in 2a72a55

InlineBytes: expectedCert,

.
Check comments here too:

net-kourier/vendor/github.com/envoyproxy/go-control-plane/envoy/extensions/transport_sockets/tls/v3/common.pb.go

Lines 816 to 835 in 2a72a55

// TLS certificate data containing certificate authority certificates to use in verifying

// a presented peer certificate (e.g. server certificate for clusters or client certificate

// for listeners). If not specified and a peer certificate is presented it will not be

// verified. By default, a client certificate is optional, unless one of the additional

// options (:ref:`require_client_certificate

// <envoy_v3_api_field_extensions.transport_sockets.tls.v3.DownstreamTlsContext.require_client_certificate>`,

// :ref:`verify_certificate_spki

// <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.verify_certificate_spki>`,

// :ref:`verify_certificate_hash

// <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.verify_certificate_hash>`, or

// :ref:`match_typed_subject_alt_names

// <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.match_typed_subject_alt_names>`) is also

// specified.

//

// It can optionally contain certificate revocation lists, in which case Envoy will verify

// that the presented peer certificate has not been revoked by one of the included CRLs. Note

// that if a CRL is provided for any certificate authority in a trust chain, a CRL must be

// provided for all certificate authorities in that chain. Failure to do so will result in

// verification failure for both revoked and unrevoked certificates from that chain.

// The behavior of requiring all certificates to contain CRLs can be altered by

Would that allow tls communication without validation or connection is failing?
Also do you think that this affects us envoyproxy/envoy#19326 downstream? 🤔

What about this part:

" Note
// that if a CRL is provided for any certificate authority in a trust chain, a CRL must be
// provided for all certificate authorities in that chain. Failure to do so will result in
// verification failure for both revoked and unrevoked certificates from that chain."
Do you know the implications of this (I am not familiar with this case)?

skonto · 2023-12-18T15:32:31Z

/lgtm
@ReToCode feel free to unhold if you are ready.

skonto · 2023-12-18T15:32:43Z

/hold

ReToCode · 2023-12-19T06:03:05Z

/unhold

ReToCode requested review from dprotaso and skonto December 6, 2023 09:03

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 6, 2023

knative-prow bot requested review from jsanin-vmw and xtreme-vikram-yadav December 6, 2023 09:03

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 6, 2023

ReToCode mentioned this pull request Dec 7, 2023

implement trusting a CA bundle knative/serving#14609

Closed

4 tasks

skonto reviewed Dec 8, 2023

View reviewed changes

pkg/generator/ingress_translator.go Show resolved Hide resolved

skonto reviewed Dec 8, 2023

View reviewed changes

vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/configmap.go Outdated Show resolved Hide resolved

skonto reviewed Dec 8, 2023

View reviewed changes

pkg/generator/ingress_translator_test.go Outdated Show resolved Hide resolved

ReToCode force-pushed the trust-bundle branch from fb8d388 to 33ab71d Compare December 8, 2023 10:33

ReToCode force-pushed the trust-bundle branch from 33ab71d to 92f2583 Compare December 13, 2023 12:32

ReToCode added 5 commits December 14, 2023 14:16

Add upstream TLS trust from CM bundles

daec9ae

Lint warnings

e512915

Get Secret and ConfigMaps from system namespace

13fa54e

Use separate CA for ConfigMap

268f7d9

Use networking.TrustBundleLabelKey

fe0c4d2

ReToCode force-pushed the trust-bundle branch from 92f2583 to fe0c4d2 Compare December 14, 2023 13:18

Update verbosity logs in happy path

7a6e5d0

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2023

skonto reviewed Dec 14, 2023

View reviewed changes

pkg/reconciler/ingress/controller.go Show resolved Hide resolved

skonto reviewed Dec 14, 2023

View reviewed changes

pkg/generator/ingress_translator.go Show resolved Hide resolved

skonto reviewed Dec 14, 2023

View reviewed changes

pkg/reconciler/ingress/controller.go Outdated Show resolved Hide resolved

skonto reviewed Dec 14, 2023

View reviewed changes

pkg/reconciler/ingress/controller.go Outdated Show resolved Hide resolved

skonto reviewed Dec 14, 2023

View reviewed changes

pkg/generator/ingress_translator.go Outdated Show resolved Hide resolved

Review improvements

2a72a55

skonto mentioned this pull request Dec 15, 2023

[Test-only] Test kourier 1171 knative/serving#14727

Closed

skonto reviewed Dec 15, 2023

View reviewed changes

add error when trust-bundle is empty

9529993

ReToCode force-pushed the trust-bundle branch from 7ea03f4 to 9529993 Compare December 18, 2023 09:23

knative-prow bot assigned skonto Dec 18, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2023

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 18, 2023

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 19, 2023

knative-prow bot merged commit 00304db into knative-extensions:main Dec 19, 2023
41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add upstream TLS trust from CM bundles #1171

Add upstream TLS trust from CM bundles #1171

ReToCode commented Dec 6, 2023 •

edited

Loading

knative-prow bot commented Dec 6, 2023

ReToCode commented Dec 6, 2023

codecov bot commented Dec 6, 2023 •

edited

Loading

skonto commented Dec 7, 2023

ReToCode commented Dec 8, 2023

skonto commented Dec 8, 2023

ReToCode commented Dec 8, 2023

skonto commented Dec 8, 2023 •

edited

Loading

ReToCode commented Dec 14, 2023

skonto Dec 14, 2023 •

edited

Loading

skonto Dec 14, 2023

ReToCode Dec 15, 2023

ReToCode Dec 15, 2023

skonto Dec 14, 2023 •

edited

Loading

ReToCode Dec 15, 2023 •

edited

Loading

skonto Dec 15, 2023

skonto Dec 15, 2023

skonto Dec 15, 2023 •

edited

Loading

skonto commented Dec 18, 2023

skonto commented Dec 18, 2023

ReToCode commented Dec 19, 2023

	// TLS certificate data containing certificate authority certificates to use in verifying
	// a presented peer certificate (e.g. server certificate for clusters or client certificate
	// for listeners). If not specified and a peer certificate is presented it will not be
	// verified. By default, a client certificate is optional, unless one of the additional
	// options (:ref:`require_client_certificate
	// <envoy_v3_api_field_extensions.transport_sockets.tls.v3.DownstreamTlsContext.require_client_certificate>`,
	// :ref:`verify_certificate_spki
	// <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.verify_certificate_spki>`,
	// :ref:`verify_certificate_hash
	// <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.verify_certificate_hash>`, or
	// :ref:`match_typed_subject_alt_names
	// <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.match_typed_subject_alt_names>`) is also
	// specified.
	//
	// It can optionally contain certificate revocation lists, in which case Envoy will verify
	// that the presented peer certificate has not been revoked by one of the included CRLs. Note
	// that if a CRL is provided for any certificate authority in a trust chain, a CRL must be
	// provided for all certificate authorities in that chain. Failure to do so will result in
	// verification failure for both revoked and unrevoked certificates from that chain.
	// The behavior of requiring all certificates to contain CRLs can be altered by

Add upstream TLS trust from CM bundles #1171

Add upstream TLS trust from CM bundles #1171

Conversation

ReToCode commented Dec 6, 2023 • edited Loading

Changes

knative-prow bot commented Dec 6, 2023

ReToCode commented Dec 6, 2023

codecov bot commented Dec 6, 2023 • edited Loading

Codecov Report

skonto commented Dec 7, 2023

ReToCode commented Dec 8, 2023

skonto commented Dec 8, 2023

ReToCode commented Dec 8, 2023

skonto commented Dec 8, 2023 • edited Loading

ReToCode commented Dec 14, 2023

skonto Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

skonto Dec 14, 2023

Choose a reason for hiding this comment

ReToCode Dec 15, 2023

Choose a reason for hiding this comment

ReToCode Dec 15, 2023

Choose a reason for hiding this comment

skonto Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

ReToCode Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

skonto Dec 15, 2023

Choose a reason for hiding this comment

skonto Dec 15, 2023

Choose a reason for hiding this comment

skonto Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

skonto commented Dec 18, 2023

skonto commented Dec 18, 2023

ReToCode commented Dec 19, 2023

ReToCode commented Dec 6, 2023 •

edited

Loading

codecov bot commented Dec 6, 2023 •

edited

Loading

skonto commented Dec 8, 2023 •

edited

Loading

skonto Dec 14, 2023 •

edited

Loading

skonto Dec 14, 2023 •

edited

Loading

ReToCode Dec 15, 2023 •

edited

Loading

skonto Dec 15, 2023 •

edited

Loading