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

[Martin Babinsky]

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

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0045.1-test-suite-for-user-host-service-certificate-managem.patch
Type: text/x-patch
Size: 12207 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150630/b73cb181/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0044.2-new-commands-to-manage-user-host-service-certificate.patch
Type: text/x-patch
Size: 15486 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150630/b73cb181/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0043.1-service-plugin-separate-functions-for-certificate-no.patch
Type: text/x-patch
Size: 2302 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150630/b73cb181/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0042.1-baseldap-add-support-for-API-commands-managing-only-.patch
Type: text/x-patch
Size: 4749 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150630/b73cb181/attachment-0003.bin>