[Pki-devel] [PATCH] TRAC Ticket #667 - provide option for ca-less drm install [20131011]
Ade Lee
alee at redhat.com
Tue Oct 15 13:59:03 UTC 2013
1. In SystemConfigService.java, you throw an exception if
throw new PKIException("No data for '" + ct + "' was found!");
This should really be a BadRequestException()
2. In that same section, you add a large section of code to be executed
if its the second step of standalone install. In this case, it would
probably be better to extract this logic into an appropriately named
helper function or two. In fact, it likely should be code that resides
in ConfigurationUtils, so that it can be reused by the config wizard
later.
3. In pkihelper.py, you have added a few helper functions like
confirmSubordinatePKI() . These make the code much more readable.
These should be named confirm_subordinate_pki(), or maybe even just
confirm_subordinate() to match with existing and PEP 8 convention.
Also, to be consistent, I would use confirm_file_exists(),
confirm_file_missing(), confirm_data_exists()
4. Just a comment, but much of this will have to change when we permit
support of cloned standalone DRM. Those, for example, will need to be
able to join an existing security domain, and will need to provide a
security domain password. In light of that, I wonder if putting in all
this logic to prevent all that is a good idea at this stage. I suppose
we can fix it and simplify then.
Other than that, it looks like you addressed most of the issues that
were mentioned before.
Ade
On Fri, 2013-10-11 at 18:08 -0700, Matthew Harmsen wrote:
> Please review the attached patch. Once again, this patch only
> pertains to the non-GUI 'pkispawn' installation/configuration process
> as documented at:
> * http://pki.fedoraproject.org/wiki/Stand-alone_PKI_Subsystems
> This "re-factored" patch has been re-based with the 'master' and
> addresses most of the suggestions submitted below; those portions
> which have not yet been addressed (#6, #11, and the object variable
> portion of #13) have been encapsulated in the following TRAC ticket:
> * https://fedorahosted.org/pki/ticket/762 TRAC Ticket #762 -
> Stand-alone DRM (cleanup tasks)
> Using this code, I have successfully installed a stand-alone DRM
> utilizing a separate PKI CA as my external CA for testing purposes.
> The 'master' re-base was performed after successful completion of this
> test.
>
> At this stage, this code has still not been tested to see if a DRM can
> be successfully cloned from a Stand-Alone DRM.
>
> On 10/04/13 10:34, Ade Lee wrote:
>
> > 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