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

[Martin Basti]

On 28/05/15 11:17, Fraser Tweedale wrote:
> On Thu, May 28, 2015 at 10:40:22AM +0200, Martin Basti wrote:
>> 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.
>>
> How do you know if you made a mistake?  This is not just about me
> making this change now.  If someone in the future comes along and
> for whatever reasons changes the list to a generator, the diff in
> isolation may look like a good change, and the reviewer might not
> notice that the change leads to undesired behaviour elsewhere
> (outside the diff context).
>
> If you have good test coverage, tests might catch the bug.  If not,
> hopefully QE notices the incorrect behaviour before a customer does.
> The best tool is a type system so you don't get a program if you
> make a mistake like this.  Well, Python doesn't give us that tool
> but let us not ignore what it *can* do to help us for the sake of
> adhereing to PEP-8 or saving a few characters.

Ok then, leave there len() > 0

but to catch these kind of mistakes, we should have asserts everywhere 
to verify type.
>
>> And, I forgot to write, please use
>> except Exception as e:  instead except Exception, e
>> to be compatible with python3
>>
> Sure, no problem.
>
>>>> 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
>>


-- 
Martin Basti