[MSYS-545] Add code to delete certificate from cert_store. #10
Conversation
NAshwini
force-pushed
the
ash/delete_single_file_certificate
branch
from
bde9dd4 to
c10384e
Compare
lib/win32/certstore.rb
Outdated
| def delete(certificate_name) | ||
| delete_cert = cert_delete(@certstore_handler, certificate_name) | ||
| close | ||
| return delete_cert |
There was a problem hiding this comment.
I realize this is the existing pattern, but we don't need an explicit return here when this is the last line of the method, i.e. this could just be delete_cert.
Similarly, I don't know that we want to close as part of every action. We could update this pattern so this entire method would just be cert_delete(@certstore_handler, certificate_name).
There was a problem hiding this comment.
Hi @btm, @NimishaS and @mwrock - Please correct me if I'm wrong, as per our discussion in the previous PR: #4 (comment), we discided to make certstore_handler secure and not accessible outside the class. That's why we created close method as private, having only read permission. For each operation of windows certificate store, we have to create a certificate_handler pointer which needs to be destroyed at the end of request. close method destroys the pointer.
So if we want to list certificates and add certificate, we have to perform the operations separately in two different blocks and with 2 different certstore_handlers. We can't perform two operations on certificate store with one certstore_handler.
lib/win32/certstore.rb
Outdated
| @@ -54,6 +54,12 @@ def add(cert_file_path) | |||
| return add | |||
| end | |||
|
|
|||
| def delete(certificate_name) | |||
| delete_cert = cert_delete(@certstore_handler, certificate_name) | |||
There was a problem hiding this comment.
when we have an attr_reader we can just call certstore_handler instead of @certstore_handler. This allows a future refactor where we could replace the reader with a method if we wanted to do something else.
lib/win32/certstore/store_base.rb
Outdated
| 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_name.read_wstring.downcase == certificate_name.downcase) | ||
| if CertDeleteCertificateFromStore(CertDuplicateCertificateContext(pCertContext)) |
There was a problem hiding this comment.
Instead of using CertEnumCertificatesInStore which lists all the certificates and looping over it, we should use some function to find the particular certificate. e.g. CertFindCertificateInStore function gives the pointer to the cert_context structure, which can be passed to CertDeleteCertificateFromStore.
This is a slightly big change.
| it "return message of successful deletion" do | ||
| store = certstore.open(store_name) | ||
| delete_cert = store.delete(certificate_name) | ||
| expect(delete_cert).to eq("Deleted certificate GeoTrust Global CA successfully") |
There was a problem hiding this comment.
CertEnumCertificatesInStore isn't stubbed so this spec may fail on a system which doesn't have a GeoTrust Global CA certificate ca store. Since appveyor isn't configured, this isn't very clear right now.
Anyway, if we decide to stop using CertEnumCertificatesInStore as per my previous comment, then the specs will change.
spec/win32/unit/certstore_spec.rb
Outdated
| allow(certbase).to receive(:CertDeleteCertificateFromStore).and_return(false) | ||
| allow(FFI::LastError).to receive(:error).and_return(-2147024891) | ||
| store = certstore.open(store_name) | ||
| expect { store.delete(certificate_name) }.to raise_error(Chef::Exceptions::Win32APIError) |
There was a problem hiding this comment.
If GlobalSign certificate isn't present on a system, this may fail.
Signed-off-by: NAshwini <[email protected]>
Signed-off-by: NAshwini <[email protected]>
NAshwini
force-pushed
the
ash/delete_single_file_certificate
branch
from
46d615d to
c42d86d
Compare
Signed-off-by: NAshwini <[email protected]>
Added code to delete single certificate from cert_store.
We can delete the certificate & check as: