[Freeipa-devel] [PATCHES] 172-196 Refactor certificate renewal code

Mon Mar 10 12:03:27 UTC 2014

On 17.10.2013 18:59, Jan Cholasta wrote:
> On 17.10.2013 18:01, Petr Viktorin wrote:
>> On 10/17/2013 02:21 PM, Jan Cholasta wrote:
>>> Hi,
>>>
>>> this patchset contains refactoring of the certificate renewal code,
>>> which will be the base for CA certificate renewal.
>>>
>>> The biggest change is a new certmonger CA helper
>>> dogtag-ipa-ca-renew-agent, which replaces
>>> dogtag-ipa-retrieve-agent-submit as well as parts of certmonger
>>> post-commands used in certificate renewal. It provides more flexibility
>>> when doing renewals and allows unified certmonger configuration on both
>>> CA master and clones.
>>>
>>> How to test: Test both CA-ful and CA-less server and replica installs
>>> and upgrades, check that certmonger is configured properly and
>>> certificate renewal works (see
>>> https://fedorahosted.org/freeipa/ticket/2803#comment:17 for details).
>>>
>>> Dependencies:
>>> freeipa-jcholast-161.3-Fix-certificate-renewal-scripts-to-work-with-separat.patch.
>>>
>>>
>>
>> Thanks!
>> Here are my comments from reading the patches; I haven't tested yet.
>>
>> 161. Looks good
>> 172. Looks good
>>
>> 173.
>> The second hunk removes `pem_cert` that is later used for
>> upload_ca_dercert()
>
> Right, will fix.
>
>>
>> 174. Looks good
>>
>> 175.
>> This line in upload_ca_cert() seems redundant:
>>      name = certdb.cacert_name
>
> It indeed is in this particular patch.
>
>>
>> 176. Looks good
>> 177. Looks good
>> 178. Looks good
>>
>> 179.
>> Note: look if the other calls don't rely on this (replica-install,
>> ca-install)
>
> I didn't see any failures.
>
>>
>> 180. Looks good
>>
>> 181.
>> What are those constants you define in the beginning? Why are most not
>> used? I think you should add a link to some reference.
>
> They are certmonger specific:
>
> https://git.fedorahosted.org/cgit/certmonger.git/tree/doc/submit.txt
> https://git.fedorahosted.org/cgit/certmonger.git/tree/src/submit-e.h
>
> Will add the links.
>
>> Nitpick - PEP8 explicitly says to avoid aligning with extra spaces:
>> http://www.python.org/dev/peps/pep-0008/#pet-peeves
>
> OK.
>
>>
>> Please use a docstring for documenting what request_cert() does, and
>> describe the return value.
>
> OK. Return value is based on information in submit.txt (see above for
> link).
>
>> I don't see the purpose of the `if rc == WAIT_WITH_DELAY` block since
>> `delay, cookie` gets joined again by the caller.
>> Wouldn't it be cleaner to always return (rc, output) instead of doing
>> that [1:] dance?
>
> In the context of this single patch, yes. It the context of the whole
> patchset, no.
>
>>
>> Did you consider using `traceback.format_exc` for logging errors? Full
>> tracebacks tend to be more useful than just type & message.
>
> I did, but was not sure I want to spam syslog with full tracebacks.
>
>>
>> 182.
>> I don't understand why you reordered get_subjectaltname & get_subject,
>> that makes it hard to see what changed.
>
> It felt more natural to me this way.
>
>>
>> 183. Looks good
>>
>> 184. Looks OK
>>
>> 185.
>> ldap_connect: conn.disconnect() should be in a finally clause
>
> Right.
>
>>
>> retrieve_cert:
>> - Please use docstrings for documenting functions
>> - The try: block is too long, NotFound only needs to be handled for
>> conn.get_entry()
>> - The `except Exception` block should be removed, or it should just log
>> and re-raise; the error handling is done in main().
>
> The body of this function is mostly copy-paste from
> dogtag-ipa-retrieve-agent-submit. Yes, it could use some cleaning up.
>
>>
>> main:
>> Shouldn't we rather raise an error if CERTMONGER_CA_PROFILE has some
>> unknown value?
>
> No, it might be a Dogtag profile name.
>
>>
>>
>> 186.
>> I'm getting lost in all the arguments to dogtag_start_tracking(). Could
>> you name them when calling it?
>
> OK.
>
>>
>> 187.
>> I think one changelog entry for all these patches is enough.
>
> OK.
>
>>
>> 188.
>> Please use docstrings for documenting functions.
>>
>> I think even with OPERATION_NOT_SUPPORTED_BY_HELPER this should display
>> some output, like "unknown operation %s".
>
> This is handled by certmonger.
>
>>
>> When store_cert() runs out of attempts, it should not return ISSUED. The
>> exception should be re-raised so it's logged.
>
> If it didn't return ISSUED, the certificate would not be saved by
> certmonger and you would not be able to retry the storage attempt. How
> much of a problem is this I don't know, the behavior is taken from
> renew_ra_cert.
>
>>
>> 189.
>> Again, naming arguments in dogtag_start_tracking calls would make them
>> clearer
>>
>> 190.
>> Instead of dogtag.install_constants, this should use
>> configured_constants().
>
> I didn't actually look at what's inside the function. Will fix.
>
>>
>> 191.
>> Again, please use docstrings for documenting functions.
>> Or just do the `if` right in main(), I don't think a separate function
>> is worth it here.
>
> OK, it probably isn't.
>
>>
>> 192.
>> Again, naming dogtag_start_tracking args would make the code clearer.
>>
>> 193.
>> Same here
>>
>> 194.
>> I don't see why you reorder the methods, it makes the changes harder to
>> spot.
>
> To have them in a single spot instead of all over the place.
>
>>
>> 195. Looks OK
>>
>> 196.
>> Again, please use docstrings for documenting functions.
>>
>>
>

