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

[Martin Basti]

On 03/06/15 15:21, Fraser Tweedale wrote:
> On Wed, Jun 03, 2015 at 01:55:47PM +0200, Milan Kubik wrote:
>> On 06/03/2015 01:17 PM, Martin Basti wrote:
>>> On 02/06/15 16:03, Jan Cholasta wrote:
>>>> Dne 2.6.2015 v 12:36 Martin Basti napsal(a):
>>>>> On 02/06/15 11:42, Fraser Tweedale wrote:
>>>>>> On Mon, Jun 01, 2015 at 02:50:45PM +0200, Martin Basti wrote:
>>>>>>> On 01/06/15 06:40, Fraser Tweedale wrote:
>>>>>>>> New version of patch; ``{host,service}-show --out=FILE`` now writes
>>>>>>>> all certs to FILE.  Rebased on latest master.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Fraser
>>>>>>>>
>>>>>>>> On Thu, May 28, 2015 at 09:18:04PM +1000, Fraser Tweedale wrote:
>>>>>>>>> Updated patch attached.  Notably restores/adds revocation behaviour
>>>>>>>>> to host-mod and service-mod.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Fraser
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 1)
>>>>>>>>>> +            if len(certs) > 0:
>>>>>>>>>>
>>>>>>>>>> please use only,
>>>>>>>>>> if certs:
>>>>>>>>>>
>>>>>>>>>> 2)
>>>>>>>>>> You need to re-generate API/ACI.txt in this patch
>>>>>>>>>>
>>>>>>>>>> 3)
>>>>>>>>>> syntax error:
>>>>>>>>>> +        for dercert in certs_der
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>> -- 
>>>>>>>>>> Martin Basti
>>>>>>>>>>
>>>>>>> My objections/proposed solutions in attached patch.
>>>>>>>
>>>>>>> * VERSION
>>>>>>> * In the previous version normalized values was stored in LDAP, so I
>>>>>>> added
>>>>>>> it back.  (I dont know why there is no normalization in param
>>>>>>> settings, but
>>>>>>> normalization for every certificate is done in callback. I will
>>>>>>> file a
>>>>>>> ticket for this)
>>>>>>> * IMO only normalized certificates should be compared in the old
>>>>>>> certificates detection
>>>>>>>
>>>>>> I incorporated your suggested changes in new patch (attached).
>>>>>>
>>>>>> There were no proposed changes to the other patchset (0001..0013)
>>>>>> since rebase.
>>>>>>
>>>>>> Thanks,
>>>>>> Fraser
>>>>> Thank you,
>>>>> ACK
>>>>> Martin^2
>>>>>
>>>> Pushed to master: 7f7c247bb5a4b0030d531f4f14c156162e808212
>>>>
>>> Regression found.
>>>
>>> Patch to fix the issue is attached.
>>>
>> The fix works, thanks.
>>
>> Milan
> Thanks for finding, fixing and testing!  ACK from me.
>
> I also present a fix of my own.  It fixes a problem where
> service-mod deleted all certificates when
> '--addattr usercertificate=XXX' was used instead of
> '--usercertificate=XXX' options.
>
> Cheers,
> Fraser
ACK

-- 
Martin Basti