Re-Enable Secure Upstream Annotation #3287

diazjf · 2018-10-24T19:30:04Z

Renables the secure-verify-ca-secret annotation. This annotation allows a CA to be verified against a service. Also adds e2e tests.

diazjf · 2018-10-24T19:30:50Z

/hold

aledbf · 2018-10-26T11:53:43Z

@diazjf please rebase to fix the e2e issue

diazjf · 2018-10-26T17:28:10Z

/hold remove

diazjf · 2018-10-26T17:28:30Z

/hold cancel

diazjf · 2018-10-26T17:28:59Z

/assign @aledbf

k8s-ci-robot · 2018-10-27T04:49:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: diazjf
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: aledbf

If they are not already assigned, you can assign the PR to them by writing /assign @aledbf in a comment when ready.

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

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

diazjf · 2018-10-31T15:34:34Z

@aledbf updated the tests. This one should be good to go.

aledbf · 2018-10-31T16:06:09Z

@diazjf please add a request and assertion/s to make sure the certificate is being used

diazjf · 2018-10-31T16:23:48Z

/hold

diazjf · 2018-10-31T16:29:06Z

@aledbf since the certificate is being verified against a service, will I need to create a service which uses a particular CA or do we have that available?

diazjf · 2018-11-02T03:56:46Z

/hold remove

diazjf · 2018-11-02T03:56:58Z

/hold cancel

diazjf · 2018-11-04T04:24:11Z

test/e2e/annotations/secureupstream.go

+			Set("Host", host).
+			End()
+		Expect(len(errs)).Should(BeNumerically("==", 0))
+		for _, pc := range resp.TLS.PeerCertificates {


@aledbf is this what you had in mind?

diazjf · 2018-11-12T19:10:02Z

@aledbf @ElvinEfendi Ready for review.

ElvinEfendi · 2018-11-13T08:15:08Z

test/e2e/annotations/secureupstream.go

+
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/backend-protocol":        "HTTPS",
+			"nginx.ingress.kubernetes.io/secure-verify-ca-secret": host,


The secret host is used for communication between client and ingress-nginx, can you use another secret here to make it realistic? (I actually expect the test to fail when you use a different secret, because the secret won't be available in the pod)

@ElvinEfendi updated the PR with your suggestions, however I'm not sure how to check that the certificate is being proxied by NGINX. I have the tests just make sure that the configuration is generated, and we trust nginx to pass it to the upstream.

ElvinEfendi · 2018-11-13T08:19:06Z

Can you add some docs? How does this work, should the user first create secret and manually mount it to ingress-nginx pods first and then configure annotation to refer to that secret?

FWIW we use:

      proxy_ssl_verify              on;
      proxy_ssl_trusted_certificate /etc/ssl/certs/ca-certificates.crt;
      proxy_ssl_verify_depth        2;

diazjf · 2018-11-14T19:27:47Z

Can you add some docs? How does this work, should the user first create secret and manually mount it to ingress-nginx pods first and then configure annotation to refer to that secret?

FWIW we use:
      proxy_ssl_verify              on;
      proxy_ssl_trusted_certificate /etc/ssl/certs/ca-certificates.crt;
      proxy_ssl_verify_depth        2;

Sure I can add some docs as well as proxy_ssl_verify_depth 2; by default. In the future I can add an annotation to configure this.

Renables the secure-verify-ca-secret annotation. This annotation allows a CA to be verified against a service. Also adds e2e tests.

diazjf · 2018-11-16T19:05:31Z

/assign @ElvinEfendi

ElvinEfendi · 2018-11-18T10:18:23Z

docs/examples/auth/client-certs/ingress2.yaml

+  annotations:
+    # Create the secret containing the trusted ca certificates
+    nginx.ingress.kubernetes.io/secure-verify-ca-secret: "ca-secret"
+    # Setup the backend protocol, must be either "HTTPS" or "GRPC".


Should this not be GRPCS instead of GRPC?

ElvinEfendi · 2018-11-18T10:18:56Z

docs/examples/auth/client-certs/ingress2.yaml

+      paths:
+      - backend:
+          serviceName: http-svc:80
+          servicePort: 80


80? not 443?

ElvinEfendi · 2018-11-18T10:21:25Z

test/e2e/annotations/secureupstream.go

+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return strings.Contains(server, proxySslTrustedCertificate) && strings.Contains(server, proxySslVerify) && strings.Contains(server, proxySslVerifyDepth)


please break this into multiple lines

ElvinEfendi · 2018-11-18T10:22:56Z

test/e2e/annotations/secureupstream.go

+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return strings.Contains(server, proxySslTrustedCertificate) && strings.Contains(server, proxySslVerify) && strings.Contains(server, proxySslVerifyDepth)
+			})


