[MSYS-543] [MSYS-546] Added feature to list certificates and add new certificate. #4

piyushawasthi · 2017-08-17T09:53:11Z

This PR have following feature.
1: Adding certificate in certificate store.
2: Listing certificates from any certificate store.

Signed-off-by: piyushawasthi [email protected]

NimishaS

@btm, @mwrock I have suggested some design related changes here. Please share your views on them.

NimishaS · 2017-08-23T11:58:58Z

lib/win32/backend/core_ext.rb

@@ -0,0 +1,83 @@
+#


Can we use some other name for this folder than backend? Since we don't have an architecture of frontend and backend, this name looks a bit confusing.
I would also prefer if the names of files match with that of Chef's. E.g. Chef has crypto.rb file for defining the cryptography windows functions: https://github.com/chef/chef/blob/master/lib/chef/win32/api/crypto.rb

@btm, @mwrock , please share your views.

Agree there are better names than backend. Maybe "store"

Thanks @mwrock

NimishaS · 2017-08-23T12:02:21Z

lib/win32/backend/policies.rb

@@ -0,0 +1,44 @@
+#


This file contains methods for validation. @btm, @mwrock , please share your views if the file name policies sounds good to you.

maybe assertions?

NimishaS · 2017-08-23T12:07:45Z

lib/win32/backend/policies.rb

+module Win32::Backend::Policies
+
+  # Validate import certificate file format
+  def validate_args(import_cert_params)


Right now this method is not being used, but I suppose this will be used while certificate add functionality. Since this validates the certificate, would it be good to name it as validate_certificate as we have validate_store below?

NimishaS · 2017-08-23T12:17:20Z

lib/win32/certstore/certificate/certificate.rb

+        return certificates_list
+      end
+
+      def list_cert


It would probably be a good idea to keep all the functions calling Windows functions in a separate file which could be kept in the folder currently named as backend.
That way we'll have only functions like list, add, delete, verify etc defined for the Certificate class and all the Windows function related definition and implementation will be in a separate folder.

@btm, @mwrock , please comment.

agree and especially if you rename backend to store, I'd expect it to expose list

NimishaS · 2017-08-23T12:21:10Z

lib/win32/certstore/certificate/certificate.rb

+      include Certificate::Win32Base
+
+      def list(store_name)
+        @store_name = store_name


Can we use store_name directly here instead of reassigning it to @store_name?

NimishaS · 2017-08-23T12:31:04Z

lib/win32/certstore/certificate/win32_base.rb

+        def get_PScommand(user_parms)
+          user_parms = update_params(user_parms)
+          if user_parms.last =~ /.pfx/
+            cmd = "powershell.exe -Command certutil -addstore -f -importpfx '#{user_parms.first}' '#{user_parms.last}'"


We aren't using certutil powershell utility for listing certificate. This function is also not being called. We can probably remove those functions which are not related to listing certificates as we aren't sure if we'll be using certutil.

NimishaS · 2017-08-23T12:34:13Z

lib/win32/certstore/certstore.rb


-    def open store_name


We should have open and close methods in this file for Certstore instead of win32_base file, probably with different names if open and close are too general.

NimishaS · 2017-08-23T12:36:51Z

lib/win32/certstore/certstore.rb

-      end
-      certstore_handle
+    def self.add_cert(*args)
+      Win32::Certstore::Certificate::Add.new(args)


Since this PR is for only list_cert, please remove all the methods that aren't related to this functionality.

NimishaS · 2017-08-23T12:39:10Z

lib/win32/certstore/certstore.rb

-      end
-      closed
+    def self.list_cert(certstore_name)
+      Win32::Certstore::Certificate.new.list(certstore_name)


It would be a good practice to have an initialize method for this class where we can have an instance variable defined for Win32::Certstore::Certificate.new. Otherwise we'll have to call Win32::Certstore::Certificate.new multiple times for different functions like add_cert, delete_cert etc.

NimishaS · 2017-08-23T12:42:30Z

spec/win32/unit/certstore_spec.rb

-    expect(Win32::Certstore::VERSION).not_to be nil
-  end
-
-  describe "#open" do


Please update the specs for certstore as per the code changes. They shouldn't be commented out.

NimishaS · 2017-08-23T12:49:28Z

We should add the test coverage for the newly added files too e.g. win32_api, policies etc.

NimishaS · 2017-08-24T13:56:04Z

lib/win32/certstore/certstore.rb


-    def open store_name
+    def self.open_store(store_name)


Most of the changes look good to me. Just 2 doubts:

All the methods defined inside Certstore are class methods. Does this bind us to define only class methods in Certstore in future?

