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

fixes #250 and provides the option to set ssl_peer_verify to false #251

Merged
merged 1 commit into from Feb 8, 2017

Conversation

mwrock
Copy link
Member

@mwrock mwrock commented May 11, 2016

No description provided.

@coderanger
Copy link
Contributor

👎 These options are a scourge and too much of a footgun IMO.

@adamleff
Copy link

👍 but could we change the parameter name so it's extremely clear what it's for? It feels too generic now which may confuse users.

How about verify_aws_ssl_peer, aws_api_ssl_verify, or similar?

@mwrock
Copy link
Member Author

mwrock commented May 11, 2016

@adamleff in a slack conversation we have been discussing the merits of other routs tro achieve the same end but without flipping the power button on security.

The thinking is that if we can provide guidance to users on how to manage their cacert.pem store with the fake root cert used in an environment as described in #250, thats a more "legit" approach. I want to keep this open while we explore that. Question is can users actually make it work in possibly sophisticated proxied environments where they may need to feed a chain of certs to the openssl cert file.

A carte-blanche verify_none may be the lesser evil in some dev/test environments.

@coderanger
Copy link
Contributor

If we do add something like this as an escape hatch, I would make the name something that makes it clear this is security risk. "disable TLS verification" sounds bad, but like "click through a Windows warning I've been desensitized to" bad :-/ Maybe just have a boolean for disable_tls_security?

@adamleff
Copy link

Yeah, I'm not crazy about having an option like this either, but we do have users where this may be the right solution.

We could also look at doing something like kitchen ssl fetch and kitchen ssl verify (a la knife) that each driver can implement depending on needs.

Proxies gonna prox. A "quick fix" here is to just make this change but also print out a HUGE warning each time kitchen is run explaining what's happening and encouraging them to visit a URL to a doc that explains how to fix this while still maintaining TLS verification.

@cheeseplus
Copy link
Contributor

If we can get this rebased and an appropriate warning and option name selected this still seems like a good thing.

@cheeseplus
Copy link
Contributor

Can we get a rebase? are we still 👍 on this?

@mwrock
Copy link
Member Author

mwrock commented Sep 28, 2016

rebased. I'm still +1.

@cheeseplus cheeseplus added this to the 1.2.1 milestone Feb 7, 2017
@cheeseplus
Copy link
Contributor

@mwrock merged some other PRs and now we need a rebase here - will get this as soon as you land one

@cheeseplus
Copy link
Contributor

The oddest thing is that now it says the merge has no conflicts which may be due to my merge/revert so I guess we don't need a rebase after all.

@cheeseplus cheeseplus merged commit 35889f0 into master Feb 8, 2017
@tas50 tas50 deleted the ssl_verify branch January 30, 2022 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants