Add ability to specify CA certs to use for TLS authentication. #1112

staebler · 2017-08-08T18:46:27Z

Closes #1064.

Adds insecureSkipTLSVerify and caBundle to the broker spec. The insecureSkipTLSVerify field allows the user to communicate with the broker via TLS in an insecure manner by ignoring TLS verification. The caBundle field allows the user to specify root CAs to use when verifying TLS.

Modifies ups-broker walkthrough example to support using TLS.

Updates the OSB client to include the changes from kubernetes-retired/go-open-service-broker-client#55. These changes bring support to the OSB client for skipping TLS verification and supplying root CAs.

As a consequence of doing a glide update, the github.com/golang/glog and github.com/gorilla/mux dependencies were update as well.

github.com/golang/glog changelog:

README: change code.google.com link to GitHub
Merge pull request Adding Service Catalog API Spec #13 from michael-berlin/fix_log_before_flag_parse
Fix problem that -log_dir will not be respected when anything is logged before flag.Parse().

github.com/gorilla/mux changelog:

Prefer scheme on child route when building URLs.
Use scheme from parent router when building URLs.
Fix typo
Add test and fix for escaped query values.
Update docs.
Add tests for support for queries in URL reversing.
Add support for queries in URL reversing.
Update Walking Routes Section
Fix invalid example code
Removing half of conflict marker (examine types and determine proper validation for each type #268)
Update README with example for Router.Walk
Update ancestors parameter for WalkFunc for matcher subrouters
Update Walk to match all subrouters

staebler · 2017-08-10T13:27:04Z

This is no longer in-progress.

arschles

This looks reasonable to me, but I left 1 comment on the helm chart. My apologies for the long delay in reviewing!

arschles · 2017-08-11T18:37:44Z

charts/ups-broker/templates/broker-deployment.yaml

+        - --tlsCert
+        - "{{ .Values.tls.cert }}"
+        {{- end}}
+        {{- if .Values.tls.key}}


not 100% sure on this, but iirc this will collapse --tlsKey onto the same line as --tlsCert. The solution would be to rewrite this as {{ if .Values.tls.key -}}. Same with the above if as well.

Both {{- if .Values.tls.key }} and {{ if .Values.tls.key -}} will yield the same yaml. The former will chomp the whitespace to the left, removing the newline from the previous line. The latter will chomp the whitespace to the right, removing the newline from the current line. In the end, they both will leave one newline. Using the left-side chomp seems to be a little less troublesome when the line after the if block is not at the same indentation level as the if block. With the right-side chomp, we would remove the spaces on the following line and leave the spaces on the current line, resulting in the following line being at the indentation of the current line.

For reference, the helm chart template guide [1] uses the left chomp in its example.

Here is the relevant snippet of the output from running helm install charts/ups-broker --name ups-broker --namespace ups-broker --set tls.cert=fake-cert --set tls.key=fake-key --dry-run --debug.

imagePullPolicy: Always args: - --port - "8080" - --tlsCert - "fake-cert" - --tlsKey - "fake-key" ports: - containerPort: 8080

[1] https://github.com/kubernetes/helm/blob/master/docs/chart_template_guide/control_structures.md

good catch - I stand corrected, thanks @staebler

now I know it's called chomp. that document is way too difficult to find.

arschles · 2017-08-11T18:56:38Z

@staebler looks like this needs a rebase too (just for generated code), so I've tagged as such. Can you please ping me on the Kubernetes slack (@arschles) when you're ready for a re-review? I'll be able to get to it much faster that way. Thanks.

Modify ups-broker walkthrough example to support using TLS.

pmorie · 2017-08-17T16:54:45Z

contrib/examples/walkthrough/ups-broker.yaml

@@ -4,3 +4,6 @@ metadata:
  name: ups-broker
 spec:
  url: http://ups-broker-ups-broker.ups-broker.svc.cluster.local
+  ##url: https://ups-broker-ups-broker.ups-broker.svc.cluster.local:80


You might add a comment explaining what the commented out fields are here.

pmorie

One change requested - make it so you cannot set both the insecure flag and CA bundle at the same time.

pmorie · 2017-08-17T19:39:10Z

pkg/apis/servicecatalog/v1alpha1/types.go

+	// InsecureSkipTLSVerify disables TLS certificate verification when communicating with this Broker.
+	// This is strongly discouraged.  You should use the CABundle instead.
+	// +optional
+	InsecureSkipTLSVerify bool `json:"insecureSkipTLSVerify,omitempty"`


I think there should be a validation that ensures that you cannot set InsecureSkipTLSVerify and CABundle at the same time.

Seems reasonable. Validation has been added.

… broker spec.

nilebox

LGTM, just a nit for validation error message.

nilebox · 2017-08-18T06:32:51Z

pkg/apis/servicecatalog/validation/broker.go

@@ -101,6 +101,10 @@ func validateBrokerSpec(spec *sc.BrokerSpec, fldPath *field.Path) field.ErrorLis

 	}