After renaming the method user will call Win32::Certificate.open_store(certstore_name). It would look better to call Win32::Certificate.open(certstore_name). Here if it's a Class method, it won't override Ruby's open method since this will always be called along with the Class name.

@btm, @mwrock please suggest if this looks fine or some modifications should be done.

I think a class method of open could instantiate an instance of store and return it. One would call list and other instance methods of of that. open could also take a block so you could have:

Win32::Certstore.open("MY") do |store| store.list end

If called with a block, you could call close on the store. It might also be worth closing the store in a finalizer.

NimishaS · 2017-08-24T15:20:13Z

Thanks for quickly working on the review comments @piyushawasthi . Overall very good work!
I have just few doubts which I shared above. Please share a sample code to show how the functions for certstore and certificate will be called along with a sample output.
This will help in understanding the usage. @btm, @mwrock please share your views after @piyushawasthi 's comment.

mwrock · 2017-08-25T06:22:36Z

lib/win32/certstore/certstore.rb


-    def open store_name
+    def self.open_store(store_name)


I think a class method of open could instantiate an instance of store and return it. One would call list and other instance methods of of that. open could also take a block so you could have:

Win32::Certstore.open("MY") do |store| store.list end

If called with a block, you could call close on the store. It might also be worth closing the store in a finalizer.

Signed-off-by: piyushawasthi <[email protected]>

NimishaS · 2017-08-28T12:42:07Z

@mwrock , currently Win32::Certstore.open uses CertOpenSystemStoreW and returns a handler to the certificate. According to your #4 (comment), it should return an object of CertStore.
Are you suggesting to modify the definition of Win32::Certstore.list_cert with:

Win32::Certstore.open("MY") do |store| 
    store.list
end

?

mwrock · 2017-08-28T16:00:11Z

That's correct. Ideally we would not want a user of this API to have to deal with handles directly.

piyushawasthi · 2017-08-31T14:46:30Z

@mwrock - Made changes as per your comment please review also if you get chance please review directory structure and list certificate calling way details here: https://gist.github.com/piyushawasthi/76fad63c0fe8d6cd3cc39cda94ada505

mwrock · 2017-08-31T17:39:17Z

lib/win32/certstore/certificate/cert_base.rb

+        include Chef::Mixin::ShellOut
+        include Chef::Mixin::WideString
+
+        def list_cert(store_handle)


I don't think list_cert belongs here. Since this is the Certificate class I would expect this to represent a single certificate instance. I'd expect to list certs using a CertStore

mwrock · 2017-08-31T17:41:37Z

lib/win32/certstore/certificate/certificate.rb

+      # To Display Certificate List
+      # Take input certificate store name
+      # Return List in JSON format
+      def list


see note above. I'd expect to use the certificate store to list certs and not a Certificate class. Since this is the only method here, I wonder if we need this class at all.

@mwrock , yeah list may be moved to CertStore since it doesn't represent a single certificate instance. But there will be other methods for adding, deleting, verifying certificates, etc which will make sense in this class. Please correct me if I missing something.

I still think methods like adding and deleting certs from a store belong in the store and would take a cert instance.

mwrock · 2017-08-31T17:46:09Z

lib/win32/certstore/certstore.rb


-    def open store_name


I think open and close should remain in certstore. Open should be a class method that returns (or yields) a store.

I too agree that open and close methods should be in certstore. But @mwrock, if open will be a class method that returns a certstore object, then that object will contain the certificate handler and users will have access to it.

I think the handle can be hidden as a private member. Alternatively you could hide open and close completely. Internally you would have public store operations like list check for a handle and call a private instance open if there isn't one. For closing, you could use ObjectSpace.define_finalizer so that it is automatically freed when it is GC'd. This is the pattern I used in winrm and it worked well: https://github.com/WinRb/WinRM/blob/master/lib/winrm/shells/base.rb#L171

NimishaS · 2017-09-01T08:10:48Z

@piyushawasthi , it would be helpful if we work on a design first taking @mwrock's comments into consideration. We should elaborate the file structure along with the methods (class or instance) that the file contains. We should highlight the Classes and Modules too. Once @mwrock or @btm approves it then we can go ahead with the implementation.

piyushawasthi · 2017-09-01T09:24:10Z

@NimishaS - Ya, That's work, We'll do pairing for better design.

Signed-off-by: piyushawasthi <[email protected]>

NimishaS · 2017-09-19T12:41:36Z

lib/win32/certstore/certstore.rb

+
+    def open(store_name)
+      certstore_handler = CertOpenSystemStoreW(nil, wstring(store_name))
+      unless certstore_handler
        last_error = FFI::LastError.error


