[Pki-devel] [PATCH] TRAC Ticket #667 - provide option for ca-less drm install [20131001]
Ade Lee
alee at redhat.com
Fri Oct 4 17:34:45 UTC 2013
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:
> Initial comments ..
>
> 1. ConfigurationUtils.java- I think we can simplify this code a bit.
>
> Instead of:
>
> boolean standalone = false;
> if (sysType.equals("KRA")) {
> standalone = config.getBoolean("kra.standalone", false);
> } else if (sysType.equals("OCSP")) {
> standalone = config.getBoolean("ocsp.standalone", false);
> }
>
> if ((sysType.equals("KRA") && standalone) ||
> (sysType.equals("OCSP") && standalone)) {
> // Treat Standalone KRA/OCSP the same as "otherca"
> config.putString(subsystem + "." + certTag + ".cert",
> "...paste certificate here...");
> } else {
>
> we could just have:
> boolean standalone = config.getBoolean(sysType.lower() + ".standalone", false);
> if (standalone) {
> // Treat standalone subsystem the same as "otherca"
> config.putString(subsystem + "." + certTag + ".cert",
> "...paste certificate here...");
> } else {
>
> 2. In SystemConfigService.java, you use a config entry to determine
> whether the system is a standalone parameter or not. That means reading
> the CS.cfg in validateData(). I think it makes better sense to add a
> field to ConfigurationRequest to indicate that the system being
> installed is a standalone system. This also eliminates the need for a
> global variable standalone. Instead, you can just reference
> data.getStandalone()
>
> 3. Its a little weird in the validateData() to be putting the
> validation under the check for new_domain. What happens if someone
> selects standalone and existing domain?
>
> 4. After validateData() has run, its not necessary to check whether we
> are an OCSP or KRA when running standalone. That check should have been
> done by validateData()
>
> 5. At the beginning of the configuration, in step 2, you append
> "signing" to the beginning of the cert.list. That will not work for
> OCSP - which already contains "signing" as the first parameter in its
> list. Maybe you want something unique like "external_signing" ?
>
> 6. Did you confirm that all these servlets/mappings are actually needed
> to be exposed in web.xml?
>
> To be continued ..
>
> On Tue, 2013-10-01 at 16:52 -0700, Matthew Harmsen wrote:
> > RESENT to include the DATE of this PATCH in the Subject line.
> >
> > The attached patch addresses the following TRAC ticket:
> > * https://fedorahosted.org/pki/ticket/667 TRAC Ticket #667 -
> > provide option for ca-less drm install
> >
> > Unlike the previous patch which did not utilize a security domain and
> > utilized the legacy GUI panel configuration, this patch only pertains
> > to the non-GUI 'pkispawn' installation/configuration process as
> > documented at:
> >
> >
> > * http://pki.fedoraproject.org/wiki/Stand-alone_PKI_Subsystems
> >
> > Using this code, I have successfully installed a stand-alone DRM
> > utilizing a separate PKI CA as my external CA for testing purposes.
> >
> >
> > Should this code be approved, I will add the following:
> >
> >
> > * update the 'pkispawn' man page
> > * add similar default values as parameters to OCSP
> >
> > At this stage, this code has not been tested to see if a DRM can be
> > successfully cloned from a Stand-Alone DRM.
> >
> >
> > -- Matt
> >
> > _______________________________________________
> > Pki-devel mailing list
> > Pki-devel at redhat.com
> > https://www.redhat.com/mailman/listinfo/pki-devel
>
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
More information about the Pki-devel
mailing list