[CLOUD-296] Add required_recipe endpoint #1214

ryancragun · 2017-04-04T23:50:04Z

See Chef RFC 89 for context.

Add the ability to serve a required recipe file to chef-clients.

/organizations/<orgname>/required_recipe returns 404 for all organizations
by default.
/organizations/<orgname>/required_recipe returns 401 when the request is
not made by a client from the requested org and the feature is enabled.
/organizations/<orgname>/required_recipe returns the required recipe and
200 when the endpoint is enabled and requested by an authorized client.
required_recipe["enable"] in chef-server.rb enables the required recipe
feature.
required_recipe["path"] in chef-server.rb specifies the recipe file to
serve.

srenatus

Some comments 😃

srenatus · 2017-04-05T06:25:44Z

...files/private-chef-cookbooks/private-chef/templates/default/nginx/nginx_chef_api_lb.conf.erb

@@ -64,6 +64,14 @@
    }
    <% end -%>

+    <% if node['private_chef']['required_recipe']['enable'] -%>
+    location ~ /organizations/([^/]+)/required_recipe {


I can't imagine how this would ever become an issue, but since we all think that this location regex starts at the beginning of the request path, can you add a ^ there?

srenatus · 2017-04-05T06:26:55Z

...vate-chef/templates/default/nginx/scripts/check_token_using_endpoint_required_recipe.lua.erb

@@ -0,0 +1,28 @@
+<% unless node['private_chef']['required_recipe']['enable'] -%>


Since the route won't be rendered if node['private_chef']['required_recipe']['enable'] is false, is this block necessary?

I modified to always render the route and serve a 404 if it's disabled.

srenatus · 2017-04-05T06:27:22Z

...vate-chef/templates/default/nginx/scripts/check_token_using_endpoint_required_recipe.lua.erb

+<% unless node['private_chef']['required_recipe']['enable'] -%>
+-- required_recipe mode is disabled to always return a 404
+
+if true then


I'm not sure I understand what is gained from if true then here 😉

srenatus · 2017-04-05T06:29:04Z

omnibus/files/private-chef-cookbooks/private-chef/recipes/nginx.rb

@@ -125,6 +130,7 @@
 'routes.lua',
 'dispatch_route.lua',
 'check_token_using_endpoint.lua',
+ 'check_token_using_endpoint_required_recipe.lua',


I wonder if there would be a way to make the existing check_token_using_endpoint.lua generic enough to cover this case, too?

I thought about that, but all of them have very different post validation actions so making it generic might not be all that useful. Furthermore, I uncovered a bug with the validation endpoint where it'll return a 200 for any properly formatted GET request. I'm working to resolve that issue before this PR moves ahead. That issue likely impacts the compliance profile endpoint as well.

root@api:/tmp# !curl curl -kLI -X GET https://api.chef-server.dev/organizations/test/validate/i_like_bananas HTTP/1.1 200 OK Server: openresty/1.11.2.1 Date: Wed, 05 Apr 2017 17:35:22 GMT Content-Type: text/html Content-Length: 2474 Last-Modified: Tue, 04 Apr 2017 22:40:36 GMT Connection: keep-alive ETag: "58e420e4-9aa" Accept-Ranges: bytes

Looks like you're right, and it's because that response is my favourite piece of html ever

<h1>Are You Looking For the Chef Server?</h1> <p>Hello! It looks like you were trying to browse to your Chef Server but you haven't installed the Management Console.</p>

@ryancragun are you looking into this or do you want me to file a big somewhere? 😉

@ryancragun nevermind, I've found the JIRA card. Thanks :)

stevendanna

In addition to the inline comments, it would be good if we had:

Tests for this endpoint, either in pedant or somewhere else that is regularly run in the test pipeline
Updates to the API docs

I like how easy it is to add small endpoints like this in lua; however, I do wonder if in the long run it would b better to put this in erchef itself.

stevendanna · 2017-04-05T07:25:14Z

omnibus/files/private-chef-cookbooks/private-chef/recipes/nginx.rb

@@ -74,6 +72,13 @@
  node.default['private_chef']['nginx']['ssl_certificate_key'] = ssl_keyfile
 end

+# Link the required recipe file into the HTML static files root.


Should we consider checking the permissions on the file provided by the user? That is, should we require that the file has restricted permissions to avoid a situation where a random user on the machine can modify content served to all chef-clients?

I like this idea. Are we thinking readable only by opscode or root?

@srenatus 644 (root:root)

This is all handled by the preflight check. Thanks!

stevendanna · 2017-04-05T08:10:39Z

When reviewing the RFC, it occurred to me that doing this in lua means we don't (currently) have access to the API versioning stuff (and also don't return API version information), which could make this harder to deprecate in the future if we ever replace it with a more robust feature.

