[Pki-devel] [PATCH] 657 Refactored CA certificate generation.

[Christina Fu]

The fixed areas look good.
If tested to work, ACK.

Christina

On 11/23/2015 11:54 AM, Endi Sukma Dewata wrote:
> Thanks for the feedback. New patch attached.
>
> On 11/16/2015 7:48 PM, Christina Fu wrote:
>> 1 in 
>> base/server/python/pki/server/deployment/scriptlets/configuration.py
>> doesn't this just add the leaf cert rather than the whole chain? In
>> other words, if your chain contains 2 or more certs, only the leaf subca
>> cert is added, isn't it?
>>
>> +                    nssdb.add_cert(
>> +                        nickname=external_ca_nickname,
>> +                        cert_file=external_ca_cert_chain_file,
>> +                        trust_attributes='CTu,CTu,CTu')
>
> Fixed. The new patch now supports PKCS #7 file, a single PEM cert, and 
> the base-64 PKCS #7 data generated by getCertChain servlet.
>
>> 2 Also in the same file
>> + # If specified, import externally-signed CA cert in NSS database.
>> ...
>> Shouldn't there be a case when the externally signed ca keys were
>> generated on the hsm, you'd then need to import the issued externally
>> signed ca cert into the hsm db as well?
>
> If the externally-signed CA cert is specified in 
> pki_external_ca_cert_path parameter it will be imported into the NSS 
> database, regardless whether HSM is used. The code now calls certutil 
> with -h option to specify the target token. Alternatively, the 
> certificate can be imported manually before starting step 2. I've 
> updated the docs: 
> http://pki.fedoraproject.org/wiki/Installing_with_Externaly-Signed_CA_Certificate
>
>> 3 base/common/src/com/netscape/certsrv/system/ConfigurationRequest.java
>> I"m not seeing the following method being called, yet the getExternal()
>> is being called...did I miss something?
>>
>> +    public void setExternal(Boolean external) {
>>
>> +        this.external = external;
>> +    }
>
> The external attribute is set in Python in pkihelper.py:3983:
>
>   data.external = self.external
>
> The value will be sent to the server via REST interface. The 
> getExternal() will read that value.
>
>> 4. 
>> base/server/cms/src/com/netscape/cms/servlet/csadmin/ConfigurationUtils.java
>> +    public static void loadCert(Cert cert) throws Exception {
>> ...
>> +        // create certificate record to reserve the serial number in 
>> internal database
>> +        ICertRecord record = cr.createCertRecord(serialNo, 
>> x509CertImpl, meta);
>> +        cr.addCertificateRecord(record);
>>
>> In case of an externally signed ca or existing ca, why would you need to
>> reserve the serial number or even add in the certificate repository?
>
> Fixed. This code is actually only needed when importing existing 
> self-signed CA cert. This way when the code generates the system 
> certificates it will not conflict with the CA cert's serial number 
> both in NSS database and in internal LDAP database. For existing 
> non-self-signed CA cert or externally signed CA cert the code will not 
> create the LDAP record.
>
>> 5.
>> Finally, please add comments to explain the cases for clarification...
>> such as "stand-alone v.s. external; step 1, step 2, etc."  For example,
>> it seems the "external" could imply "existing" as well in terms of ca
>> cert, you might want to put in comment.
>
> Yes, the "external" code handles both external CA and existing CA 
> cases. I've added some inline comments. Please let me know if we need 
> more.
>