allow self signed certs with insecureSkipVerify #1769

s12chung · 2019-08-16T20:18:04Z

adds insecureSkipVerify server config for AWS storage and --insecureSkipVerify flag on client for self-signed certs

if needed, let me know how to add a test. I've tested manually and we're using a variant of the code as a team. most of the code is simply passing along the insecureSkipVerify flag around until InsecureSkipVerify is set on the http client.

closes #1027

you'll get this error otherwise: x509: certificate signed by unknown authority Signed-off-by: Steven Chung <[email protected]>

nrb · 2019-08-16T21:19:30Z

Thanks @s12chung! I'm curious, what object storage system are you using with self-signed certs?

s12chung · 2019-08-16T22:01:10Z

happy to contribute :) we're using minio

Signed-off-by: Steven Chung <[email protected]>

carlisia

Some comments wrt documenting the new flag.

carlisia · 2019-08-19T23:10:03Z

pkg/cmd/cli/backup/describe.go

@@ -85,6 +86,7 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command {

 	c.Flags().StringVarP(&listOptions.LabelSelector, "selector", "l", listOptions.LabelSelector, "only show items matching this label selector")
 	c.Flags().BoolVar(&details, "details", details, "display additional detail in the command output")
+	c.Flags().BoolVar(&insecureSkipVerify, "insecureskipverify", insecureSkipVerify, "accept any TLS certificate presented by the storage service")


Would it be good to add something like not secure, or not recommended for production to all the cli documentation?

do not verify the TLS certificate for storage requests. This is susceptible to man-in-the-middle attacks. -- think man-in-the-middle attacks says it's dangerous. since this is the client and the flag is non-permanent, I kept it shorter.

carlisia · 2019-08-19T23:13:16Z

site/docs/master/api-types/backupstoragelocation.md

@@ -53,6 +53,7 @@ The configurable parameters are as follows:
 | `kmsKeyId` | string | Empty | *Example*: "502b409c-4da1-419f-a16e-eif453b3i49f" or "alias/`<KMS-Key-Alias-Name>`"<br><br>Specify an [AWS KMS key][10] id or alias to enable encryption of the backups stored in S3. Only works with AWS S3 and may require explicitly granting key usage rights.|
 | `signatureVersion` | string | `"4"` | Version of the signature algorithm used to create signed URLs that are used by velero cli to download backups or fetch logs. Possible versions are "1" and "4". Usually the default version 4 is correct, but some S3-compatible providers like Quobyte only support version 1.|
 | `profile` | string | "default" | AWS profile within the credential file to use for given store |
+| `insecureSkipVerify` | bool | `false` | Set this to `true` if you do not want to verify the CA certificate for storage requests--like self-signed certs in Minio. |


Please add text warning that this is unsafe.

Set this to true if you do not want to verify the TLS certificate for storage requests--like self-signed certs in Minio. This is susceptible to man-in-the-middle attacks and is not recommended for production.

Signed-off-by: Steven Chung <[email protected]>

prydonius

lgtm mostly!

prydonius · 2019-08-20T01:13:51Z

pkg/cmd/cli/backup/describe.go

@@ -85,6 +86,7 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command {

 	c.Flags().StringVarP(&listOptions.LabelSelector, "selector", "l", listOptions.LabelSelector, "only show items matching this label selector")
 	c.Flags().BoolVar(&details, "details", details, "display additional detail in the command output")
+	c.Flags().BoolVar(&insecureSkipVerify, "insecureskipverify", insecureSkipVerify, "do not verify the TLS certificate for storage requests. This is susceptible to man-in-the-middle attacks.")


Any reason to not have this be insecure-skip-verify? It might also be a good idea to use the same flag kubectl uses: insecure-skip-tls-verify.

Good point 👍

good idea. changed it all to insecure-skip-tls-verify

prydonius · 2019-08-20T01:17:20Z

pkg/cmd/cli/backup/describe.go

@@ -85,6 +86,7 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command {

 	c.Flags().StringVarP(&listOptions.LabelSelector, "selector", "l", listOptions.LabelSelector, "only show items matching this label selector")
 	c.Flags().BoolVar(&details, "details", details, "display additional detail in the command output")
+	c.Flags().BoolVar(&insecureSkipVerify, "insecureskipverify", insecureSkipVerify, "do not verify the TLS certificate for storage requests. This is susceptible to man-in-the-middle attacks.")


We should also probably clarify here that this flag is for storage bucket requests and not requests to the Kubernetes API server

hmm... it already says do not verify the TLS certificate for storage requests

so I only to the end of that: do not verify the TLS certificate for storage requests only

sorry I misread that, you're right that it was good as it was, but the change doesn't hurt, thanks!

Signed-off-by: Steven Chung <[email protected]>

prydonius

lgtm!

prydonius · 2019-08-20T17:28:32Z

@carlisia @skriss @nrb I assume we want to hold this back until 1.2?

carlisia

👍

This reverts commit 8e35ce0.

…#1769)" This reverts commit 8e35ce0. Signed-off-by: Carlisia <[email protected]>

skriss · 2019-08-20T18:13:58Z

we need to discuss further if we want to make this option available to folks. the better change would be to support custom CA bundles per #1027 (comment). @s12chung is that an option for you?

Regardless, we're not going to include this in v1.1 so we're backing out the merge with #1776.

carlisia · 2019-08-20T18:26:37Z

My bad for merging!

s12chung · 2019-08-21T01:26:28Z

@skriss then both the server and client would have to retrieve the certs in order to do operations. am I correct?

hmm.... would need to talk to product owners about this. and need to play around with the code of course. would it be ok to keep insecureSkipVerify as an option for now?

* upstream/master: (118 commits) restore: rename PV when remapping a namespace if PV exists in-cluster (vmware-tanzu#1779) when backing up PVCs with restic, explicitly specify --parent (vmware-tanzu#1807) Unit tests for restic restore (vmware-tanzu#1747) Upgrade kubernetes dependencies to 1.15.3 (vmware-tanzu#1808) create backups from schedules using velero create backup (vmware-tanzu#1734) remove calls to restic check before/after prune (vmware-tanzu#1794) Propose adding feature flags to velero restic backup and restore progress proposal (vmware-tanzu#1765) allow custom restic repo prefix to be specified in BSL config error if restic repo identifier can't be determined update nokogiri dep for website update links on website home page for latest release (vmware-tanzu#1789) Velero 1.1 blog post v1.1.0 changelog fix error formatting Revert "allow self signed certs with insecureSkipVerify (vmware-tanzu#1769)" allow self signed certs with insecureSkipVerify (vmware-tanzu#1769) v1.1.0 docs Add the prefix to BSL config map so that object stores can use it when initializing (vmware-tanzu#1767) add stable/velero to helm commands ...

* jesse/20190828_merge: (511 commits) fix(ci): Update arm32 target. feat(ci): Auto-build restic-restore-helper image in CI. restore: rename PV when remapping a namespace if PV exists in-cluster (vmware-tanzu#1779) when backing up PVCs with restic, explicitly specify --parent (vmware-tanzu#1807) Unit tests for restic restore (vmware-tanzu#1747) Upgrade kubernetes dependencies to 1.15.3 (vmware-tanzu#1808) create backups from schedules using velero create backup (vmware-tanzu#1734) remove calls to restic check before/after prune (vmware-tanzu#1794) Propose adding feature flags to velero restic backup and restore progress proposal (vmware-tanzu#1765) allow custom restic repo prefix to be specified in BSL config error if restic repo identifier can't be determined update nokogiri dep for website update links on website home page for latest release (vmware-tanzu#1789) Velero 1.1 blog post v1.1.0 changelog fix error formatting Revert "allow self signed certs with insecureSkipVerify (vmware-tanzu#1769)" allow self signed certs with insecureSkipVerify (vmware-tanzu#1769) v1.1.0 docs ...

macevil · 2020-03-05T21:08:05Z

I thought that skipping is merged into the master, but I still get this error:

velero install --provider aws --bucket velero-2c2fe3cc --secret-file ./credentials-velero --use-volume-snapshots=false --backup-location-config region=default,s3ForcePathStyle="true",s3Url=https://os-s3.domain.de --plugins velero/velero-plugin-for-aws:v1.0.0 --image velero/velero:v1.3.0 --insecure-skip-tls-verify
Error: unknown flag: --insecure-skip-tls-verify

Any ideas?

skriss · 2020-03-05T21:15:00Z

@macevil for the server, this is a config option on BackupStorageLocations, so you should append ,insecureSkipTLSVerify="true" to the value of your --backup-location-config flag on the velero install command. If you run velero backup/restore logs, velero backup/restore describe --details or velero backup download, you can use the --insecure-skip-tls-verify flag on those commands.

macevil · 2020-03-05T21:27:55Z

@skriss Oh, thanks, I was on the hose. 🤦🏻‍♂️

Steven Chung added 2 commits August 16, 2019 16:19

patch velero to handle self-signed certs on client

dc9549c

you'll get this error otherwise: x509: certificate signed by unknown authority Signed-off-by: Steven Chung <[email protected]>

patch velero to always handle self-signed certs on server

95c0fe3

you'll get this error otherwise: x509: certificate signed by unknown authority Signed-off-by: Steven Chung <[email protected]>

s12chung force-pushed the ssl branch from bf593de to 7092373 Compare August 16, 2019 20:24

add changelog for insecureSkipVerify

7d1717e

Signed-off-by: Steven Chung <[email protected]>

s12chung force-pushed the ssl branch from 7092373 to 7d1717e Compare August 16, 2019 22:03

carlisia reviewed Aug 19, 2019

View reviewed changes

add more serious warnings to insecureSkipVerify text

f44caef

Signed-off-by: Steven Chung <[email protected]>

prydonius reviewed Aug 20, 2019

View reviewed changes

Steven Chung added 2 commits August 20, 2019 11:08

rename insecureSkipVerify to insecureSkipTLSVerify

bff78a2

Signed-off-by: Steven Chung <[email protected]>

make insecureSkipVerify messages more specific to storage requests

7736632

Signed-off-by: Steven Chung <[email protected]>

prydonius approved these changes Aug 20, 2019

View reviewed changes

carlisia approved these changes Aug 20, 2019

View reviewed changes

carlisia merged commit 8e35ce0 into vmware-tanzu:master Aug 20, 2019

carlisia added a commit that referenced this pull request Aug 20, 2019

Revert "allow self signed certs with insecureSkipVerify (#1769)"

7bf4830

This reverts commit 8e35ce0.

carlisia mentioned this pull request Aug 20, 2019

Revert "allow self signed certs with insecureSkipVerify" #1776

Merged

carlisia added a commit to carlisia/velero that referenced this pull request Aug 20, 2019

Revert "allow self signed certs with insecureSkipVerify (vmware-tanzu…

b25865f

…#1769)" This reverts commit 8e35ce0. Signed-off-by: Carlisia <[email protected]>

s12chung mentioned this pull request Aug 22, 2019

Revert "Revert "allow self signed certs with insecureSkipVerify"" #1793

Merged

This was referenced Mar 11, 2020

Unable to set insecureSkipTLSVerify for backupStorageLocation vmware-tanzu/helm-charts#77

Closed

adding insecureSkipTLSVerify vmware-tanzu/helm-charts#78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow self signed certs with insecureSkipVerify #1769

allow self signed certs with insecureSkipVerify #1769

s12chung commented Aug 16, 2019 •

edited

Loading

nrb commented Aug 16, 2019

s12chung commented Aug 16, 2019

carlisia left a comment

carlisia Aug 19, 2019

s12chung Aug 19, 2019

carlisia Aug 19, 2019

s12chung Aug 19, 2019

prydonius left a comment

prydonius Aug 20, 2019

carlisia Aug 20, 2019

s12chung Aug 20, 2019

prydonius Aug 20, 2019

s12chung Aug 20, 2019 •

edited

Loading

prydonius Aug 20, 2019

prydonius left a comment

prydonius commented Aug 20, 2019

carlisia left a comment

skriss commented Aug 20, 2019

carlisia commented Aug 20, 2019

s12chung commented Aug 21, 2019

macevil commented Mar 5, 2020

skriss commented Mar 5, 2020

macevil commented Mar 5, 2020

allow self signed certs with insecureSkipVerify #1769

allow self signed certs with insecureSkipVerify #1769

Conversation

s12chung commented Aug 16, 2019 • edited Loading

nrb commented Aug 16, 2019

s12chung commented Aug 16, 2019

carlisia 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

prydonius 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

s12chung Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius left a comment

Choose a reason for hiding this comment

prydonius commented Aug 20, 2019

carlisia left a comment

Choose a reason for hiding this comment

skriss commented Aug 20, 2019

carlisia commented Aug 20, 2019

s12chung commented Aug 21, 2019

macevil commented Mar 5, 2020

skriss commented Mar 5, 2020

macevil commented Mar 5, 2020

s12chung commented Aug 16, 2019 •

edited

Loading

s12chung Aug 20, 2019 •

edited

Loading