New Custom Resource: Certificate and Library: SecurityCommand #53

mjmerin · 2018-03-01T22:36:34Z

This pull request fixes Issue #50. Rather than create a security resource, the task was split up to create a security back end to drive other resources such as a certificate and even a keychain resource.

SecurityCommand Library
Basic library intended to encapsulate some features of the security command. As of now this will allow you to unlock a keychain, and add a certificate of the .p12 or .cer format.

Certificate Custom Resource
This is a new custom resource that utilizes the SecurityCommand library mentioned above as its backend. It currently only has 3 properties for basic certificate install.

Notes

For testing, a PFX format Test.p12 has been added. This is a dummy certificate file

- Split up smoke tests for easier testing

Use the security command to install a certificate Use 3 properties and a simple if else statement to differentiate between .cer and .p12 files

- Add new recipe installing dummy .p12 file

Add _test to file name

Use some functions that are modeled after the actual security command such as unlock-keychain, add-certificates, and import. There is also a install_certificate wrapper that wraps the import and add-certificate commands to hide the differences of .p12 and .cer file installation from the user.

class.

americanhanko · 2018-03-01T22:42:03Z

libraries/security_cmd.rb

+      [@security, 'unlock-keychain', '-p', kc_passwd]
+    end
+
+    def add_certificates


Let's use a ternary statement here in the interest of brevity:

@keychain == '' ? [@security, 'add-certificates', @cert] : [@security, 'add-certificates', @cert, '-k', @keychain]

americanhanko · 2018-03-01T22:46:37Z

resources/certificate.rb

+property :keychain, String
+
+action_class do
+  def keychain


Ternary statement here as well:

property_is_set?(:keychain) ? new_resource.keychain : ''

americanhanko · 2018-03-01T22:50:10Z

resources/certificate.rb

+  test = SecurityCommand.new(new_resource.certfile, keychain)
+
+  execute 'unlock keychain' do
+    command [*test.unlock_keychain('vagrant')]


Let's change 'vagrant' to node['macos']['admin_user'] attribute; just in case someones uses a different username in their environment.

americanhanko · 2018-03-01T22:51:29Z

resources/certificate.rb

+end
+
+action :install do
+  test = SecurityCommand.new(new_resource.certfile, keychain)


I think we can use a better name than test here: perhaps certificate or resource_cert?

americanhanko · 2018-03-01T23:00:34Z

libraries/security_cmd.rb

+    end
+
+    def install_certificate(cert_passwd)
+      if ::File.extname(@cert) == '.p12'


In the X.509 standard, there are a few different file extensions used for the various types of files. For certificates, we might find them with .der, .crt, or .cer and for PKCS #12 files, we might find them with .p12 of pfx.

Might it make sense to check for these possible variations? Could we test for that?

The 'import' command can actually support the formats: openssl, bsafe, raw, pkcs7, pkcs8, pkcs12, x509, openssh1, openssh2, and pemseq. We can probably make an array of acceptable file formats and check against those.

americanhanko · 2018-03-01T23:07:00Z

libraries/security_cmd.rb

+      elsif ::File.extname(@cert) == '.cer'
+        add_certificates
+      else
+        Chef::Exception.fatal('invalid cert file')


In the interest of informative exceptions, let's include the certificate in the exception message. Maybe something like this?

Chef::Exception.fatal("Invalid certificate: #{@cert}. We looked in the #{@keychain} keychain. Double-check the integrity of both the certificate and the keychain.")

americanhanko · 2018-03-01T23:08:33Z

resources/certificate.rb

@@ -0,0 +1,28 @@
+resource_name :certificate
+default_action :install


If there is only one action, we don't need to indicate a default. If we decide to add more actions later, then we'll need to add it. I would probably remove it until then, but it's up to you.

americanhanko · 2018-03-01T23:09:44Z

spec/unit/libraries/security_cmd_spec.rb

+describe MacOS::SecurityCommand do
+  context 'when SecurityCommand object is instantiated' do
+    it 'security should use /usr/bin/security' do
+      test = MacOS::SecurityCommand.new('/Users/vagrant/Test.p12', '')


We don't need to instantiate this object for every test. Let's use let to create these objects, as well as some more descriptive names for the ones that get used:

