[ER-459] Add crypto2 to the Erlang load path in FIPS mode #1065

rmoshier · 2017-01-26T22:52:09Z

This templatizes vm.args to include the crypot2 module at the front of the load path if FIPS mode is enabled. This means that after adding fips true to chef-server.rb and reconfiguring, the Erlang app will be using the crypto2 module to access the OpenSSL FIPS module.

We include the erlang-crypto2 module in all omnibus distributions, but users chose to activate it at runtime.

Manual testing to make sure this works
Update the other Erlang modules to use a similar pattern

stevendanna

Nice start on this! Left a few (hopefully minor) comments. Let me know if any of them are bigger changes than seem reasonable.

stevendanna · 2017-01-26T23:57:02Z

omnibus/Gemfile.lock

  specs:
    license_scout (0.1.2)
      ffi-yajl (~> 2.2)
      mixlib-shellout (~> 2.2)

 GIT
  remote: git://github.com/chef/omnibus-software.git
-  revision: 9f7c4536e484ab68fcbae96e527a3fbaf961bb15
+  revision: d55178fb6d2afbafee17b7e95ed0d1bad4736e22


I think these changes will go away if you rebase on master.

stevendanna · 2017-01-26T23:58:14Z

omnibus/config/software/erlang-crypto2.rb

+
+# TODO get Jay to convert this to Apache 2
+license "Reserved"
+license_file "LICENSE"


The LICENSE file looks like a 3-clause BSD license, so I think we are fine here, just need to change "Reserved" to BSD-3

stevendanna · 2017-01-27T00:00:10Z

omnibus/config/software/erlang-crypto2.rb

+license_file "LICENSE"
+
+dependency "erlang"
+dependency "rebar"


This will be rebar 2. Does this compile with rebar3? If so, it might be good to make a rebar3 software definition (we can pull their prebuilt binary and copy it into embedded/bin) so we don't have to deal with both rebar 2 and rebar 3 when figuring out build issues in the future.

It compiled successfully with rebar 2 in my tests. So I figured I would try that instead of trying to bring in rebar 3.

I thought that chef-server was using rebar 2? Thats the default version of the rebar definition (and there is no rebar3 binary)

@tyler-ball Chef Server uses rebar3 everywhere. We moved to rebar3 during the beta so we vendored rebar3 into each app at the time of conversion. We have a long-standing TODO to get to a single version now that it is stable. (You'll see src/oc_erchef/rebar3, src/bookshelf/rebar3, etc).

stevendanna · 2017-01-27T00:01:48Z

omnibus/config/software/erlang-crypto2.rb

+build do
+  env = with_standard_compiler_flags(with_embedded_path)
+
+  env['USE_SYSTEM_GECODE'] = "1"


Are these environment variables necessary for erlang-crypto2? I don't seem them referenced in Jay's repo.

I have no idea - I can try removing them and seeing if it still compiles. I just copied the erchef definition and left in whatever was there

stevendanna · 2017-01-27T00:03:52Z

omnibus/files/private-chef-cookbooks/private-chef/recipes/opscode-erchef.rb

@@ -59,6 +59,12 @@
  notifies :restart, 'runit_service[opscode-erchef]' unless backend_secondary?
 end

+template "/opt/opscode/embedded/service/opscode-erchef/vm.args" do
+  source "oc_erchef.vm.args.erb"


I believe we'll eventually want to do this for the other erlang apps as well so we'll likely want this to be a more generic template that we can pass the app name into.

But the other vm.args I looked at was pretty different. I'll see if I can make some common template parts I will. If they are all different, I'll leave them as separate templates.

stevendanna · 2017-01-27T00:28:36Z

omnibus/files/private-chef-cookbooks/private-chef/recipes/opscode-erchef.rb

@@ -59,6 +59,12 @@
  notifies :restart, 'runit_service[opscode-erchef]' unless backend_secondary?
 end

+template "/opt/opscode/embedded/service/opscode-erchef/vm.args" do


For sys.config we write the template into /var/opt/opscode/opscode-erchef and then symlink that into /opt/opscode/embedded/service/opscode-erchef/. Perhaps we should do the same here for symmetry?

I saw that. I didn't know the reasoning behind symlinking instead of just writing the file. If you think thats the right thing to do, then I'll do that!

@tyler-ball I am honestly unsure on the original intent of these symlinks. I think we've been carting them around for a while. @marcparadise Do you know?

I meant to comment not approve.

tyler-ball · 2017-02-01T18:39:12Z

I need to test this some more and see if there are any tests I can add/update, but I would love to start getting eyes on this. /cc @chef/chef-server-maintainers

stevendanna

Nice! Thanks for fighting through this one. I like the approach y'all landed on.

I've left a few comments but the only ones I think is blocking is to clean up all of the rebar.config's and the re-ordering of the project file.

I think that eventually we could unify those templates but don't want that to block this moving forward.

stevendanna · 2017-02-01T18:46:58Z

omnibus/config/projects/chef-server.rb

@@ -97,6 +97,7 @@
 dependency "private-chef-upgrades"
 dependency "private-chef-cookbooks"
 dependency "chef-ha-plugin-config"
+dependency "erlang-crypto2"


Let's move this further up. My guess is this code will not be changing often after we have it stable.

Done, moved into the global commented block

stevendanna · 2017-02-01T18:51:55Z

omnibus/config/software/oc_bifrost.rb

@@ -25,7 +25,7 @@

 build do
  env = with_standard_compiler_flags(with_embedded_path)
-  profile_name = fips_mode? ? "fips" : "default"
+  profile_name = "default"


Not important right now, but we can probably remove the profile_name stuff completely and just assume "default" which is what rebar3 will build by...default.

I assumed it would do that but wanted to introduce as few changes as possible. I'll go ahead and remove this though since it does default by... default 😁

stevendanna · 2017-02-01T18:55:54Z

src/oc_erchef/rebar.config

@@ -101,11 +101,6 @@
                ]},
        {erl_opts, [export_all]}
       ]},
