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

[Jan Cholasta]

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.)

-- 
Jan Cholasta