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

[Jan Cholasta]

Dne 3.8.2015 v 14:34 Martin Babinsky napsal(a):
> 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?
>

I don't see why not.

Pushed to:
master: 555229e33e44a200a4035d21da326f568b25946c
ipa-4-2: d0db86f9b5ec33b9015163035ff84da20d859a2a

-- 
Jan Cholasta