However, I can't think of a reason we couldn't move this into erchef at the point of wanting to deprecate it.

stevendanna · 2017-04-05T08:17:54Z

Per @srenatus:

Hmm this also has HA downsides, doesn’t it? you’d have to put that on each FE

Since this is being presented as a "super advanced" feature, perhaps we can just document that you have to keep them in sync; but things could get /very/ confusing if they were out of sync.

stevendanna · 2017-04-05T12:36:17Z

omnibus/files/private-chef-cookbooks/private-chef/attributes/default.rb

@@ -41,6 +41,9 @@

 default['private_chef']['fips_enabled'] = ChefConfig.fips?

+default['private_chef']['required_recipe']['enable'] = false


I'm wondering if we want both of these? We could alternatively check if path was non-nil. If we do want both of these, we might want to add a check that path is set when enable is true to avoid a potentially confusing error message during reconfigure.

Yeah, I can see why having two might not be necessary. I just wonder if having an on/off switch makes more usability sense than just having a path.

Circling back on this, I definitely think sticking with both is ideal. If we ever move to a place where this file is stored in a database then we're unlikely to have a path. In that scenario we'd be left without a toggle.

But if the file is stored in the database, then we'll need no config because if it's there it should be returned and if it's not it wont. But that's jumping to implementation details.

I dunno, the file existing and the feature being enabled/disabled are two different things. But yeah, YAGNI. The idea here is to have an implementation agnostic switch and the path is definitely not agnostic.

stevendanna · 2017-04-05T12:37:53Z

I'm wondering if we want to send a header such as Content-MD5 or similar that the client could use to verify the contents weren't modified/corrupted in transit?

ryancragun · 2017-04-05T17:55:42Z

@stevendanna @srenatus Sorry y'all, this is very much a WIP PR that I hadn't cleaned up, added documentation for, or added tests for. That said, I appreciate the feedback.

When reviewing the RFC, it occurred to me that doing this in lua means we don't (currently) have access to the API versioning stuff (and also don't return API version information), which could make this harder to deprecate in the future if we ever replace it with a more robust feature.

However, I can't think of a reason we couldn't move this into erchef at the point of wanting to deprecate it.

That's exactly the plan. I don't anticipate that happening any time soon though.

Hmm this also has HA downsides, doesn’t it? you’d have to put that on each FE
Since this is being presented as a "super advanced" feature, perhaps we can just document that you have to keep them in sync; but things could get /very/ confusing if they were out of sync.

Yep, this was definitely a consideration but we've decided that the scope is limited to standalone installs. If we ever chose to support HA we'd probably move it into erchef and make the create API limited to server admins/pivotal.

I'm wondering if we want to send a header such as Content-MD5 or similar that the client could use to verify the contents weren't modified/corrupted in transit?

I don't really see a way to have nginx add that header for static content. It should be easy enough to calculate it during the reconfigure and inject it into the nginx location block though.

marcparadise · 2017-04-05T19:50:06Z

Overall this looks okay, but I see this as something that belongs more properly in the erchef API in the longer term. I know that we're in a situation where that may not be an option right now.

To that end, I'd like to create a sustainable interface contract now so that we can address this in the future without further client updates being necessary.