+	if spec.InsecureSkipTLSVerify && len(spec.CABundle) > 0 {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("CABundle"), spec.CABundle, "CABundle cannot be used when InsecureSkipTLSVerify is true"))


The JSON field name is caBundle (and the other is insecureSkipTLSVerify), so it should be:

allErrs = append(allErrs, field.Invalid(fldPath.Child("caBundle"), spec.CABundle, "caBundle cannot be used when insecureSkipTLSVerify is true"))

pmorie · 2017-08-18T17:22:45Z

THanks a bunch!

arschles · 2017-08-18T17:33:38Z

This has 2 LGTMs (I am also LGTM on this, fwiw), so I am merging

MHBauer · 2017-08-18T19:31:21Z

pkg/apis/servicecatalog/checksum/checksum_test.go

@@ -77,6 +77,8 @@ func TestBrokerSpecChecksum(t *testing.T) {
 				},
 			},
 		},
+		InsecureSkipTLSVerify: true,
+		CABundle:              []byte{13, 24, 35, 46},


Do these bytes represent something specific?

No, nothing specific. Just a test set of bytes that can test that the cheksum is created correctly.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 8, 2017

MHBauer added the in-progress label Aug 8, 2017

staebler force-pushed the 1064-BrokerTLS branch 2 times, most recently from 43e0593 to 1678a98 Compare August 10, 2017 05:51

staebler changed the title ~~(WIP) Add ability to specify CA certs to use for TLS authentication.~~ Add ability to specify CA certs to use for TLS authentication. Aug 10, 2017

jpeeler removed the in-progress label Aug 10, 2017

arschles added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2017

arschles reviewed Aug 11, 2017

View reviewed changes

Add ability to specify CA certs to use for TLS authentication.

5da8ea5

Modify ups-broker walkthrough example to support using TLS.

staebler force-pushed the 1064-BrokerTLS branch from 1678a98 to 5da8ea5 Compare August 14, 2017 14:01

pmorie removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2017

pmorie reviewed Aug 17, 2017

View reviewed changes

staebler added 2 commits August 17, 2017 16:17

Validate InsecureSkipTLSVerify and CABundle are not both present in a…

a26cdba

… broker spec.

Add comments in ups-broker.yaml explaining TLS optional values.

d959674

MHBauer self-requested a review August 18, 2017 01:27

nilebox reviewed Aug 18, 2017

View reviewed changes

nilebox added the LGTM1 label Aug 18, 2017

Fix incorrect case in btroker validation error message

a17a894

pmorie added the LGTM2 label Aug 18, 2017

arschles approved these changes Aug 18, 2017

View reviewed changes

arschles merged commit 70c2b9b into kubernetes-retired:master Aug 18, 2017

MHBauer reviewed Aug 18, 2017

View reviewed changes

staebler deleted the 1064-BrokerTLS branch August 18, 2017 19:35

pmorie mentioned this pull request Sep 1, 2017

ERROR: logging before flag.Parse in all log messages #1187

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to specify CA certs to use for TLS authentication. #1112

Add ability to specify CA certs to use for TLS authentication. #1112

staebler commented Aug 8, 2017 •

edited

Loading

staebler commented Aug 10, 2017

arschles left a comment

arschles Aug 11, 2017

staebler Aug 14, 2017

arschles Aug 18, 2017

MHBauer Aug 18, 2017

arschles commented Aug 11, 2017 •

edited

Loading

pmorie Aug 17, 2017

pmorie left a comment

pmorie Aug 17, 2017

staebler Aug 17, 2017

nilebox left a comment

nilebox Aug 18, 2017

pmorie commented Aug 18, 2017

arschles commented Aug 18, 2017

MHBauer Aug 18, 2017

staebler Aug 18, 2017

Add ability to specify CA certs to use for TLS authentication. #1112

Add ability to specify CA certs to use for TLS authentication. #1112

Conversation

staebler commented Aug 8, 2017 • edited Loading

staebler commented Aug 10, 2017

arschles left a 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

arschles commented Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

pmorie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilebox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie commented Aug 18, 2017

arschles commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staebler commented Aug 8, 2017 •

edited

Loading

arschles commented Aug 11, 2017 •

edited

Loading