Add changes to run clusterinterceptor as HTTPS #1333

savitaashture · 2022-03-24T03:48:26Z

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Triggers now support end to end secure connection by running ClusterInterceptor server as HTTPS

tekton-robot · 2022-03-24T03:50:10Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	88.7%	83.6%	-5.1
pkg/sink/initialization.go	50.0%	27.6%	-22.4

tekton-robot · 2022-03-24T04:08:44Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	88.7%	83.6%	-5.1
pkg/sink/initialization.go	50.0%	27.6%	-22.4

tekton-robot · 2022-03-24T06:39:28Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	88.7%	83.6%	-5.1
pkg/sink/initialization.go	50.0%	27.6%	-22.4

tekton-robot · 2022-03-24T07:05:42Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	88.7%	83.6%	-5.1
pkg/sink/initialization.go	50.0%	27.6%	-22.4

tekton-robot · 2022-04-04T08:51:33Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	88.7%	83.6%	-5.1
pkg/sink/initialization.go	50.0%	27.6%	-22.4

tekton-robot · 2022-04-05T10:53:39Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	88.7%	83.6%	-5.1
pkg/sink/initialization.go	50.0%	27.6%	-22.4

tekton-robot · 2022-04-06T07:18:47Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	88.7%	88.1%	-0.6
pkg/sink/initialization.go	50.0%	27.6%	-22.4

tekton-robot · 2022-04-06T07:37:00Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/interceptors/interceptors.go	88.7%	88.1%	-0.6
pkg/sink/initialization.go	50.0%	27.6%	-22.4

dibyom · 2022-04-08T17:45:15Z

cmd/interceptors/main.go

@@ -72,8 +89,62 @@ func main() {
 	mux.Handle("/", service)
 	mux.HandleFunc("/ready", handler)

+	name := os.Getenv("SVC_NAME")


IMO we should give this a more explicit name than SVC name e.g. INTERCEPTOR_TLS_SECRET or something like that

cmd/interceptors/main.go

dibyom · 2022-04-08T17:49:17Z

config/200-clusterrole.yaml

@@ -65,7 +65,7 @@ metadata:
 rules:
  - apiGroups: [""]
    resources: ["secrets"]
-    verbs: ["get", "list", "watch"]
+    verbs: ["get", "list", "watch", "update"]


I don't think we should change the role to be able to update all secrets. Instead create another rule that can update the specific secret that we need

I see tekton-triggers-core-interceptors cluster role is only used for tekton-triggers-core-interceptors deployment so i believe this is for specified one only

Let me know if this is your concern or something else

Right the concern is that this allows the core interceptor server access to update any secret in the cluster which is a bit scary. What we should do is to make sure it can only update the TLS secret for the cluster interceptor.
See the example here where the webhook role can only update the webhook-certs secret using the resourceName field: https://github.com/tektoncd/pipeline/blob/main/config/200-role.yaml#L59-L65

config/interceptors/interceptor-secrets.yaml

dibyom · 2022-04-08T17:59:28Z

pkg/apis/triggers/v1alpha1/cluster_interceptor_types.go

 	if svc.Port != nil {
 		port = *svc.Port
 	}
 	url := &apis.URL{
-		Scheme: "http", // TODO: Support HTTPs if caBundle is present
+		Scheme: "https",


hmm..we'd have to be a bit careful here...if we move this to only support https, anyone who has written a cluster interceptor will not be able use Triggers until they upgrade their code. Should we support both for a release or two?

Ah i see

Okay so do you mean we need to have support for both HTTP and HTTPS right/

If thats the case we need to take input from deplpyment yaml to know whether they want HTTP or HTTPS
Shall i change implementation that way ?

we discussed this in the WG - we should take the input for whether it is HTTP/HTTPS from interceptor CRD itself.

savitaashture · 2022-04-27T16:14:38Z

cmd/interceptors/main.go

+		createCerts = true
+	}
+
+	// TODO: Certification validation and rotation is pending


@dibyom will create an issue to handle it in separate PR

could you open an issue to track?

tekton-robot · 2022-04-27T16:15:29Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults.go	100.0%	N/A	-100.0
pkg/interceptors/interceptors.go	88.7%	87.1%	-1.6

dibyom

Almost there - I do think we need to modify how we are creating the http clients else we will break timeouts

dibyom · 2022-04-27T22:49:05Z

pkg/interceptors/interceptors.go

+			return nil, fmt.Errorf("unable to parse cert from %s", ic.Spec.ClientConfig.CaBundle)
+		}
+
+		client = &http.Client{


I think we have to update this - creating a new client here means we will ignore all of the config options that we set in the already passed in HTTP client (such as the timeout values). Creating a new HTTP client per call means we can't reuse the underlying connections so we might be limiting the number of calls from the EL to the interceptor.

I think we'd have to add in the certs in the sinker.Start function - we could pass in a clusterinterceptorlister as an arg to the sinker struct - list all the clusterinterceptors, get the caCert and add it to the certPool. and then create a httpClient with all the certs. (This does mean that if we add a new interceptor in the middle, we'd have to restart each EL.

(Ideally in the future we'd have something that listens for changes to interceptors and updates the httpClient with newCerts on the fly)

@dibyom Yes i agree with you
It overrides previous given values

Updated code PTAL 🙏
Thank you

dibyom · 2022-04-27T22:52:39Z

cmd/interceptors/main.go

+		createCerts = true
+	}
+
+	// TODO: Certification validation and rotation is pending


could you open an issue to track?

dibyom · 2022-04-27T22:53:56Z

cmd/interceptors/main.go

+			log.Printf("secret %s is missing", interceptorSecretName)
+			return "", "", []byte{}, err
+		}
+		log.Printf("error accessing certificate secret %q: %v", interceptorSecretName, err)