This should behave not as a recipe endpoint, but as a cookbook endpoint. Chef Client already knows how to to load a cookbook in full. Let's have our response look the same. Some modifications will still be needed on the client (may not be a metadata.rb that's downloadable).

On the server side, the endpoint would give a JSON response that looks like a versioned cookbook get. It would contain the storage URL for the recipe we want the client to grab. The recipe file we care about would come out of the backing file store (S3/bookshelf) , but still proxied via nginx and able to take advantage of caching if it's enabled. This would be something we could more easily expand in the future, with support for cookbook files and dependencies if we wanted to go that route.

For nginx/lua this would mean constructing the JSON response and generating the S3 url.

More importantly than the potential to expand it in the future though: I am increasingly less comfortable with the path of adding one-off behaviors into the reverse proxy instead of into the application that houses our API.
If we want to have a separate lightweight API service for some of these tasks , we should design it and build it (nginx or otherwise) - but accumulating one-off by_lua behaviors isn't a sustainable approach.

Separately: we will also need to add pedant coverage for this endpoint.

danielsdeleo · 2017-04-05T22:47:11Z

To give a little more context here:

A few customers have asked for the ability to enforce certain items being in a node's run list at the server level, but this isn't that feature. There are a lot of possible designs that include having a side channel for a single cookbook, validating the run list at create/edit time to enforce required content, or having multiple run lists on the node. Each of these has variations on how the cookbook content could be uploaded/managed. On top of that, there are a lot of trade-offs related to attribute visibility between the various pieces and so on that would need to be decided upon in the client implementation. We would also need to look at the end to end development/testing/deployment story. To have a better chance at getting all of this right, we would want to engage some development partners and do a fair bit of research with them to work through the various trade-offs that various approaches could have. However, we haven't seen enough demand for such a feature that we moved it to the top of the priority list, and some folks (me especially) think that such a feature could discourage users from taking a more productive route to solving their problems, such as collaborating via shared Ci infrastructure.

Given all that, we're inclined to go hard on the YAGNI principle here. A lot of our strategy for long term scalability (in functionality terms) of the feature is to implement as little functionality as possible so it's not a huge cost to throw away and replace. We know so little about what shape a replacement would look like that a lot of work we might try to do to anticipate future improvements would very likely end up as wasted work.

In terms of the client work, Chef Client also already knows how to load single recipes in addition to its run list (see chef-client -z path/to/recipe.rb) so we are re-using that code path.

All that said, while we are targeting standalone installations for now, we do want to write the code so that an HA-compatible implementation could be written in the future without changing the configuration interface or Chef Client's download mechanism.

marcparadise · 2017-04-14T20:13:52Z

@ryancragun @danielsdeleo: I am sorry. I should have done a much better job at not being an ass in my initial comment. Next time our paths cross, 🍺 (or other drink of choice) is on me.

ryancragun · 2017-04-17T22:00:52Z

@marcparadise @stevendanna @srenatus @sdelano

Howdy y'all. I think that this and #1226 are ready for 👀. All of the feedback should be addressed and an integration test plan has been formed on #1226.

Thanks!

sdelano · 2017-04-20T01:40:26Z

...files/private-chef-cookbooks/private-chef/templates/default/nginx/nginx_chef_api_lb.conf.erb

+      add_header Content-MD5 <%= Digest::MD5.base64digest(::File.read(node['private_chef']['required_recipe']['path'])) %>;
+      alias <%= ::File.join(@dir, "html", "required_recipe") %>;
+    <% else -%>
+      return 404;


This will return 404 if the location is not configured. It seems a bit more straightforward to have the whole location block within the if block and just not render it when it's not enabled.

Putting the location inside also seems to be more in keeping with the patterns elsewhere.

sdelano · 2017-04-20T01:44:00Z

@ryancragun I left 2 very minor comments related to dual config (enabled and recipe_path) and how we render the nginx config. In the end, the functionality remains the same, so I'll leave it up to you on whether you'd like to address those two issues. If not, merge away!

markan

Everything looks good other than the above comments. I think we're probably better off all around copying the required recipe over to a secure location than guarding it with checks, but if we are diligent with guards that could be made to work.

