Re: [Pki-devel] [PATCH] TRAC Ticket #667 - provide option for ca-less drm install [20131001]

7. In default.cfg, why are defaults not provided for the OCSP standalone? We should probably say in the comment there that its not supported yet. 8. In pkihelper, there are a few places where you have conditionals based on if (subsystem = kra or subsytem = ocsp) and standalone) We can probably just simplify those to : if (standalone) ... 9. In pkihelper, you've added a rather large section where we check for the existence of certain parameters. To make this easier to review and to maintain, we should use some helper functions In particular, you could define something like this: def confirmNotEmpty(self, param): if not self.master_dict.has_key(param) or not len(self.master_dict[param]): config.pki_log.error( log.PKIHELPER_UNDEFINED_CONFIGURATION_FILE_ENTRY_2, param, self.master_dict['pki_user_deployment_cfg'], extra=config.PKI_INDENTATION_LEVEL_2) raise Exception( log.PKIHELPER_UNDEFINED_CONFIGURATION_FILE_ENTRY_2 % (param, self.master_dict['pki_user_deployment_cfg'])) Then: if self.master_dict['pki_subsystem'] == "OCSP": # Stand-alone PKI OCSP OCSP Signing Certificate # (External CA Step 2) ** <---- what is this? ** if not self.master_dict.has_key('pki_external_signing_cert_path') or\ not len(self.master_dict['pki_external_signing_cert_path']): config.pki_log.error( log.PKIHELPER_UNDEFINED_CONFIGURATION_FILE_ENTRY_2, "pki_external_signing_cert_path", self.master_dict['pki_user_deployment_cfg'], extra=config.PKI_INDENTATION_LEVEL_2) raise Exception( log.PKIHELPER_UNDEFINED_CONFIGURATION_FILE_ENTRY_2 % ("pki_external_signing_cert_path", self.master_dict['pki_user_deployment_cfg'])) elif not os.path.exists(self.master_dict['pki_external_signing_cert_path']) or\ not os.path.isfile( self.master_dict['pki_external_signing_cert_path']): config.pki_log.error( log.PKI_FILE_MISSING_OR_NOT_A_FILE_1, self.master_dict['pki_external_signing_cert_path'], extra=config.PKI_INDENTATION_LEVEL_2) raise Exception( log.PKI_FILE_MISSING_OR_NOT_A_FILE_1 % "pki_external_signing_cert_path") becomes: if self.master_dict['pki_subsystem'] == "OCSP": # Stand-alone PKI OCSP OCSP Signing Certificate confirmNotEmpty("pki_external_signing_cert_path") if not os.path.exists( ...) You could probably use suitable helper functions for the os.path... part of it as well. We need to try and avoid large blocks of duplicated code. 10. Incidentally, there are places where you have comments like # (External CA Step 1) which are probably from a cut/paste operation and are not correct. 11. You could probably create a similar helper function for when you save the CSRs. 12. You need to create the relevant enterprise users and groups as we discussed. 13. This logic can be simplified : # Security Domain if self.master_dict['pki_subsystem'] != "CA" or\ config.str2bool(self.master_dict['pki_clone']) or\ config.str2bool(self.master_dict['pki_subordinate']): if ((self.master_dict['pki_subsystem'] == "KRA" or self.master_dict['pki_subsystem'] == "OCSP") and config.str2bool(self.master_dict['pki_standalone'])): # Stand-alone PKI KRA/OCSP self.set_new_security_domain(data) else: # PKI KRA, PKI OCSP, PKI RA, PKI TKS, PKI TPS, # CA Clone, KRA Clone, OCSP Clone, TKS Clone, TPS Clone, or # Subordinate CA self.set_existing_security_domain(data) else: # PKI CA or External CA self.set_new_security_domain(data) to: # Security Domain if ((self.master_dict['pki_subsystem'] != "CA" or\ config.str2bool(self.master_dict['pki_clone']) or\ config.str2bool(self.master_dict['pki_subordinate']) and\ (!config.str2bool(self.master_dict['pki_standalone'])): # PKI KRA, PKI OCSP, PKI RA, PKI TKS, PKI TPS, # CA Clone, KRA Clone, OCSP Clone, TKS Clone, TPS Clone, or # Subordinate CA self.set_existing_security_domain(data) else: # PKI CA or External CA or Standalone self.set_new_security_domain(data) Also, as I look at this code, it occurs to me that it would make the code a lot simpler if we simply defined a bunch of object variables for the most commonly used options. So, in the init() method for ConfigClient: def __init__(self, deployer): self.deployer = deployer self.master_dict = deployer.master_dict self.clone = config.str2bool(deployer.master_dict['pki_clone']) self.subsystem = deployer.master_dict['pki_subsystem'] self.subordinate ... The the above becomes: # Security Domain if ((self.subsystem != "CA" or self.clone or self.subordinate) and\ self.standalone): self.set_existing_security_domain(data) else: self.set_new_security_domain(data) We could start simple and do just the ConfigClient class, although the ConfigurationFile class could really use this kind of simplification. If you are worried about confusing things, fix things as is for now - and then create a separate patch above this one that makes the changes. In fact, thats probably a good idea in any case. 14. In pkiparser, finalization and initialization.py, pkispawn - once again - only check for standalone. Ade On Thu, 2013-10-03 at 17:21 -0400, Ade Lee wrote: