Add TLS support for backup-restore server #196

shreyas-s-rao · 2019-09-03T06:19:15Z

Signed-off-by: Shreyas Rao [email protected]

What this PR does / why we need it:
This PR introduces TLS support for the etcd-backup-restore server. TLS can be enabled by passing the paths to both TLS cert and key PEM-format files via --server-cert and --server-key flags. This PR also introduces security headers HTTP-Strict-Transport-Security and Content-Security-Policy for the HTTP responses when TLS is enabled.

Which issue(s) this PR fixes:
Fixes #189 #187

Special notes for your reviewer:
Renamed tls section to etcdTLS in helm values file, to differentiate between etcd tls config and etcdbr server tls config. Bootstrap script updated to use apk's wget instead of busybox wget in the case where backup-restore TLS is enabled.

Release note:

Added TLS support for backup-restore server. TLS can be enabled by passing the paths to both TLS cert and key PEM-format files via `--server-cert` and `--server-key` flags.

swapnilgm

Overall LGTM. Some minor changes. I've test it out yet.

swapnilgm · 2019-09-03T20:25:01Z

cmd/server.go

+		// Check for existence of server cert and key files before proceeding
+		if serverTLSCertFile == "" || serverTLSKeyFile == "" {
+			return nil, fmt.Errorf("TLS enabled but server TLS cert/key file not provided. Will not start HTTPS server")
+		} else if _, err := os.Stat(serverTLSCertFile); os.IsNotExist(err) {


Unnecessary else.

Please Return err other than os.IsNotExist

swapnilgm · 2019-09-03T20:25:16Z

cmd/server.go

+			return nil, fmt.Errorf("TLS enabled but server TLS cert/key file not provided. Will not start HTTPS server")
+		} else if _, err := os.Stat(serverTLSCertFile); os.IsNotExist(err) {
+			return nil, fmt.Errorf("TLS enabled but server TLS cert file is invalid. Will not start HTTPS server")
+		} else if _, err := os.Stat(serverTLSKeyFile); os.IsNotExist(err) {


Same as above.

swapnilgm

I had tested it out. But wget request was failing for me with ca verification issue. @georgekuruvillak will you please review and test this PR.

swapnilgm · 2019-09-11T11:40:38Z

chart/etcd-backup-restore/values.yaml

    pullPolicy: IfNotPresent
  # etcd-backup-restore image to use
  etcdBackupRestore:
-    repository: eu.gcr.io/gardener-project/gardener/etcdbrctl
-    tag: 0.7.0
+    repository:  eu.gcr.io/gardener-project/gardener/etcdbrctl


Suggested change

repository: eu.gcr.io/gardener-project/gardener/etcdbrctl

repository: eu.gcr.io/gardener-project/gardener/etcdbrctl

swapnilgm · 2019-09-12T11:51:29Z

chart/etcd-backup-restore/templates/etcd-bootstrap-configmap.yaml

            STATUS=`cat status`;
            case $STATUS in
            "New")
-                  wget "http://localhost:{{ .Values.servicePorts.backupRestore }}/initialization/start?mode=$1{{- if .Values.backup.failBelowRevision }}&failbelowrevision={{ int $.Values.backup.failBelowRevision }}{{- end }}" -S -O - ;;
+                  wget {{ if .Values.backupRestoreTls }}--ca-certificate /var/etcdbr/tls/ca/ca.crt "https{{ else }}"http{{ end }}://localhost:{{ .Values.servicePorts.backupRestore }}/initialization/start?mode=$1{{- if .Values.backup.failBelowRevision }}&failbelowrevision={{ int $.Values.backup.failBelowRevision }}{{- end }}" -S -O - ;;


Won't you require to pass client certificate and key here?

If not authenticating client then client certificate and key neednt be passed.

georgekuruvillak · 2019-09-16T05:05:19Z

chart/etcd-backup-restore/templates/etcd-bootstrap-configmap.yaml

            STATUS=`cat status`;
            case $STATUS in
            "New")
-                  wget "http://localhost:{{ .Values.servicePorts.backupRestore }}/initialization/start?mode=$1{{- if .Values.backup.failBelowRevision }}&failbelowrevision={{ int $.Values.backup.failBelowRevision }}{{- end }}" -S -O - ;;
+                  wget {{ if .Values.backupRestoreTls }}--ca-certificate /var/etcdbr/tls/ca/ca.crt "https{{ else }}"http{{ end }}://localhost:{{ .Values.servicePorts.backupRestore }}/initialization/start?mode=$1{{- if .Values.backup.failBelowRevision }}&failbelowrevision={{ int $.Values.backup.failBelowRevision }}{{- end }}" -S -O - ;;


I guess the naming convention requires that you change backupRestoreTls to backupRestoreTLS to pass linting.

georgekuruvillak · 2019-09-16T05:06:44Z

chart/etcd-backup-restore/templates/etcd-bootstrap-configmap.yaml

@@ -91,19 +98,19 @@ data:
    {{- end }}

    # List of comma separated URLs to listen on for client traffic.
-    listen-client-urls: {{ if .Values.tls }}https{{ else }}http{{ end }}://0.0.0.0:{{ .Values.servicePorts.client }}
+    listen-client-urls: {{ if .Values.etcdTls }}https{{ else }}http{{ end }}://0.0.0.0:{{ .Values.servicePorts.client }}


etcdTLS instead of etcdTls. Similar change needed everywhere else too.

georgekuruvillak

Please document the steps to create the certificate, csr and key using some cert generation tool such as openssl.

shreyas-s-rao · 2019-10-16T08:33:57Z

