Eventing TLS: support `K_CA_CERTS` in adapter/v2 by pierDipi · Pull Request #6848 · knative/eventing

pierDipi · 2023-04-05T16:31:37Z

This patch adds support for the K_CA_CERTS environment variable for the source adapter library.

This will enable the APIServerSource data plane and other sources leveraging adapter/v2 to work with the Eventing TLS feature.

Fixes: #6847

codecov · 2023-04-05T16:44:32Z

Codecov Report

Patch coverage: 30.70% and project coverage change: -0.44 ⚠️

Comparison is base (d991040) 80.40% compared to head (be128a8) 79.96%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6848      +/-   ##
==========================================
- Coverage   80.40%   79.96%   -0.44%     
==========================================
  Files         236      237       +1     
  Lines       12243    12354     +111     
==========================================
+ Hits         9844     9879      +35     
- Misses       1907     1981      +74     
- Partials      492      494       +2

Impacted Files	Coverage Δ
pkg/kncloudevents/message_receiver.go	`84.44% <18.18%> (-9.39%)`	⬇️
pkg/eventingtls/eventingtls.go	`21.17% <21.17%> (ø)`
pkg/adapter/v2/cloudevents.go	`80.89% <81.25%> (+2.01%)`	⬆️
pkg/adapter/v2/config.go	`82.14% <100.00%> (+0.43%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-advanced-security · 2023-04-05T18:06:22Z

You have successfully added a new CodeQL configuration .github/workflows/knative-security.yaml:analyze-codeql. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

pierDipi · 2023-04-05T19:00:01Z

go.mod

 module knative.dev/eventing

-go 1.18
+go 1.19


This is needed to be able to use x509.CertPool.Clone()

This patch adds support for the `K_CA_CERTS` environment variable for the source adapter library. This will enable the APIServerSource data plane to work with the Eventing TLS feature. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2023-04-13T13:58:04Z

pkg/eventingtls/eventingtls.go

+	// DefaultMinTLSVersion is the default minimum TLS version for servers and clients.
+	DefaultMinTLSVersion = tls.VersionTLS12


I was wondering if we should go with 1.3?

pierDipi · 2023-04-13T13:58:10Z

/cc @matzew @creydr @aliok @evankanderson

pierDipi · 2023-04-17T14:46:59Z

pkg/eventingtls/eventingtls.go

+		if err != nil {
+			return
+		}


gab-satchi · 2023-04-17T16:29:45Z

pkg/adapter/v2/cloudevents.go

 	"github.com/cloudevents/sdk-go/v2/event"
 	"github.com/cloudevents/sdk-go/v2/protocol"
 	"github.com/cloudevents/sdk-go/v2/protocol/http"
+	cehttp "github.com/cloudevents/sdk-go/v2/protocol/http"


Nit: Is this a duplicate import? Can just use the import above

gab-satchi · 2023-04-17T17:02:10Z

pkg/eventingtls/eventingtls.go

+			UpdateFunc: func(_, newObj interface{}) {
+				store(newObj)
+			},
+			DeleteFunc: nil,


Should we clear the store on a delete? I can't think of a valid reason for a user to delete the Secret but the intent is likely to stop the usage of the cert when that happens.

I was thinking about that, however, if the secret is deleted by mistake that would cause an outage or maybe the cert management is migrated to a different tool (like Cert-Manager first and Vault after), there might be a need to delete and recreate the secret without downtime and that is hard if we stop serving the certs.

That makes sense to me

gab-satchi · 2023-04-17T17:07:41Z

pkg/eventingtls/eventingtls.go

+
+	// Clone the pool before appending other certs or returning since we don't want to add user
+	// provided CA certs for different resources to the system pool or to any other "global" pool.
+	p = p.Clone()


I think it's safe to assume it'll already be a copy. From SystemCertPool :

// SystemCertPool returns a copy of the system cert pool. // Any mutations to the returned pool are not written to disk and do not affect // any other pool returned by SystemCertPool.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2023-04-18T14:23:19Z

/test upgrade-tests

knative-prow · 2023-04-18T18:09:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gab-satchi, pierDipi

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~~ [pierDipi]

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

This patch adds support for the `K_CA_CERTS` environment variable for the source adapter library. This will enable the APIServerSource data plane and other sources leveraging adapter/v2 to work with the Eventing TLS feature. Fixes: knative#6847 --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-prow bot requested review from aliok and lionelvillard April 5, 2023 16:31

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

pierDipi changed the title ~~Eventing TLS: support K_CA_CERTS in adapter/v2~~ [WIP] Eventing TLS: support K_CA_CERTS in adapter/v2 Apr 5, 2023

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2023

pierDipi force-pushed the support-k-ca-certs-in-adapter-v2 branch 2 times, most recently from 81d5079 to e85b76a Compare April 5, 2023 17:45

pierDipi force-pushed the support-k-ca-certs-in-adapter-v2 branch from e85b76a to d4871fe Compare April 5, 2023 18:59

pierDipi commented Apr 5, 2023

View reviewed changes

pierDipi force-pushed the support-k-ca-certs-in-adapter-v2 branch from d4871fe to 284cf32 Compare April 5, 2023 22:22

pierDipi force-pushed the support-k-ca-certs-in-adapter-v2 branch from 284cf32 to b592562 Compare April 5, 2023 22:25

Bump Go to 1.19 in e2e GH actions

3c85744

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi changed the title ~~[WIP] Eventing TLS: support K_CA_CERTS in adapter/v2~~ Eventing TLS: support K_CA_CERTS in adapter/v2 Apr 11, 2023

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2023

pierDipi commented Apr 13, 2023

View reviewed changes

knative-prow bot requested review from creydr, evankanderson and matzew April 13, 2023 13:58

pierDipi commented Apr 17, 2023

View reviewed changes

pkg/eventingtls/eventingtls.go

Comment on lines +101 to +103

if err != nil {

return

}

Copy link

Member Author

pierDipi Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log error

gab-satchi reviewed Apr 17, 2023

View reviewed changes

pierDipi mentioned this pull request Apr 18, 2023

Eventing TLS: support path-based routing for MessageReceiver #6865

Closed

pierDipi added 3 commits April 18, 2023 12:59

Remove duplicate import

65cdfe2

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Remove unnecessary x509 pool clone

fbd56ca

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Log error when failing to create key pair

be128a8

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi requested a review from gab-satchi April 18, 2023 14:43

gab-satchi approved these changes Apr 18, 2023

View reviewed changes

knative-prow bot assigned gab-satchi Apr 18, 2023

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

knative-prow bot merged commit 193f2df into knative:main Apr 18, 2023

pierDipi deleted the support-k-ca-certs-in-adapter-v2 branch April 18, 2023 18:26

This was referenced Apr 20, 2023

Eventing TLS: support creating TLS server for mt-broker-ingress #6876

Closed

Eventing TLS: support creating TLS server for mt-broker-filter #6877

Closed

		// DefaultMinTLSVersion is the default minimum TLS version for servers and clients.
		DefaultMinTLSVersion = tls.VersionTLS12

Conversation

pierDipi commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-advanced-security bot commented Apr 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierDipi commented Apr 13, 2023

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!

pierDipi commented Apr 18, 2023

Uh oh!

knative-prow bot commented Apr 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pierDipi commented Apr 5, 2023 •

edited

Loading

codecov bot commented Apr 5, 2023 •

edited

Loading