describe MacOS::SecurityCommand do let(:p12_cert) { MacOS::SecurityCommand.new('/Users/vagrant/Test.p12', '') } context 'when a SecurityCommand object is instantiated' do it 'security should use /usr/bin/security' do expect(p12_cert.security).to eq '/usr/bin/security' end end end

From betterspecs.org:

When you have to assign a variable instead of using a before block to create an instance variable, use let. Using let the variable lazy loads only when it is used the first time in the test and get cached until that specific test is finished.

americanhanko · 2018-03-01T23:21:36Z

libraries/security_cmd.rb

+  class SecurityCommand
+    attr_reader :cert
+    attr_reader :keychain
+    attr_reader :security


If this attribute is the path to the command, let's indicate that with the name. Something like :security_executable, :security_command or :security_cmd

- Use ternary statements - Change vagrant password - Rename created certificate object for testing - Support more file types by putting them in an array before calling add or import - Use more informative exception

jazaval

Great work here @mjmerin - thanks to @americanhanko as well for the good feedback.

I'm excited to see what this looks like when paired with a keychain resource!

This merge contains hotfixes

mjmerin added 12 commits February 23, 2018 13:05

Initial commit for certificate resource

e6e8b59

- Split up smoke tests for easier testing

First pass of certificate resource

ec77f24

Use the security command to install a certificate Use 3 properties and a simple if else statement to differentiate between .cer and .p12 files

Testing the certificate resource

754a3e9

- Add new recipe installing dummy .p12 file

empty class for helper library

db00927

Fix verifier for certificate tests

462f686

Add smoke test to test certificate resource

6873962

Add _test to file name

Add spec tests to test the security_cmd library

1030ac6

Add certificate test recipe

0f76d95

Modify the custom certificate resource to use the SecurityCommand

04075d6

class.

Add documentation outlining usage of the new certificate custom resource

4d9f825

Fix typo

5818b67

mjmerin self-assigned this Mar 1, 2018

mjmerin requested review from americanhanko and jazaval March 1, 2018 22:36

americanhanko reviewed Mar 1, 2018

View reviewed changes

Fix in response to review comments

f72d8f5

- Use ternary statements - Change vagrant password - Rename created certificate object for testing - Support more file types by putting them in an array before calling add or import - Use more informative exception

jazaval approved these changes Mar 2, 2018

View reviewed changes

Merge branch 'release/1.7' into feature/pre_cert_rsrc

099de80

This merge contains hotfixes

jazaval merged commit d1fc2ed into release/1.7 Mar 5, 2018

americanhanko deleted the feature/pre_cert_rsrc branch March 7, 2018 03:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Custom Resource: Certificate and Library: SecurityCommand #53

New Custom Resource: Certificate and Library: SecurityCommand #53

mjmerin commented Mar 1, 2018

americanhanko Mar 1, 2018 •

edited

Loading

mjmerin Mar 2, 2018

americanhanko Mar 1, 2018

mjmerin Mar 2, 2018

americanhanko Mar 1, 2018

mjmerin Mar 2, 2018

americanhanko Mar 1, 2018 •

edited

Loading

mjmerin Mar 2, 2018

americanhanko Mar 1, 2018

mjmerin Mar 1, 2018

americanhanko Mar 1, 2018

mjmerin Mar 2, 2018

americanhanko Mar 1, 2018

mjmerin Mar 2, 2018

americanhanko Mar 1, 2018 •

edited

Loading

mjmerin Mar 2, 2018

americanhanko Mar 1, 2018

mjmerin Mar 2, 2018

jazaval left a comment

		@@ -0,0 +1,28 @@
		resource_name :certificate
		default_action :install

New Custom Resource: Certificate and Library: SecurityCommand #53

New Custom Resource: Certificate and Library: SecurityCommand #53

Conversation

mjmerin commented Mar 1, 2018

americanhanko Mar 1, 2018 • 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

americanhanko Mar 1, 2018 • 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

americanhanko Mar 1, 2018 • 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

jazaval left a comment

Choose a reason for hiding this comment

americanhanko Mar 1, 2018 •

edited

Loading

americanhanko Mar 1, 2018 •

edited

Loading

americanhanko Mar 1, 2018 •

edited

Loading