[Freeipa-devel] [PATCHES 0042-45] new commands for adding/removing certificates from entries

Milan Kubík mkubik at redhat.com
Tue Jul 14 13:22:47 UTC 2015


On 07/02/2015 04:44 PM, Jan Cholasta wrote:
> Dne 2.7.2015 v 16:36 Martin Babinsky napsal(a):
>> On 07/02/2015 02:37 PM, Martin Babinsky wrote:
>>> On 07/02/2015 11:28 AM, Martin Babinsky wrote:
>>>> On 07/02/2015 11:12 AM, Martin Babinsky wrote:
>>>>> On 07/01/2015 03:05 PM, Martin Babinsky wrote:
>>>>>> On 06/30/2015 02:45 PM, Martin Babinsky wrote:
>>>>>>> On 06/30/2015 01:11 PM, Martin Babinsky wrote:
>>>>>>>> On 06/30/2015 12:04 PM, Jan Cholasta wrote:
>>>>>>>>> Dne 29.6.2015 v 10:36 Martin Babinsky napsal(a):
>>>>>>>>>> On 06/23/2015 01:49 PM, Martin Babinsky wrote:
>>>>>>>>>>> This patchset implements new API commands for manipulating
>>>>>>>>>>> user/host/service userCertificate attribute alongside some
>>>>>>>>>>> underlying
>>>>>>>>>>> plumbing.
>>>>>>>>>>>
>>>>>>>>>>> PATCH 0045 is a small test suite that I slapped together since
>>>>>>>>>>> manual
>>>>>>>>>>> testing of this stuff is very cumbersome. It requires my PATCH
>>>>>>>>>>> 0040 to
>>>>>>>>>>> apply and work which was pushed to master recently
>>>>>>>>>>> (commit 74883bbc959058c8bfafd9f63e8fad0e3792ac28).
>>>>>>>>>>>
>>>>>>>>>>> The work is related to
>>>>>>>>>>> http://www.freeipa.org/page/V4/User_Certificates
>>>>>>>>>>> and https://fedorahosted.org/freeipa/ticket/4238
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> Attaching updated patches.
>>>>>>>>>>
>>>>>>>>>> Here are some notes for Jan because I did some things 
>>>>>>>>>> differently
>>>>>>>>>> than
>>>>>>>>>> we agreed on during review:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 1.) I chose not to rename 'usercertificate' to
>>>>>>>>>> 'usercertificate;binary'
>>>>>>>>>> and back in pre/post callbacks. Despite the fact that the 
>>>>>>>>>> correct
>>>>>>>>>> way to
>>>>>>>>>> name the certificate attribute is 'usercertificate;binary', I 
>>>>>>>>>> feel
>>>>>>>>>> that
>>>>>>>>>> suddenly renaming it in the new code is asking for trouble.
>>>>>>>>>
>>>>>>>>> New code is new, there is no renaming, there is naming, and that
>>>>>>>>> naming
>>>>>>>>> should follow standards, and the standard is
>>>>>>>>> userCertificate;binary.
>>>>>>>>>
>>>>>>>>> (For the record I did not ask for any renaming in *old* host and
>>>>>>>>> service
>>>>>>>>> code.)
>>>>>>>>>
>>>>>>>> OK I will then use 'usercertificate;binary' and try to not break
>>>>>>>> things.
>>>>>>>>>>
>>>>>>>>>> I'm all for changing the mapping between CLI options and actual
>>>>>>>>>> attribute names but it should be done in a systematic fashion.
>>>>>>>>>
>>>>>>>>> +1, shall I post a patch?
>>>>>>>>>
>>>>>>>> That would be great, but I'm not sure if there is time for it.
>>>>>>>> Maybe we
>>>>>>>> can create a ticket for tracking?
>>>>>>>>>>
>>>>>>>>>> 2.) I have kept the `normalize_certs` function. It has the
>>>>>>>>>> potential to
>>>>>>>>>> catch incorrectly formatted/encoded certificates and in a way
>>>>>>>>>> circumvents the slightly demented way the framework deals with
>>>>>>>>>> supposedly binary data.
>>>>>>>>>
>>>>>>>>> One sentence above you asked for doing things in systematic
>>>>>>>>> fashion.
>>>>>>>>> This is exactly what it isn't. A systematic solution would be 
>>>>>>>>> a new
>>>>>>>>> parameter type for certificates.
>>>>>>>>>
>>>>>>>> Ha I didn't notice that incorrect encoding is caught by validator.
>>>>>>>>
>>>>>>>> But I think that we still need to catch malformed certificates 
>>>>>>>> that
>>>>>>>> can
>>>>>>>> not be decoded to DER and AFAIK we don't do that anywhere (failing
>>>>>>>> tests
>>>>>>>> when adding a random Base64-encoded string confirm this).
>>>>>>>>
>>>>>>>> All this probably stems from my confusion about the way IPA
>>>>>>>> framework
>>>>>>>> guesses binary data. For example, if I call
>>>>>>>> `api.Command.user_add_cert`
>>>>>>>> and fill 'certificate' option with Base64 blob reencoded to 
>>>>>>>> Unicode,
>>>>>>>> everything works as expected.
>>>>>>>>
>>>>>>>> However, filling this option with 'str' leads to another round of
>>>>>>>> Base64
>>>>>>>> encoding in the framework, leading to 'userCertificate;binary'
>>>>>>>> which is
>>>>>>>> filled by original Base64 blob instead of DER encoded cert.
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I have also added two negative test cases which deal with
>>>>>>>>>> incorrectly
>>>>>>>>>> encoded and formatted certificates.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> Attaching updated patches (actually only 44 is updated, I added the
>>>>>>> rename to/from 'usercertificate;binary' to user pre/post 
>>>>>>> callbacks).
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> Another patch update attached (mainly fixing pep8 complaints and
>>>>>> reworking certificate validation).
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> Updated patches attached.
>>>>>
>>>>>
>>>>>
>>>>
>>>> I left a a bug in PATCH 0043. Attaching updated version.
>>>>
>>>>
>>>>
>>> Attaching updated patches.
>>>
>>>
>>>
>> Attaching revised patchset.
>
> Thanks, ACK on patch 42-44.
>
> Pushed to master: 76eea85701af80dc972c47e14aecc7a688b9c846
>
>>
>> It would be nice if Milan could comment on PATCH 0045.
>>
>
> (I did not push this patch.)
>
Hi,

sorry for the delay. The test looks good to me.

Though we'll better rewrite the test after the Trackers for the plugins 
involved in certificate signing and  CA ACL enforcement will be 
available. It won't be necessary right away, though.

ACK

Thanks,
Milan




More information about the Freeipa-devel mailing list