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

Add SSLPolicy class that will use chefdk trusted certs path #1640

Merged
merged 11 commits into from
Dec 15, 2016

Conversation

afiune
Copy link

@afiune afiune commented Dec 7, 2016

We are introducing a new class called SSLPolicy that, by default, will create the SSL Store pointing to the chef trusted certificates path. The class has been implemented in the main Uploader and Downloader so that they now pass an extra SSL option called cert_store. This options are passed to the underlayer gem Ridley that itself passes them to Faraday (HTTP client) so that on every connection we use the SSL store we provided for authentication. :mindblown:

Fixes #1470

howdoicomputer and others added 9 commits December 5, 2016 12:20
…with the trusted_certs_dir that knife uses for self-signed certificates. Tests are not passing and there is a lot left to be desired
…ail when you pass in a trusted_certs_dir attribute for Ridley::Config as well a cert store into connection constructors - one test is skipped and that's on my todo list to mock out and fix
…er or not a trusted_certs_dir exists and whether or not it's specified in a knife.rb file for a workstation; I also created a fixture for providing a dummy ssl cert for the Ridley::Chef::Config double object so that tests function
Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
If we pass the option `false`, we would expect to assert for the same
option.

Signed-off-by: Salim Afiune <afiune@chef.io>
Signed-off-by: Salim Afiune <afiune@chef.io>
@afiune
Copy link
Author

afiune commented Dec 7, 2016

ping @howdoicomputer @srenatus !!

Gotta open a new PR but look at this, tests passed!!

@afiune
Copy link
Author

afiune commented Dec 7, 2016

cc/ @berkshelf/berks-core

@howdoicomputer
Copy link
Contributor

howdoicomputer commented Dec 7, 2016

huzzah

Edit: HUZZAH!

@afiune
Copy link
Author

afiune commented Dec 8, 2016

Looking more in detail I really don't see how this is going to work. 😂 At the end of the day we need Ridley to understand the trusted_cert_dir and I don't think we do that there. I might look closer at this tomorrow morning.

Signed-off-by: Salim Afiune <afiune@chef.io>
@afiune
Copy link
Author

afiune commented Dec 8, 2016

This commit will fail on the tests intentionally so we have time to make some analysis on the missing parts of the puzzle.

Im putting this on hold for a moment since we have some other priorities with ChefDK release.

I'll come back soon though!

@afiune
Copy link
Author

afiune commented Dec 13, 2016

Alright Im back to work on this!! 👍

Also added test for the chef_server location type! 😄

Signed-off-by: Salim Afiune <afiune@chef.io>
@afiune afiune changed the title Issue 1470 chef dk trusted certs path Add SSLPolicy class that will use chefdk trusted certs path Dec 14, 2016
@afiune
Copy link
Author

afiune commented Dec 14, 2016

If someone looks like this after reading through this PR; I will understand...

@afiune
Copy link
Author

afiune commented Dec 14, 2016

cc/ @berkshelf/berks-core

@danielsdeleo
Copy link
Contributor

This looks good to me. If you wanted to go a step further, you could move some of the code into chef-config so berks and knife will have identical behavior and bug fixes. But this is already a big improvement so that could be saved for the future.

@howdoicomputer
Copy link
Contributor

Oh, yeah, this thing.

@lamont-granquist
Copy link
Contributor

👍

@afiune
Copy link
Author

afiune commented Dec 15, 2016

I am approving this since we have two +1

@danielsdeleo I am creating an issue to go the extra mile on a different card. It is indeed a great idea that we would love to implement on the next iteration. 💯

@afiune afiune merged commit 468e156 into master Dec 15, 2016
@afiune afiune deleted the ISSUE-1470-chef-dk-trusted-certs-path branch December 15, 2016 03:49
@howdoicomputer
Copy link
Contributor

Ahhhh code I wrote ended up in berkshellf 😄 I can look into that additional ticket.

@afiune
Copy link
Author

afiune commented Dec 15, 2016

@vinyar
Copy link
Contributor

vinyar commented Jan 22, 2017

Ok, so I wanted to make sure the code in this PR handles a particular usecase:

Chef server is using a cert issued by company's internal CA. All of their machines come with a CA built-in, so all chef-clients run succeed without the need to have full cert chain in trusted_certs folder.

If Berks only knows about the trusted_cert folder, it may still fail, because root CA is in a different store.

@berkshelf berkshelf locked and limited conversation to collaborators Jun 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants