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

[Jan Cholasta]

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.
>
>

-- 
Jan Cholasta