-    {fips, [


Can we do similar cleanups in other erlang apps?

:doh: Incoming!

stevendanna · 2017-02-01T19:02:24Z

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

+  owner OmnibusHelper.new(node).ownership['owner']
+  group OmnibusHelper.new(node).ownership['group']
+  mode "644"
+  notifies :restart, 'runit_service[oc_bifrost]' unless backend_secondary?


Note to other reviewers: The differences in the restart strategies across the various apps match the already existing differences from the sys.config files for these apps. My guess is that this can be sorted out so they are all the same, but I don't think it should be a blocker for this.

tyler-ball · 2017-02-01T20:16:04Z

@stevendanna Updated per your comments!

tyler-ball · 2017-02-01T20:51:27Z

Pedant tests passed with a local build

marcparadise

This looks a lot more manageable.

Since we're moving the vm.args files [back] into templates, let's also remove them from their original locations in $project/config/vm.args, and the overlay references in $project/rebar.config

marcparadise · 2017-02-01T20:52:18Z

omnibus/config/software/erlang-crypto2.rb

+#
+
+name "erlang-crypto2"
+default_version "er-459/update-crypto"


Reminder to change this once the branch lands

marcparadise · 2017-02-01T20:54:47Z

omnibus/config/software/erlang-crypto2.rb

+skip_transitive_dependency_licensing true
+
+dependency "erlang"
+dependency "rebar"


This will pull in rebar2 which we're trying to eradicate. It's not great, but our current practice is to include the rebar3 binary within the project.

I'm fine with updating to rebar3. How is that binary included in chef-server right now? I don't see an omnibus software dependency on it. I definitely want to build this using whatever other rebar tools we use.

I see that it is just copied into the 4 Erlang applications. I'm going to use the .../src/bookshelf/rebar3 instance to build this application.

tyler-ball · 2017-02-02T20:00:44Z

@marcparadise I had to restore the vm.args for the oc_bifrost application. Without this file present in the root of the release, the bin/oc_bifrost start command loads vm.args and sys.config from releases/12.12.0-blah/. Those files are defaulted / empty so they are missing necessary config values to run.

Restoring vm.args to the root of the release dir causes the bin/oc_bifrost start to load vm.args and sys.config from the root dir. This sys.config has the correct configurations needed and the applicaiton starts.

It looks like the bin/oc_bifrost command is generated by rebar (or relx) so I could not change that. Instead I opted to restore the vm.args file that I had deleted.

rmoshier · 2017-02-02T21:36:08Z

doc/HOW_THINGS_WORK.md

+* Build the erlang-crypto2 app as a separate omnibus definition using the same
+Erlang libraries used to build all other Erlang apps. Copy the `ebin` and `priv`
+folders from the build into a custom location inside the omnibus package.
+* If `fips true` is set and the server reconfigured, we update the `vm.args`


s/server reconfigured/server is reconfigured

Signed-off-by: tyler-ball <[email protected]>

…g FIPS module does not support TLS Signed-off-by: tyler-ball <[email protected]>

Signed-off-by: tyler-ball <[email protected]>

…ave a rebar 2 binary in the final package Signed-off-by: tyler-ball <[email protected]>

…template now Signed-off-by: tyler-ball <[email protected]>

They fail because the missing vm.args file causes the oc_bifrost application to load vm.args and sys.config from the releases folder. These files have default / empty values and the oc_bifrost application then fails to start. Signed-off-by: tyler-ball <[email protected]>

Signed-off-by: tyler-ball <[email protected]>

stevendanna previously approved these changes Jan 27, 2017

View reviewed changes

rmoshier changed the title ~~Er 454/crypto2~~ Er 459/crypto2 Jan 30, 2017

tyler-ball force-pushed the er-454/crypto2 branch from 395e595 to f7c2aba Compare January 30, 2017 19:22

tyler-ball changed the title ~~Er 459/crypto2~~ [ER-459] Add crypto2 to the Erlang load path in FIPS mode Jan 30, 2017

This was referenced Feb 1, 2017

Update to support runtime loading chef/erlang-crypto2#9

Closed

Bringing omnibus process in line with other Chef applications #1070

Closed

tyler-ball force-pushed the er-454/crypto2 branch from a19a302 to f0d437f Compare February 1, 2017 18:34

tyler-ball force-pushed the er-454/crypto2 branch from 8e429e7 to 23d4dcd Compare February 1, 2017 18:41

stevendanna suggested changes Feb 1, 2017

View reviewed changes

stevendanna approved these changes Feb 1, 2017

View reviewed changes

marcparadise requested changes Feb 1, 2017

View reviewed changes

tyler-ball force-pushed the er-454/crypto2 branch from 76c8d8f to 2b1a534 Compare February 2, 2017 20:55

rmoshier commented Feb 2, 2017

View reviewed changes

marcparadise approved these changes Feb 2, 2017

View reviewed changes

tyler-ball added 9 commits February 2, 2017 16:24

[ER-459] Add crypto2 to the Erlang load path in FIPS mode

05cf261

Signed-off-by: tyler-ball <[email protected]>

Disable rabbitmq SSL mode when running fips_enabled because our Erlan…

a54b780

…g FIPS module does not support TLS Signed-off-by: tyler-ball <[email protected]>

Removing missed instances of the fips relx profile

011273f

Signed-off-by: tyler-ball <[email protected]>

Using crypto2 repo from the chef org since we moved it

3014e32

Signed-off-by: tyler-ball <[email protected]>

Using existing rebar3 binary to compile erlang-crypto2 so we don't le…

1ab7b3c

…ave a rebar 2 binary in the final package Signed-off-by: tyler-ball <[email protected]>

Removing default vm.args files because we are generating them from a …

d6f6bc5

…template now Signed-off-by: tyler-ball <[email protected]>

Updating HOW_THINGS_WORK.md with an explanation of FIPS mode

4160a18

Hardcoding /opt/opscode is bad, mkay

950e768

Signed-off-by: tyler-ball <[email protected]>

tyler-ball force-pushed the er-454/crypto2 branch from 2b1a534 to 950e768 Compare February 2, 2017 22:25

tyler-ball merged commit e5b2c91 into master Feb 3, 2017

tyler-ball deleted the er-454/crypto2 branch February 3, 2017 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ER-459] Add crypto2 to the Erlang load path in FIPS mode #1065

[ER-459] Add crypto2 to the Erlang load path in FIPS mode #1065

rmoshier commented Jan 26, 2017 •

edited by tyler-ball

Loading

stevendanna left a comment •

edited

Loading

stevendanna Jan 26, 2017

stevendanna Jan 26, 2017

stevendanna Jan 27, 2017

tyler-ball Jan 27, 2017

stevendanna Jan 27, 2017

stevendanna Jan 27, 2017

tyler-ball Jan 27, 2017

stevendanna Jan 27, 2017

tyler-ball Jan 27, 2017

stevendanna Jan 27, 2017

tyler-ball Jan 27, 2017

stevendanna Jan 27, 2017

tyler-ball commented Feb 1, 2017

stevendanna left a comment •

edited

Loading

stevendanna Feb 1, 2017

tyler-ball Feb 1, 2017

stevendanna Feb 1, 2017

tyler-ball Feb 1, 2017

stevendanna Feb 1, 2017

tyler-ball Feb 1, 2017

stevendanna Feb 1, 2017 •

edited

Loading

tyler-ball commented Feb 1, 2017

tyler-ball commented Feb 1, 2017

marcparadise left a comment

marcparadise Feb 1, 2017

marcparadise Feb 1, 2017

tyler-ball Feb 2, 2017

tyler-ball Feb 2, 2017

tyler-ball commented Feb 2, 2017

rmoshier Feb 2, 2017

[ER-459] Add crypto2 to the Erlang load path in FIPS mode #1065

[ER-459] Add crypto2 to the Erlang load path in FIPS mode #1065

Conversation

rmoshier commented Jan 26, 2017 • edited by tyler-ball Loading

stevendanna left a comment • edited Loading

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

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

tyler-ball commented Feb 1, 2017

stevendanna left a comment • edited Loading

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 Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

tyler-ball commented Feb 1, 2017

tyler-ball commented Feb 1, 2017

marcparadise 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

tyler-ball commented Feb 2, 2017

Choose a reason for hiding this comment

rmoshier commented Jan 26, 2017 •

edited by tyler-ball

Loading

stevendanna left a comment •

edited

Loading

stevendanna left a comment •

edited

Loading

stevendanna Feb 1, 2017 •

edited

Loading