markan · 2017-04-20T03:59:23Z

...s/files/private-chef-cookbooks/private-chef/libraries/preflight_required_recipe_validator.rb

+
+  def verify_required_recipe_owner
+    unless ::File.stat(@required_recipe['path']).uid == 0
+      fail_with <<EOF


If the intent is to protect the user from a unsafe file choice, we probably should require the entire path to the file be owned by root, and not have any links.
For example:

mark@souschef{1}% ln -s /etc/passwd mark@souschef{2}% pwd /home/mark mark@souschef{3}% irb irb(main):001:0> ::File.stat('passwd') => #<File::Stat dev=0x801, ino=1836570, mode=0100644, nlink=1, uid=0, gid=0, rdev=0x0, size=2154, blksize=4096, blocks=8, atime=2017-04-18 23:15:01 -0700, mtime=2016-12-14 22:09:57 -0800, ctime=2016-12-14 22:09:57 -0800> irb(main):002:0> ::File.stat('passwd').uid => 0

Later....

badguy@souschef{4}% rm passwd badguy@souschef{5}% ln -s badfile passwd

Similar things could happen if the path contained a link or was otherwise under the control/ownership of a not completely trusted party...

It might be safer to have reconfigure copy the file off to a location under our control (perhaps /var/opt/opscode/nginx.... ) and set the permissions/ownership to be correct. That would prevent issues with users changing the file and forgetting to reconfigure, and make it easier to run nginx as a chroot-ed process or container with mounted filesystem at some future date.

markan · 2017-04-20T04:04:30Z

...files/private-chef-cookbooks/private-chef/templates/default/nginx/nginx_chef_api_lb.conf.erb

+      add_header Content-MD5 <%= Digest::MD5.base64digest(::File.read(node['private_chef']['required_recipe']['path'])) %>;
+      alias <%= ::File.join(@dir, "html", "required_recipe") %>;
+    <% else -%>
+      return 404;


Putting the location inside also seems to be more in keeping with the patterns elsewhere.

stevendanna · 2017-04-20T19:43:12Z

@markan

I think we're probably better off all around copying the required recipe over to a secure location than guarding it with checks, but if we are diligent with guards that could be made to work.

The only problem I see is that the user might want to change the content, so it will always be the case that we need to read it in from the user-provided location so we will always want that user-provided location to have safe permissions.

ryancragun · 2017-04-20T20:20:35Z

Checking the permissions at reconfigure time is a good idea. I agree that copying it instead of linking is needed. The content md5 header has to be regenerated if you intend to change the file content, a case the previous link method did not address. By validating and copying we ensure that:

the file is always in a directory we own
can only be changed by root
source file must be owned by root
that the md5 header is always in sync

markan · 2017-04-20T20:23:51Z

👍 LGTM

stevendanna

Left a few minor comments, but this looks good to me.

stevendanna · 2017-04-20T20:31:54Z

...s/files/private-chef-cookbooks/private-chef/libraries/preflight_required_recipe_validator.rb

+The required_recipe must have a mode of 644. Please set mode to 644 and
+reconfigure the Chef server:
+
+    chmod 644 #{@required_recipe['path']}


Since we are now copying the contents, then we might consider making this 600 and/or making the check accept either 644 or 600.

stevendanna · 2017-04-20T20:32:36Z

...s/files/private-chef-cookbooks/private-chef/libraries/preflight_required_recipe_validator.rb

+  end
+
+  def verify_required_recipe_mode
+    mode = ::File.stat(@required_recipe['path']).mode


Very minor, but we could save a few calls to stat by moving this into a memoized function.

stevendanna · 2017-04-20T20:33:19Z

omnibus/files/private-chef-cookbooks/private-chef/recipes/nginx.rb

+# ensure that it's only modifiable by root.
+if node['private_chef']['required_recipe']['enable']
+  remote_file ::File.join(nginx_html_dir, 'required_recipe') do
+    source "file://#{node['private_chef']['required_recipe']['path']}"


