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

[Martin Babinsky]

On 07/14/2015 03:22 PM, Milan Kubík wrote:
> 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
>

So what about PATCH 45 (tests). Milan ACKed it, can we push it?

-- 
Martin^3 Babinsky