@swapnilgm @georgekuruvillak thanks for your reviews. I have addressed your comments and updated the PR accordingly. PTAL.

swapnilgm

Thanks for adding documentation. Overall LGTM. Some minor changes suggested. PTAL.

cmd/server.go

doc/usage/generating_ssl_certificates.md

shreyas-s-rao · 2019-10-22T03:49:09Z

@swapnilgm Thanks for your review. I have addressed the comments. @georgekuruvillak PTAL. Thanks.

swapnilgm

LGTM

georgekuruvillak

Well written PR.

doc/usage/generating_ssl_certificates.md

Signed-off-by: Shreyas Rao <[email protected]>

shreyas-s-rao added this to the 0.8.0 milestone Sep 3, 2019

shreyas-s-rao requested review from georgekuruvillak and swapnilgm as code owners September 3, 2019 06:19

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 3, 2019

swapnilgm suggested changes Sep 5, 2019

View reviewed changes

swapnilgm added the priority/normal label Sep 11, 2019

swapnilgm reviewed Sep 12, 2019

View reviewed changes

swapnilgm assigned georgekuruvillak Sep 12, 2019

georgekuruvillak reviewed Sep 16, 2019

View reviewed changes

georgekuruvillak suggested changes Sep 17, 2019

View reviewed changes

shreyas-s-rao force-pushed the add-tls-support branch from 63b25a7 to 0bcbab1 Compare October 16, 2019 08:14

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 16, 2019

shreyas-s-rao force-pushed the add-tls-support branch from 0bcbab1 to 1255967 Compare October 16, 2019 08:32

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 16, 2019

shreyas-s-rao force-pushed the add-tls-support branch from 1255967 to 561ca73 Compare October 16, 2019 08:37

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 16, 2019

shreyas-s-rao force-pushed the add-tls-support branch from 561ca73 to d58a28e Compare October 16, 2019 08:44

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 16, 2019

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 16, 2019

swapnilgm suggested changes Oct 21, 2019

View reviewed changes

cmd/server.go Outdated Show resolved Hide resolved

cmd/server.go Outdated Show resolved Hide resolved

doc/usage/generating_ssl_certificates.md Outdated Show resolved Hide resolved

shreyas-s-rao force-pushed the add-tls-support branch from d58a28e to 9f931b2 Compare October 22, 2019 03:48

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 22, 2019

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 22, 2019

swapnilgm approved these changes Oct 22, 2019

View reviewed changes

georgekuruvillak approved these changes Oct 22, 2019

View reviewed changes

swapnilgm reviewed Oct 22, 2019

View reviewed changes

doc/usage/generating_ssl_certificates.md Outdated Show resolved Hide resolved

Add TLS support for backup-restore server

9df523c

Signed-off-by: Shreyas Rao <[email protected]>

shreyas-s-rao force-pushed the add-tls-support branch from 9f931b2 to 9df523c Compare October 22, 2019 09:51

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 22, 2019

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 22, 2019

swapnilgm merged commit dd26b65 into gardener:master Oct 22, 2019

swapnilgm mentioned this pull request Oct 22, 2019

Static Code Analysis #187

Closed

shreyas-s-rao deleted the add-tls-support branch October 23, 2019 04:38

swapnilgm mentioned this pull request Dec 20, 2019

Static code analysis #206

Closed

gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/3 Priority (lower number equals higher priority) labels Mar 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TLS support for backup-restore server #196

Add TLS support for backup-restore server #196

shreyas-s-rao commented Sep 3, 2019 •

edited

Loading

swapnilgm left a comment

swapnilgm Sep 3, 2019

swapnilgm Sep 3, 2019

swapnilgm left a comment

swapnilgm Sep 11, 2019

swapnilgm Sep 12, 2019

georgekuruvillak Sep 17, 2019

georgekuruvillak Sep 16, 2019 •

edited

Loading

georgekuruvillak Sep 16, 2019 •

edited

Loading

georgekuruvillak left a comment

shreyas-s-rao commented Oct 16, 2019

swapnilgm left a comment •

edited

Loading

shreyas-s-rao commented Oct 22, 2019

swapnilgm left a comment

georgekuruvillak left a comment

	repository: eu.gcr.io/gardener-project/gardener/etcdbrctl
	repository: eu.gcr.io/gardener-project/gardener/etcdbrctl

Add TLS support for backup-restore server #196

Add TLS support for backup-restore server #196

Conversation

shreyas-s-rao commented Sep 3, 2019 • edited Loading

swapnilgm left a comment

Choose a reason for hiding this comment

swapnilgm Sep 3, 2019

Choose a reason for hiding this comment

swapnilgm Sep 3, 2019

Choose a reason for hiding this comment

swapnilgm left a comment

Choose a reason for hiding this comment

swapnilgm Sep 11, 2019

Choose a reason for hiding this comment

swapnilgm Sep 12, 2019

Choose a reason for hiding this comment

georgekuruvillak Sep 17, 2019

Choose a reason for hiding this comment

georgekuruvillak Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

georgekuruvillak Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

georgekuruvillak left a comment

Choose a reason for hiding this comment

shreyas-s-rao commented Oct 16, 2019

swapnilgm left a comment • edited Loading

Choose a reason for hiding this comment

shreyas-s-rao commented Oct 22, 2019

swapnilgm left a comment

Choose a reason for hiding this comment

georgekuruvillak left a comment

Choose a reason for hiding this comment

shreyas-s-rao commented Sep 3, 2019 •

edited

Loading

georgekuruvillak Sep 16, 2019 •

edited

Loading

georgekuruvillak Sep 16, 2019 •

edited

Loading

swapnilgm left a comment •

edited

Loading