Interesting! I've always used file with content ::File.read(blah), didn't know file_urls worked here.

See the [chef-rfc]("chef-boneyard/chef-rfc#258") for full details on the motivation. Add the ability to serve a required recipe file to chef-clients. * `/organizations/<orgname>/required_recipe` returns `404` for all organizations by default. * `/organizations/<orgname>/required_recipe` returns `401` when the request is not made by a client from the requested org and the feature is enabled. * `/organizations/<orgname>/required_recipe` returns the required recipe and `200` when the endpoint is enabled and requested by an authorized client. * `required_recipe["enable"]` in `chef-server.rb` enables the require recipe feature. * `required_recipe["path"]` in `chef-server.rb` specifies the recipe file to serve. Signed-off-by: Ryan Cragun <[email protected]>

ryancragun force-pushed the ryan/cloud-296 branch from 6aba02f to 3ad872e Compare April 4, 2017 23:50

srenatus reviewed Apr 5, 2017

View reviewed changes

stevendanna reviewed Apr 5, 2017

View reviewed changes

ryancragun force-pushed the ryan/cloud-296 branch from 3ad872e to 52eb1a0 Compare April 5, 2017 17:33

sdelano mentioned this pull request Apr 6, 2017

server enforced required recipe chef/chef#6032

Merged

4 tasks

ryancragun force-pushed the ryan/cloud-296 branch 2 times, most recently from fc59d9e to 812ada7 Compare April 13, 2017 00:52

ryancragun mentioned this pull request Apr 17, 2017

[CLOUD-306] Add validate upstream location to nginx #1226

Merged

ryancragun force-pushed the ryan/cloud-296 branch from 812ada7 to 8f6f98d Compare April 20, 2017 00:45

sdelano reviewed Apr 20, 2017

View reviewed changes

markan suggested changes Apr 20, 2017

View reviewed changes

ryancragun force-pushed the ryan/cloud-296 branch from 8f6f98d to d443e21 Compare April 20, 2017 20:13

markan approved these changes Apr 20, 2017

View reviewed changes

stevendanna approved these changes Apr 20, 2017

View reviewed changes

ryancragun force-pushed the ryan/cloud-296 branch from d443e21 to f6fa451 Compare April 20, 2017 21:14

ryancragun merged commit 13c6642 into master Apr 20, 2017

ryancragun deleted the ryan/cloud-296 branch April 20, 2017 21:26

danielsdeleo mentioned this pull request Jul 12, 2017

Document required_recipe feature and configuration chef/chef-web-docs#793

Merged

		@@ -0,0 +1,28 @@
		<% unless node['private_chef']['required_recipe']['enable'] -%>

		@@ -41,6 +41,9 @@

		default['private_chef']['fips_enabled'] = ChefConfig.fips?

		default['private_chef']['required_recipe']['enable'] = false

[CLOUD-296] Add required_recipe endpoint #1214

[CLOUD-296] Add required_recipe endpoint #1214

Conversation

ryancragun commented Apr 4, 2017 • edited Loading

srenatus 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

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

stevendanna 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

stevendanna commented Apr 5, 2017

stevendanna commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryancragun Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

sdelano Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevendanna commented Apr 5, 2017

ryancragun commented Apr 5, 2017

marcparadise commented Apr 5, 2017 • edited Loading

danielsdeleo commented Apr 5, 2017

marcparadise commented Apr 14, 2017 • edited Loading

ryancragun commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdelano commented Apr 20, 2017

markan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevendanna commented Apr 20, 2017

ryancragun commented Apr 20, 2017 • edited Loading

markan commented Apr 20, 2017

stevendanna 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

ryancragun commented Apr 4, 2017 •

edited

Loading

ryancragun Apr 20, 2017 •

edited

Loading

sdelano Apr 20, 2017 •

edited

Loading

marcparadise commented Apr 5, 2017 •

edited

Loading

marcparadise commented Apr 14, 2017 •

edited

Loading

ryancragun commented Apr 20, 2017 •

edited

Loading