Updated patches attached.

Note that patches 178 & 179 were already pushed. Also, patch 190 was 
changed to store information about which CA instance is master in LDAP.

Dependencies:
freeipa-jcholast-236.1-Log-unhandled-exceptions-in-certificate-renewal-scri.patch

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-161.4-Fix-certificate-renewal-scripts-to-work-with-separat.patch
Type: text/x-patch
Size: 5892 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-172.1-Move-CACERT-definition-to-a-single-place.patch
Type: text/x-patch
Size: 13423 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-173.1-Do-not-create-CA-certificate-files-in-CA-less-server.patch
Type: text/x-patch
Size: 1928 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-174.1-Use-LDAP-API-to-upload-CA-certificate-instead-of-lda.patch
Type: text/x-patch
Size: 2811 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-175.1-Upload-CA-certificate-from-DS-NSS-database-in-CA-les.patch
Type: text/x-patch
Size: 3169 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-176.1-Remove-unused-method-export_ca_cert-of-dsinstance.patch
Type: text/x-patch
Size: 979 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-177.1-Show-progress-when-enabling-SSL-in-DS-in-ipa-server-.patch
Type: text/x-patch
Size: 3276 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-180.1-Use-certmonger-D-Bus-API-to-configure-certmonger-in-.patch
Type: text/x-patch
Size: 4923 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-181.1-Add-new-certmonger-CA-helper-dogtag-ipa-ca-renew-age.patch
Type: text/x-patch
Size: 4170 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-182.1-Update-pkcs10-module-functions-to-always-load-CSRs-a.patch
Type: text/x-patch
Size: 5484 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-183.1-Remove-unused-function-get_subjectaltname-from-the-c.patch
Type: text/x-patch
Size: 1324 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-184.1-Add-function-for-parsing-friendly-name-from-certific.patch
Type: text/x-patch
Size: 2869 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-185.1-Support-retrieving-renewed-certificates-from-LDAP-in.patch
Type: text/x-patch
Size: 3515 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-186.1-Use-dogtag-ipa-ca-renew-agent-to-retrieve-renewed-ce.patch
Type: text/x-patch
Size: 5177 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-187.1-Remove-dogtag-ipa-retrieve-agent-submit.patch
Type: text/x-patch
Size: 4906 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-188.1-Support-storing-renewed-certificates-to-LDAP-in-dogt.patch
Type: text/x-patch
Size: 5516 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-189.1-Use-dogtag-ipa-ca-renew-agent-to-track-certificates-.patch
Type: text/x-patch
Size: 13042 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0016.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-190.1-Store-information-about-which-CA-server-is-master-in.patch
Type: text/x-patch
Size: 5330 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0017.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-191.1-Make-the-default-dogtag-ipa-ca-renew-agent-behavior-.patch
Type: text/x-patch
Size: 2858 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0018.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-192.1-Merge-restart_pkicad-functionality-to-renew_ca_cert-.patch
Type: text/x-patch
Size: 7777 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0019.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-193.1-Merge-restart_httpd-functionality-to-renew_ra_cert.patch
Type: text/x-patch
Size: 2238 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0020.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-194.1-Use-the-same-certmonger-configuration-for-both-CA-ma.patch
Type: text/x-patch
Size: 9618 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0021.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-195.1-Update-certmonger-configuration-in-ipa-upgradeconfig.patch
Type: text/x-patch
Size: 7235 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0022.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-196.1-Support-exporting-CSRs-in-dogtag-ipa-ca-renew-agent.patch
Type: text/x-patch
Size: 1733 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140310/95a1d66e/attachment-0023.bin>