can we use logger as we do in the rest of the function vs log.Printf

tekton-robot · 2022-04-29T15:50:00Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults.go	100.0%	N/A	-100.0

tekton-robot · 2022-04-29T17:48:17Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults.go	100.0%	N/A	-100.0

dibyom · 2022-04-29T20:19:33Z

cmd/interceptors/main.go

+	}
+	// write serverCert to file so that it can be passed while running https server.
+	// Expect WriteFile permissions to be 0600 or less (gosec)
+	//nolint:gosec


Hmm why do we have 0600?

if given file permission is more than 600 the its giving security issue during build

cmd/interceptors/main.go:211:11: G306: Expect WriteFile permissions to be 0600 or less (gosec) if err = ioutil.WriteFile(certFile, serverCert, 0644); err != nil { ^

For now instead of using 644 i m using 600 and removing //nolint:gosec
Updated main.go

Thanks for pointing it out 👍 I forgot to remove it earlier

dibyom

Just one question, else LGTM!

tekton-robot · 2022-05-01T14:09:35Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults.go	100.0%	N/A	-100.0

tekton-robot · 2022-05-01T14:37:50Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults.go	100.0%	N/A	-100.0

savitaashture · 2022-05-02T03:27:19Z

/test pull-tekton-triggers-build-tests

tekton-robot · 2022-05-02T05:20:18Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/cluster_interceptor_defaults.go	100.0%	N/A	-100.0

savitaashture · 2022-05-02T05:57:26Z

/test pull-tekton-triggers-integration-tests

savitaashture · 2022-05-02T13:48:42Z

@dibyom PR is ready for final review

Thank You!!

tekton-robot · 2022-05-02T20:11:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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

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

dibyom · 2022-05-02T20:12:17Z

/lgtm

savitaashture requested review from khrm and dibyom March 24, 2022 03:48

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 24, 2022

tekton-robot requested a review from vtereso March 24, 2022 03:48

savitaashture added kind/feature Categorizes issue or PR as related to a new feature. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 24, 2022

savitaashture force-pushed the https-ci-changes branch from ec3c188 to 443197b Compare March 24, 2022 04:07

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 24, 2022

savitaashture force-pushed the https-ci-changes branch from 443197b to 5606bff Compare March 24, 2022 06:37

savitaashture force-pushed the https-ci-changes branch from 5606bff to a306cc4 Compare March 24, 2022 07:03

savitaashture force-pushed the https-ci-changes branch from a306cc4 to 666b2b6 Compare April 4, 2022 08:50

savitaashture force-pushed the https-ci-changes branch from 666b2b6 to e117c5b Compare April 5, 2022 10:52

savitaashture force-pushed the https-ci-changes branch from e117c5b to a210326 Compare April 6, 2022 07:16

savitaashture force-pushed the https-ci-changes branch from a210326 to 25003f7 Compare April 6, 2022 07:33

dibyom mentioned this pull request Apr 8, 2022

[TEP-0102] HTTPS Connection to Triggers ClusterInterceptor tektoncd/community#662

Merged

dibyom reviewed Apr 8, 2022

View reviewed changes

cmd/interceptors/main.go Show resolved Hide resolved

dibyom reviewed Apr 8, 2022

View reviewed changes

cmd/interceptors/main.go Outdated Show resolved Hide resolved

dibyom reviewed Apr 8, 2022

View reviewed changes

config/interceptors/interceptor-secrets.yaml Outdated Show resolved Hide resolved

dibyom reviewed Apr 8, 2022

View reviewed changes

dibyom linked an issue Apr 27, 2022 that may be closed by this pull request

Allow TLS connections between EventListener and Interceptors #871

Closed

savitaashture force-pushed the https-ci-changes branch from d7a6804 to e6d8bfd Compare April 27, 2022 16:13

savitaashture commented Apr 27, 2022

View reviewed changes

dibyom reviewed Apr 27, 2022

View reviewed changes

This was referenced Apr 28, 2022

Handle validation and rotation of certificate for clusterinterceptor #1354

Closed

Handle validation and rotation of certificate for clusterinterceptor #1355

Closed

savitaashture force-pushed the https-ci-changes branch from e6d8bfd to 4f20fd2 Compare April 29, 2022 15:48

savitaashture force-pushed the https-ci-changes branch from 4f20fd2 to 795717f Compare April 29, 2022 17:46

dibyom reviewed Apr 29, 2022

View reviewed changes

dibyom approved these changes Apr 29, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2022

savitaashture force-pushed the https-ci-changes branch from 795717f to d73798a Compare May 1, 2022 14:07

savitaashture force-pushed the https-ci-changes branch from d73798a to e21cb18 Compare May 1, 2022 14:36

Add changes to run clusterinterceptor as HTTPS

47c6a51

savitaashture force-pushed the https-ci-changes branch from e21cb18 to 47c6a51 Compare May 2, 2022 05:18

dibyom approved these changes May 2, 2022

View reviewed changes

tekton-robot assigned dibyom May 2, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2022

tekton-robot merged commit 28d7b73 into tektoncd:main May 2, 2022

Add changes to run clusterinterceptor as HTTPS #1333

Add changes to run clusterinterceptor as HTTPS #1333

Conversation

savitaashture commented Mar 24, 2022

Changes

Submitter Checklist

Release Notes

tekton-robot commented Mar 24, 2022

tekton-robot commented Mar 24, 2022

tekton-robot commented Mar 24, 2022

tekton-robot commented Mar 24, 2022

tekton-robot commented Apr 4, 2022

tekton-robot commented Apr 5, 2022

tekton-robot commented Apr 6, 2022

tekton-robot commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savitaashture Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Apr 27, 2022

dibyom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savitaashture Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Apr 29, 2022

tekton-robot commented Apr 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom left a comment

Choose a reason for hiding this comment

tekton-robot commented May 1, 2022

tekton-robot commented May 1, 2022

savitaashture commented May 2, 2022

tekton-robot commented May 2, 2022

savitaashture commented May 2, 2022

savitaashture commented May 2, 2022

tekton-robot commented May 2, 2022

dibyom commented May 2, 2022

savitaashture Apr 11, 2022 •

edited

Loading

savitaashture Apr 29, 2022 •

edited

Loading