[Pki-devel] [PATCH] TRAC Ticket #667 - provide option for ca-less drm install [20131011]
Matthew Harmsen
mharmsen at redhat.com
Sat Oct 12 01:08:33 UTC 2013
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20131011/7512b4c0/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20131011-Stand-alone-DRM.patch
Type: text/x-patch
Size: 178737 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20131011/7512b4c0/attachment.bin>
More information about the Pki-devel
mailing list