6:52 p.m.
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
4:21 p.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
12:34 p.m.
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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
8:08 p.m.
New subject: [PATCH] TRAC Ticket #667 - provide option for ca-less drm install [20131011]
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(a)redhat.com
>> https://www.redhat.com/mailman/listinfo/pki-devel
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel(a)redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
8:59 a.m.
New subject: [PATCH] TRAC Ticket #667 - provide option for ca-less drm install [20131011]
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(a)redhat.com
> > > https://www.redhat.com/mailman/listinfo/pki-devel
> >
> > _______________________________________________
> > Pki-devel mailing list
> > Pki-devel(a)redhat.com
> > https://www.redhat.com/mailman/listinfo/pki-devel
>
3848
days inactive
3862
days old
4 comments
2 participants
participants (2)
-
Ade Lee -
Matthew Harmsen