Can we use lookup_error method here as well?

open and close are the private methods of certstore.

Makes sense.

NimishaS · 2017-09-19T12:41:45Z

lib/win32/certstore/certstore.rb

-    def close certstore_handle
-      closed = CertCloseStore(certstore_handle, CERT_CLOSE_STORE_FORCE_FLAG)
+    def close
+      closed = CertCloseStore(@certstore_handler, CERT_CLOSE_STORE_FORCE_FLAG)
      unless closed
        last_error = FFI::LastError.error


Can we use lookup_error method here as well?

NimishaS · 2017-09-19T12:48:01Z

lib/win32/certstore/store_base.rb

+          end
+          CertFreeCertificateContext(pCertContext)
+        rescue Exception => e
+          @error = "load"


Since @error actually holds the operation which fails here, it would be good to name this variable as @failed_operation. Also it seems like the operation is list in this case. Is there any reason to use load?

I would suggest to pass failed_operation as an argument to lookup_error method with default value as an empty string. By doing so someone can easily understand by looking at the method ( e.g. lookup_error(failed_operation = '') ) that the function accepts operation too.
Right now one has to look at the complete definition of the method to understand that @error instance variable can also be passed.

NimishaS · 2017-09-19T12:55:34Z

lib/win32/certstore/store_base.rb

+        begin
+          if (CertAddEncodedCertificateToStore(store_handler, X509_ASN_ENCODING, pointer_cert, cert_length, 2, nil))
+            return "Added certificate #{File.basename(cert_path)} successfully"
+          else


Do we need this If-Else block for exception handling, since we are already using a rescue block?

NimishaS · 2017-09-19T13:03:08Z

lib/win32/certstore/store_base.rb

+        when -2146881278
+          raise Chef::Exceptions::Win32APIError, "ASN1 unexpected end of data.  "
+        else
+          raise Chef::Exceptions::Win32APIError, "Unable to #{@error} certificate with error: #{last_error}."


Nitpicking... Although this message is absolutely correct, I have one suggestion. I would suggest to use Failed instead of Unable. The reason behind this is that when I am debugging a big log file, I search for keywords like error, fail etc. This highlights the issue very clearly. It's unlikely to search for unable keyword.

NimishaS · 2017-09-19T13:17:26Z

spec/win32/unit/certstore/certstore_spec.rb

+        allow(certbase).to receive(:CertAddEncodedCertificateToStore).and_return(false)
+        allow(FFI::LastError).to receive(:error).and_return(-2146885628)
+        store = certstore.open(store_name)
+        expect { store.add(cert_file_path) }.to raise_error(Chef::Exceptions::Win32APIError)


It would be good to check this with the actual error message.

NimishaS · 2017-09-19T13:17:36Z

spec/win32/unit/certstore/certstore_spec.rb

+        allow(certbase).to receive(:CertAddEncodedCertificateToStore).and_return(false)
+        allow(FFI::LastError).to receive(:error).and_return(-2146885629)
+        store = certstore.open(store_name)
+        expect { store.add(cert_file_path) }.to raise_error(Chef::Exceptions::Win32APIError)


It would be good to check this with the actual error message

NimishaS · 2017-09-19T13:17:42Z

spec/win32/unit/certstore/certstore_spec.rb

+        allow(certbase).to receive(:CertAddEncodedCertificateToStore).and_return(false)
+        allow(FFI::LastError).to receive(:error).and_return(-2146881269)
+        store = certstore.open(store_name)
+        expect { store.add(cert_file_path) }.to raise_error(Chef::Exceptions::Win32APIError)


It would be good to check this with the actual error message

NimishaS · 2017-09-19T13:17:47Z

spec/win32/unit/certstore/certstore_spec.rb

+        allow(certbase).to receive(:CertAddEncodedCertificateToStore).and_return(false)
+        allow(FFI::LastError).to receive(:error).and_return(-2146881278)
+        store = certstore.open(store_name)
+        expect { store.add(cert_file_path) }.to raise_error(Chef::Exceptions::Win32APIError)


It would be good to check this with the actual error message

NimishaS · 2017-09-19T13:18:58Z

spec/win32/unit/store/assertions_spec.rb

+    context "When passing invalid certificate store name" do
+      let (:store_name) { "Chef" }
+      it "Raise ArgumentError" do
+        expect { certstore.validate_store(store_name) }.to raise_error(ArgumentError)


If we are manually raising some error anywhere, we should check with the actual error message instead of ArgumentError

Signed-off-by: piyushawasthi <[email protected]>

piyushawasthi · 2017-09-27T09:18:42Z

