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

[Martin Babinsky]

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.

It would be nice if Milan could comment on PATCH 0045.

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0045.2-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/20150702/78b7dec6/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0044.4-new-commands-to-manage-user-host-service-certificate.patch
Type: text/x-patch
Size: 14792 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150702/78b7dec6/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0043.6-reworked-certificate-normalization-and-revocation.patch
Type: text/x-patch
Size: 13530 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150702/78b7dec6/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0042.2-baseldap-add-support-for-API-commands-managing-only-.patch
Type: text/x-patch
Size: 4804 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150702/78b7dec6/attachment-0003.bin>