Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Nov 15, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ build-iPhoneSimulator/

# for a library or gem, you might want to ignore these files since the code is
# intended to run in multiple environments; otherwise, check them in:
# Gemfile.lock
Gemfile.lock
# .ruby-version
# .ruby-gemset

Expand Down
3 changes: 2 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ rvm:
branches:
only:
- master
before_install: gem install bundler -v 1.12.5
before_install:
- gem install bundler
script: bundle exec rake spec
2 changes: 2 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@ source 'https://rubygems.org'

# Specify your gem's dependencies in win32-certstore.gemspec
gemspec

gem 'rb-readline'
5 changes: 4 additions & 1 deletion lib/win32-certstore.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
$LOAD_PATH.unshift File.expand_path('../../lib/win32', __FILE__)
#
# Author:: Nimisha Sharad (<[email protected]>)
# Copyright:: Copyright (c) 2017 Chef Software, Inc.
Expand All @@ -15,4 +16,6 @@
# See the License for the specific language governing permissions and
# limitations under the License.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should keep the license header here.


require 'win32/certstore/certstore'
require "mixin/crypto"
require "certstore/certstore"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this is certstore/certstore here ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

require "win32-certstore/version"
43 changes: 0 additions & 43 deletions lib/win32/api/reserved_names.rb

This file was deleted.

55 changes: 44 additions & 11 deletions lib/win32/certstore/certstore.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,30 +15,63 @@
# See the License for the specific language governing permissions and
# limitations under the License.

require "win32/certstore/version"
require 'win32/api/reserved_names'
require 'mixin/crypto'
require 'mixin/assertions'
require_relative 'store_base'

module Win32
module Certstore
include Win32::API::ReservedNames
class Certstore
include Win32::Mixin::Crypto
extend Win32::Mixin::Assertions
include Chef::Mixin::WideString
include Win32::Certstore::StoreBase

def open store_name

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

certstore_handle = CertOpenSystemStoreW(nil, wstring(store_name))
unless certstore_handle
attr_reader :store_name

def initialize(store_name)
@certstore_handler = open(store_name)
end

def self.open(store_name)
validate_store(store_name)
if block_given?
yield self.new(store_name)
else
self.new(store_name)
end
end

def list
list = cert_list(@certstore_handler)
close
return list
end

def add(cert_file_path)
add = cert_add(@certstore_handler, cert_file_path)
close
return add
end

private

attr_reader :certstore_handler

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use lookup_error method here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open and close are the private methods of certstore.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

raise Chef::Exceptions::Win32APIError, "Unable to open the Certificate Store `#{store_name}` with error: #{last_error}."
end
certstore_handle
certstore_handler
end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use lookup_error method here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same above

raise Chef::Exceptions::Win32APIError, "Unable to close the Certificate Store with error: #{last_error}."
end
closed
end
end
end
95 changes: 95 additions & 0 deletions lib/win32/certstore/store_base.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
#
# Author:: Piyush Awasthi (<[email protected]>)
# Copyright:: Copyright (c) 2017 Chef Software, Inc.
# License:: Apache License, Version 2.0
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

require 'mixin/crypto'
require 'openssl'

module Win32
class Certstore
module StoreBase
include Win32::Mixin::Crypto
include Win32::Mixin::Assertions
include Chef::Mixin::WideString
include Chef::Mixin::ShellOut

def cert_list(store_handler)
cert_name = FFI::MemoryPointer.new(2, 128)
cert_list = []
begin
while (pCertContext = CertEnumCertificatesInStore(store_handler, pCertContext) and not pCertContext.null? ) do
if (CertGetNameStringW(pCertContext, CERT_NAME_FRIENDLY_DISPLAY_TYPE, CERT_NAME_ISSUER_FLAG, nil, cert_name, 1024))
cert_list << cert_name.read_wstring
end
end
CertFreeCertificateContext(pCertContext)
rescue Exception => e
lookup_error("list")
end
cert_list.to_json
end

def cert_add(store_handler, cert_file_path)
validate_certificate(cert_file_path)
file_content = read_certificate_content(cert_file_path)
pointer_cert = FFI::MemoryPointer.from_string(file_content)
cert_length = file_content.bytesize
begin
if (CertAddEncodedCertificateToStore(store_handler, X509_ASN_ENCODING, pointer_cert, cert_length, 2, nil))
"Added certificate #{File.basename(cert_file_path)} successfully"
else

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lookup_error
end
rescue Exception => e
lookup_error("add")
end
end

private

def lookup_error(failed_operation = nil)
last_error = FFI::LastError.error
case last_error
when 1223
raise Chef::Exceptions::Win32APIError, "The operation was canceled by the user."
when -2146885628
raise Chef::Exceptions::Win32APIError, "Cannot find object or property."
when -2146885629
raise Chef::Exceptions::Win32APIError, "An error occurred while reading or writing to a file."
when -2146881269
raise Chef::Exceptions::Win32APIError, "ASN1 bad tag value met. -- Is the certificate in DER format?"
when -2146881278
raise Chef::Exceptions::Win32APIError, "ASN1 unexpected end of data."
else
raise Chef::Exceptions::Win32APIError, "Unable to #{failed_operation} certificate with error: #{last_error}."
end
end

# This is a single public certificate in X509 DER format.
# If your certificate has a header and footer line like "---- BEGIN CERTIFICATE ----" then it is in PEM format, not DER format.
# 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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Tempfile.new here. This will:

  1. use the temp directory without having to shell out to get it (using ENV['TEMP'] in Ruby would have been more appropriate here)
  2. 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.

shell_out("powershell.exe -Command openssl x509 -in #{cert_path} -outform DER -out #{temp_file}")
cert_path = temp_file
end
File.read("#{cert_path}")
end

end
end
end
5 changes: 0 additions & 5 deletions lib/win32/certstore/version.rb

This file was deleted.

44 changes: 44 additions & 0 deletions lib/win32/mixin/assertions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#
# Author:: Piyush Awasthi (<[email protected]>)
# Copyright:: Copyright (c) 2017 Chef Software, Inc.
# License:: Apache License, Version 2.0
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

module Win32::Mixin::Assertions

# Validate certificate store name
def validate_store(store_name)
unless valid_store_name.include?(store_name&.upcase)
raise ArgumentError, "Invalid Certificate Store."
end
end

# Validate certificate type
def validate_certificate(cert_file_path)
unless (!cert_file_path.nil? && File.extname(cert_file_path) =~ /.cer|.crt|.pfx|.der/ )
raise ArgumentError, "Invalid Certificate format."
end
end

private

# These Are Valid certificate store name
# CA -> Certification authority certificates.
# MY -> A certificate store that holds certificates with associated private keys.
# ROOT -> Root certificates.
# SPC -> Software Publisher Certificate.
def valid_store_name
["MY", "CA", "ROOT", "SPC"]
end
end
Loading