How to use win32-certstore.

1: Listing certificates of root certificate store.

store = Win32::Certstore.open("Root")
store.list

or

Win32::Certstore.open("Root") do |store|
    store.list
end

2: Adding certificate to root certificate store.

store = Win32::Certstore.open("Root")
store.add(certificate_file_path)

or

Win32::Certstore.open("Root") do |store|
   store.add(certificate_file_path)
end

btm · 2017-09-28T00:12:35Z

can you but the above examples in a README.md?

What's the thinking behind the lib/win32/win32-certstore/version.rb file pattern? why not lib/win32/certstore/version.rb

mwrock · 2017-09-28T22:26:27Z

I'd consider using a finalizer for the certstore_handle. Unless you are using the store inside of a block, the store will remain open for the duration of the process. What happens if you try to open a new instance of the store without closing the other? If it throws an error related to it already being open, we should handle that and emit a clear message instructing the user to call close first on any other instances. I'd also add examples that call close in the readme.

Signed-off-by: piyushawasthi <[email protected]>

NimishaS · 2017-11-09T06:39:17Z

@mwrock , @btm , please review.

btm · 2017-11-14T15:50:06Z

lib/win32-certstore.rb

@@ -15,4 +16,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

-require 'win32/certstore/certstore'
+require "mixin/crypto"
+require "certstore/certstore"


I wonder why this is certstore/certstore here ?

I'd suggest being more specific here, i.e. use `require "win32/certstore/certstore" so we don't risk picking up something else in the LOAD_PATH called 'certstore'.

btm · 2017-11-14T15:50:59Z

lib/win32/win32-certstore/version.rb

@@ -0,0 +1,6 @@
+module Win32
+  module Win32Certstore
+    VERSION = "1.0.0"


please change this to 0.1.0 so we can make some pre-releases with non-prerelease versions. we'll ship 1.0.0 when we think we're feature complete.

btm · 2017-11-14T15:56:20Z

spec/win32/unit/certstore/certstore_spec.rb

+      it "returns no certificate list" do
+        allow(certbase).to receive(:CertAddEncodedCertificateToStore).and_return(false)
+        store = certstore.open(store_name)
+        expect { store.add(cert_file_path) }.to raise_error(Chef::Exceptions::Win32APIError)


this test and all tests after it fail on my Windows desktop when running 'rake spec' with an error like:

1) Win32::Certstore#list When adding invalid certificate returns no certificate list Failure/Error: expect { store.add(cert_file_path) }.to raise_error(Chef::Exceptions::Win32APIError) expected Chef::Exceptions::Win32APIError, got #<Errno::ENOENT: No such file or directory @ rb_sysopen - C:\Users\btm_000\AppData\Local\Temp\TempCert.der> with backtrace: # C:/Users/btm_000/Documents/Github/win32-certstore/lib/win32/certstore/store_base.rb:90:in `read' # C:/Users/btm_000/Documents/Github/win32-certstore/lib/win32/certstore/store_base.rb:90:in `read_certificate_content' # C:/Users/btm_000/Documents/Github/win32-certstore/lib/win32/certstore/store_base.rb:47:in `cert_add' # ./lib/win32/certstore/certstore.rb:51:in `add' # ./spec/win32/unit/certstore/certstore_spec.rb:68:in `block (5 levels) in <top (required)>' # ./spec/win32/unit/certstore/certstore_spec.rb:68:in `block (4 levels) in <top (required)>' # ./spec/win32/unit/certstore/certstore_spec.rb:68:in `block (4 levels) in <top (required)>'

btm · 2017-11-14T16:02:10Z

lib/win32/certstore/store_base.rb

+      # A certificate can be converted with `openssl x509 -in example.crt -out example.der -outform DER`
+      def read_certificate_content(cert_path)
+        unless (File.extname(cert_path) == ".der")
+          temp_file = shell_out("powershell.exe -Command $env:temp").stdout.strip.concat("\\TempCert.der")


Use Tempfile.new here. This will:

use the temp directory without having to shell out to get it (using ENV['TEMP'] in Ruby would have been more appropriate here)

produce a temp file with a random string inserted so you don't have to worry about the existence of a file, or a security issue with someone maliciously creating a temporary file with that filename, etc.

btm · 2017-11-14T16:34:44Z

lib/win32/mixin/crypto.rb

+require 'mixlib/shellout'
+
+module Win32
+  module Mixin


can you move this to Win32::Crypto::Mixin please? one more directory. Nothing here is meant to be shared with other Win32 libraries so let's use our own namespace.

btm · 2017-11-15T17:00:50Z

I made some changes and merged this via #6.