can we also send a request and assert that it is successfully proxied all the way to the echo server?

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 24, 2018

k8s-ci-robot requested review from antoineco and ElvinEfendi October 24, 2018 19:30

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 24, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2018

diazjf force-pushed the redirect-https branch 2 times, most recently from 977f055 to 8fdd346 Compare October 25, 2018 18:54

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 25, 2018

diazjf force-pushed the redirect-https branch 4 times, most recently from 06ff842 to 28da226 Compare October 26, 2018 01:23

diazjf force-pushed the redirect-https branch from 28da226 to c9e1c18 Compare October 26, 2018 16:54

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2018

k8s-ci-robot assigned aledbf Oct 26, 2018

diazjf force-pushed the redirect-https branch from c9e1c18 to 42c0858 Compare October 27, 2018 04:49

diazjf force-pushed the redirect-https branch 3 times, most recently from 71c70b6 to 9ce30cb Compare October 30, 2018 21:35

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 31, 2018

diazjf force-pushed the redirect-https branch 5 times, most recently from 2ec220c to 6815bb2 Compare November 1, 2018 22:57

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2018

diazjf commented Nov 4, 2018

View reviewed changes

diazjf force-pushed the redirect-https branch 2 times, most recently from e79bc5c to 8696e74 Compare November 12, 2018 03:42

diazjf mentioned this pull request Nov 12, 2018

Missing e2e tests for ingress annotations #3124

Closed

61 tasks

ElvinEfendi reviewed Nov 13, 2018

View reviewed changes

Re-Enable Secure Upstream Annotation

4c8f862

Renables the secure-verify-ca-secret annotation. This annotation allows a CA to be verified against a service. Also adds e2e tests.

diazjf force-pushed the redirect-https branch from 8696e74 to 4c8f862 Compare November 14, 2018 22:59

k8s-ci-robot assigned ElvinEfendi Nov 16, 2018

ElvinEfendi reviewed Nov 18, 2018

View reviewed changes

docs/examples/auth/client-certs/ingress2.yaml

paths:

- backend:

serviceName: http-svc:80

servicePort: 80

Copy link

Member

ElvinEfendi Nov 18, 2018

Choose a reason for hiding this comment

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

80? not 443?

ElvinEfendi reviewed Nov 18, 2018

View reviewed changes

diazjf closed this Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-Enable Secure Upstream Annotation #3287

Re-Enable Secure Upstream Annotation #3287

diazjf commented Oct 24, 2018

diazjf commented Oct 24, 2018

aledbf commented Oct 26, 2018

diazjf commented Oct 26, 2018

diazjf commented Oct 26, 2018

diazjf commented Oct 26, 2018

k8s-ci-robot commented Oct 27, 2018

diazjf commented Oct 31, 2018

aledbf commented Oct 31, 2018

diazjf commented Oct 31, 2018

diazjf commented Oct 31, 2018

diazjf commented Nov 2, 2018

diazjf commented Nov 2, 2018

diazjf Nov 4, 2018

diazjf commented Nov 12, 2018

ElvinEfendi Nov 13, 2018

diazjf Nov 14, 2018

ElvinEfendi commented Nov 13, 2018

diazjf commented Nov 14, 2018

diazjf commented Nov 16, 2018

ElvinEfendi Nov 18, 2018

ElvinEfendi Nov 18, 2018

ElvinEfendi Nov 18, 2018

ElvinEfendi Nov 18, 2018

Re-Enable Secure Upstream Annotation #3287

Re-Enable Secure Upstream Annotation #3287

Conversation

diazjf commented Oct 24, 2018

diazjf commented Oct 24, 2018

aledbf commented Oct 26, 2018

diazjf commented Oct 26, 2018

diazjf commented Oct 26, 2018

diazjf commented Oct 26, 2018

k8s-ci-robot commented Oct 27, 2018

diazjf commented Oct 31, 2018

aledbf commented Oct 31, 2018

diazjf commented Oct 31, 2018

diazjf commented Oct 31, 2018

diazjf commented Nov 2, 2018

diazjf commented Nov 2, 2018

diazjf Nov 4, 2018

Choose a reason for hiding this comment

diazjf commented Nov 12, 2018

ElvinEfendi Nov 13, 2018

Choose a reason for hiding this comment

diazjf Nov 14, 2018

Choose a reason for hiding this comment

ElvinEfendi commented Nov 13, 2018

diazjf commented Nov 14, 2018

diazjf commented Nov 16, 2018

ElvinEfendi Nov 18, 2018

Choose a reason for hiding this comment

ElvinEfendi Nov 18, 2018

Choose a reason for hiding this comment

ElvinEfendi Nov 18, 2018

Choose a reason for hiding this comment

ElvinEfendi Nov 18, 2018

Choose a reason for hiding this comment