(PUP-5588) Prevent Windows duplicate cert loads

 - Due to an errant test, it was discovered that unique cert detection
   when loading from the Windows::RootCerts is flawed.

   The underlying Windows API call CertOpenSystemStore(nil, 'ROOT') may
   return certs that are duplicates / contain the same raw cert data.
   https://msdn.microsoft.com/en-us/library/windows/desktop/aa376560(v=vs.85).aspx

   This can lead to extraneous messages every time Windows::RootCerts
   are copied into the local OpenSSL::X509::Store instance, like:

   Failed to add /C=US/O=GeoTrust Inc./CN=GeoTrust Global CA
   Failed to add /C=US/O=Equifax/OU=Equifax Secure Certificate Authority

   This is particularly problematic for modules like the Azure module
   that are frequently reloading the OpenSSL::X509::Store to make REST
   API calls.

 - The solution is to update the OpenSSL::X509::Store monkey path to
   pass a predicate to Rubys uniq function, to ensure that certs are
   compared by their DER byte representation.  Without this change, the
   default uniq implementation uses the .hash function on the
   X509::Certificate object, which varies by object instance, not actual
   cert contents. Although the PEM representation of a cert could be
   used for comparison, DER generates smaller objects for comparison.

 - Fix the errant unit test that used the same cert instances (which
   carry the same .hash value), to use certs with duplicate data, but
   stored in different object instances.  This removes false positive
   results. This properly models how duplicate certificates appear in
   the system under test.

 - Duplicate detection is necessary to perform in Puppet, because Rubys
   OpenSSL wrapper provides no ability to enumerate the store, or query
   if a cert has been added already. The code simply fails if dupes are
   added:

https://github.com/ruby/ruby/blob/ruby_2_1/ext/openssl/ossl_x509store.c#L303-L316

   That code delegates to the OpenSSL function X509_STORE_add_crl, which
   ultimately uses X509_OBJECT_retrieve_match to determine if a given
   cert matches any in the store already:

https://github.com/openssl/openssl/blob/OpenSSL_1_0_2g/crypto/x509/x509_lu.c#L572-L597

  An attempt was made to see if FFI wrappers could be used over top of
  these underlying functions, but the level of effort required was not
  justified given the simpler Ruby solution presented here.

Paired with: Ethan Brown <ethan@puppet.com> (@Iristyle)

lib/puppet/util/monkey_patches.rb

@@ -121,7 +121,8 @@ def set_default_paths
       # cert store, see https://rt.openssl.org/Ticket/Display.html?id=2158
       unless @puppet_certs_loaded
         @puppet_certs_loaded = true
-        Puppet::Util::Windows::RootCerts.instance.to_a.uniq.each do |x509|
+
+        Puppet::Util::Windows::RootCerts.instance.to_a.uniq { |cert| cert.to_der }.each do |x509|
           begin
             add_cert(x509)
           rescue OpenSSL::X509::StoreError => e

spec/unit/util/monkey_patches_spec.rb

@@ -177,8 +177,9 @@ def do_test( range, other, expected )
 
 
 describe OpenSSL::X509::Store, :if => Puppet::Util::Platform.windows? do
-  let(:store) { described_class.new }
-  let(:cert)  { OpenSSL::X509::Certificate.new(File.read(my_fixture('x509.pem'))) }
+  let(:store)    { described_class.new }
+  let(:cert)     { OpenSSL::X509::Certificate.new(File.read(my_fixture('x509.pem'))) }
+  let(:samecert) { cert.dup() }
 
   def with_root_certs(certs)
     Puppet::Util::Windows::RootCerts.expects(:instance).returns(certs)
@@ -199,9 +200,12 @@ def with_root_certs(certs)
   end
 
   it "ignores duplicate root certs" do
-    with_root_certs([cert, cert])
+    # prove that even though certs have identical contents, their hashes differ
+    expect(cert.hash).to_not eq(samecert.hash)
+    with_root_certs([cert, samecert])
 
     store.expects(:add_cert).with(cert).once
+    store.expects(:add_cert).with(samecert).never
 
     store.set_default_paths
   end