piyushawasthi · 2017-11-17T06:43:48Z

thanks @btm

piyushawasthi requested review from NimishaS and btm August 17, 2017 09:53

piyushawasthi force-pushed the piyush/single_file_cert_list branch from a04049f to eee4766 Compare August 17, 2017 10:17

piyushawasthi changed the title ~~One file to store all the certificates function~~ Certificates List Aug 17, 2017

piyushawasthi force-pushed the piyush/single_file_cert_list branch 3 times, most recently from 690c4ab to cc77d44 Compare August 17, 2017 12:16

NimishaS suggested changes Aug 23, 2017

View reviewed changes

NimishaS requested a review from mwrock August 23, 2017 12:44

piyushawasthi force-pushed the piyush/single_file_cert_list branch 6 times, most recently from 94f0826 to 1da21be Compare August 24, 2017 13:00

NimishaS suggested changes Aug 24, 2017

View reviewed changes

mwrock requested changes Aug 25, 2017

View reviewed changes

One file to store all the certificate related function

deea2dc

Signed-off-by: piyushawasthi <[email protected]>

piyushawasthi force-pushed the piyush/single_file_cert_list branch from 1da21be to deea2dc Compare August 28, 2017 05:58

piyushawasthi mentioned this pull request Aug 29, 2017

[In-progress] Added feature to add certificate in certificate store. #5

Closed

mwrock requested changes Aug 31, 2017

View reviewed changes

Update code to accept &block and call

5ac4928

Signed-off-by: piyushawasthi <[email protected]>

piyushawasthi force-pushed the piyush/single_file_cert_list branch from 1b2be0f to 5ac4928 Compare September 4, 2017 09:19

NimishaS suggested changes Sep 19, 2017

View reviewed changes

piyushawasthi force-pushed the piyush/single_file_cert_list branch 5 times, most recently from a939cf3 to a60258e Compare September 26, 2017 10:38

Add certificate in certificate store

3dda9f7

Signed-off-by: piyushawasthi <[email protected]>

piyushawasthi force-pushed the piyush/single_file_cert_list branch from a60258e to 3dda9f7 Compare September 26, 2017 11:04

NimishaS approved these changes Sep 26, 2017

View reviewed changes

piyushawasthi requested review from btm and mwrock September 27, 2017 09:19

piyushawasthi force-pushed the piyush/single_file_cert_list branch 4 times, most recently from e2550d3 to 64213a7 Compare October 9, 2017 13:21

Update README

393454c

Signed-off-by: piyushawasthi <[email protected]>

piyushawasthi force-pushed the piyush/single_file_cert_list branch from 64213a7 to 393454c Compare October 9, 2017 13:22

Added finalizer in certstore

f30b3a4

Signed-off-by: piyushawasthi <[email protected]>

piyushawasthi changed the title ~~Certificates List~~ [MSYS-543] [MSYS-546] Added feature to listing certificates and add new certificate. Nov 9, 2017

piyushawasthi changed the title ~~[MSYS-543] [MSYS-546] Added feature to listing certificates and add new certificate.~~ [MSYS-543] [MSYS-546] Added feature to list certificates and add new certificate. Nov 9, 2017

btm suggested changes Nov 14, 2017

View reviewed changes

btm mentioned this pull request Nov 15, 2017

set initial file hierarchy #6

Merged

btm merged commit f30b3a4 into chef:master Nov 15, 2017

piyushawasthi mentioned this pull request Nov 23, 2017

[MSYS-545] Add code to delete certificate from cert_store. #10

Merged

[MSYS-543] [MSYS-546] Added feature to list certificates and add new certificate. #4

[MSYS-543] [MSYS-546] Added feature to list certificates and add new certificate. #4

Conversation

piyushawasthi commented Aug 17, 2017 • edited Loading

NimishaS 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

NimishaS Aug 23, 2017 • 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

NimishaS commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NimishaS commented Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

NimishaS commented Aug 28, 2017

mwrock commented Aug 28, 2017

piyushawasthi commented Aug 31, 2017

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

NimishaS commented Sep 1, 2017

piyushawasthi commented Sep 1, 2017

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

Choose a reason for hiding this comment

piyushawasthi commented Sep 27, 2017

btm commented Sep 28, 2017

mwrock commented Sep 28, 2017

NimishaS commented Nov 9, 2017

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

btm commented Nov 15, 2017

piyushawasthi commented Nov 17, 2017

piyushawasthi commented Aug 17, 2017 •

edited

Loading

NimishaS Aug 23, 2017 •

edited

Loading

NimishaS commented Aug 24, 2017 •

edited

Loading