[Freeipa-devel] [PATCH 0014] Support multiple user and host certificates

[Martin Basti]

On 28/05/15 10:13, Fraser Tweedale wrote:
> On Wed, May 27, 2015 at 06:12:50PM +0200, Martin Basti wrote:
>> On 27/05/15 15:53, Fraser Tweedale wrote:
>>> This patch adds supports for multiple user / host certificates.  No
>>> schema change is needed ('usercertificate' attribute is already
>>> multi-value).  The revoke-previous-cert behaviour of host-mod and
>>> user-mod has been removed but revocation behaviour of -del and
>>> -disable is preserved.
>>>
>>> The latest profiles/caacl patchset (0001..0013 v5) depends on this
>>> patch for correct cert-request behaviour.
>>>
>>> There is one design question (or maybe more, let me know): the
>>> `--out=FILENAME' option to {host,service} show saves ONE certificate
>>> to the named file.  I propose to either:
>>>
>>> a) write all certs, suffixing suggested filename with either a
>>>     sequential numerical index, e.g. "cert.pem" becomes
>>>     "cert.pem.1", "cert.pem.2", and so on; or
>>>
>>> b) as above, but suffix with serial number and, if there are
>>>     different issues, some issuer-identifying information.
>>>
>>> Let me know your thoughts.
>>>
>>> Thanks,
>>> Fraser
>>>
>>>
>> Is there a possible way how to store certificates into one file?
>> I read about possibilities to have multiple certs in one .pem file, but I'm
>> not cert guru :)
>>
>> I personally vote for serial number in case there are multiple certificates,
>> if ^ is no possible.
>>
> Actually, yeah, we can just write the PEMs to a single file,
> sequentially.  I don't know why I didn't think of that... ¯\_(ツ)_/¯
>
>> 1)
>> +            if len(certs) > 0:
>>
>> please use only,
>> if certs:
>>
> I have strong feels about this.  ``if len(certs) > 0:`` admits fewer
> bugs than ``if certs:`` , e.g. if 'certs' were bound to a
> non-length-able object by mistake.  Even though we have already
> iterated ``certs`` at this point in the function, this failure mode
> is still possible, e.g. if ``certs`` is a generator.  Using ``if
> certs:`` will not fail for a generator, but it will be a silent bug!
>
> PEP-8 is wrong on this one.  It is better to use the construction
> that admits fewer errors.  Unless it causes lint failure (on f21 it
> does not) I prefer not to change it.
I still think it should be in PEP8 style.

We should avoid these mistakes, not create the code which will detect it.

And, I forgot to write, please use
except Exception as e:  instead except Exception, e
to be compatible with python3
>> 2)
>> You need to re-generate API/ACI.txt in this patch
>>
> Good pickup, thanks.
>
>> 3)
>> syntax error:
>> +        for dercert in certs_der
>>
> Geez... dunno how I let that one past -_-  My bad.
>
>> 4)
>> command
>> ipa user-mod ca_user --certificate=<ceritifcate>
>>
>> removes the current certificate from the LDAP, by design.
>> Should be the old certificate(s) revoked? You removed that part in the code.
>>
>> only the --addattr='usercertificate=<cert>' appends new value there
>>
> Yeah... I was a bit confused about how --addattr=... interacts with
> options.  I understand it now, and yes I think we should revoke
> certificates that get removed this way.
>
> cert-request will use addattr= so no existing certificates will get
> removed (or revoked).  New patch addressing this and other points
> will arrive on list today.
>
> Thanks for reviewing!
> Fraser
>
>> -- 
>> Martin Basti
>>


-- 
Martin Basti