Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

s12chung
Copy link
Contributor

Reverts #1776, which is a revert of #1769

I'd like to bring this code up, as it'd be great if we could have this feature. I don't think I can put in the time to support support custom CA bundles per #1027 (comment) now. I understand that insecureSkipVerify is insecure--hence the warnings. If I can get the code in, awesome. Otherwise, our team will live with our fork until we move away from self-signed certs.

Can we discuss about this topic on this PR?

relates to #1027

@skriss
Copy link
Contributor

skriss commented Sep 24, 2019

@s12chung based on all of the discussions, we've decided that we'll go ahead with getting this PR merged. I'll take a look at the code later today and provide any feedback. We may want to tweak the messages/documentation to make it super-clear that this is an insecure option.

cc @nrb @carlisia @prydonius

@prydonius
Copy link
Contributor

We talked about potentially removing the insecure option from the server, and only allowing insecure connections from the client. This way, you would have to use a self-signed cert for the server -> object store connection, but in the client you can use --insecure-skip-tls-verify for debugging purposes. Would that work for you @s12chung?

@s12chung
Copy link
Contributor Author

@prydonius I believe the server would need access to a secret that contains the cert. I'm good with that :) I just don't have the time to implement/test it on our end right now.

@prydonius
Copy link
Contributor

prydonius commented Sep 24, 2019

@s12chung yeah, we would need a separate PR to accept a self-signed cert and add that to the certificate chain. In this PR, we should just remove the insecure option for the server for now. Good to hear that would work for you, I think that will mitigate the attack surface of this change.

@s12chung
Copy link
Contributor Author

@prydonius without the insecure option on the server, I think the server would run into the same error the client does. unless

I believe the server would need access to a secret that contains the cert.

Copy link
Contributor

@skriss skriss left a comment

Choose a reason for hiding this comment

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

The plan for this is:

@carlisia, since this is also touching the AWS cloud provider code, need your input on whether we can merge this into master now-ish, or whether we need to split out the AWS-specific pieces into a separate PR for the new repo.

pkg/cmd/cli/backup/describe.go Outdated Show resolved Hide resolved
pkg/cmd/cli/backup/download.go Outdated Show resolved Hide resolved
pkg/cmd/cli/backup/logs.go Outdated Show resolved Hide resolved
pkg/cmd/cli/restore/describe.go Outdated Show resolved Hide resolved
pkg/cmd/cli/restore/logs.go Outdated Show resolved Hide resolved
site/docs/master/api-types/backupstoragelocation.md Outdated Show resolved Hide resolved
@s12chung s12chung force-pushed the revert-1776-revert-1769-ssl branch from 96251f9 to dc45af6 Compare October 3, 2019 19:50
@s12chung s12chung force-pushed the revert-1776-revert-1769-ssl branch from dc45af6 to 4c3aa42 Compare October 3, 2019 20:15
@s12chung
Copy link
Contributor Author

s12chung commented Oct 3, 2019

rebased. changed the changelog and added the suggested changes.

Copy link
Contributor

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM.

@skriss
Copy link
Contributor

skriss commented Oct 3, 2019

@nrb @carlisia @prydonius PTAL.

@skriss skriss requested review from carlisia, nrb and prydonius October 3, 2019 20:41
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

Lgtm. 👍

@carlisia carlisia merged commit db59d8d into vmware-tanzu:master Oct 3, 2019
@s12chung s12chung deleted the revert-1776-revert-1769-ssl branch October 3, 2019 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants