[Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names

[Jan Cholasta]

On 23.8.2016 11:46, Fraser Tweedale wrote:
> Thanks for review; rebased and updated patch attached.  Only 0090
> has substantive changes.
>
> Cheers,
> Fraser
>
> On Mon, Aug 22, 2016 at 09:22:08AM +0200, Jan Cholasta wrote:
>> On 19.8.2016 13:11, Fraser Tweedale wrote:
>>> Bump for review.
>>>
>>> On Mon, Aug 15, 2016 at 05:15:16PM +1000, Fraser Tweedale wrote:
>>>> Thanks for reviews.  Rebased and updated patches attached (and one
>>>> new patch).  No substantive changes to 92..94.  Patch order is:
>>>>
>>>> 92-2, 93-2, 94-2, 98, 90-3
>>>>
>>>> Other comments inline.
>>>>
>>>> Thanks,
>>>> Fraser
>>>>
>>>> On Fri, Aug 12, 2016 at 11:33:28AM +0200, Jan Cholasta wrote:
>>>>> Patch 0092: ACK
>>>>>
>>>>> Patch 0093: ACK
>>>>>
>>>>> Patch 0094: ACK
>>
>> Please fix this PEP8 issue before pushing:
>>
>> ./ipaserver/plugins/cert.py:597:17: W503 line break before binary operator
>>
>>
>> Patch 0098: ACK
>>
>>>>>
>>>>> Patch 0090:
>>>>>
>>>>> 1) Generic otherNames (san_other) do not work correctly. The OID is not
>>>>> included in the value and names with complex type other than
>>>>> KerberosPrincipal are not parsed correctly. The value should include the OID
>>>>> and DER blob of the name.
>>>>>
>>>> Updated to use "OID:b64(DER)" as the attribute value.
>>
>> OK.
>>
>>>>
>>>>> 2) With --all, san_other should be included in the result for all
>>>>> otherNames, even the known ones, to provide (limited) forward compatibility.
>>>>>
>>>> Done; when --all given, known otherName kinds are included in
>>>> 'san_other' attribute in addition to their own attribute.
>>
>> OK.
>>
>>>>
>>>>> 3) Do we have to support *all* the name types? I mean we could, for the sake
>>>>> of completeness, but it might be easier to just keep the few ones we
>>>>> actually care about (email, DNS name, principal name, UPN and directory name
>>>>> in your patch 0095).
>>>>>
>>>> Yeah, why not support them all?  See also Petr's comments in other
>>>> branch of thread.
>>
>> Works for me, but see Lukáš's reply, I think he has a point. Maybe we can
>> make a compromise and show only supported name types by default and
>> everything with --all?
>>
> Now only showing DNSName, RFC822Name, DirectoryName, UPN and
> KRBPrincipalName unless --all is given.
>
>>>>
>>>>> 4)
>>>>>
>>>>> +            obj.setdefault(attr_name, []).append(unicode(name))
>>>>>
>>>>> The value should not (always) be unicode, but of the type declared by the
>>>>> param (unicode or ipapython.kerberos.Principal or
>>>>> ipapython.dnsutil.DNSName).
>>>>>
>>>> I now pass the value to the constructor of whatever type the
>>>> parameter uses:
>>>>
>>>>         attr_value = self.params[attr_name].type(name_formatted)
>>>>         obj.setdefault(attr_name, []).append(attr_value)
>>
>> OK.
>>
>>
>> 5) san_directoryname should be a DNParam rather than Str.
>>
> Fixed, thanks.
>
>>
>> 6) Could we use "Subject <name type>" instead of "Subject Alternative Name
>> (<name type>)" for labels? Or something else which is shorter and has the
>> name type more "visible" than the current form.
>>
> No worries.
>
>>
>> 7) The patch needs a rebase.

Thanks, ACK, but I have a couple additional nitpicks:

8) I think we should move the san_* param definitions right after 
subject param definition, so that they are visually close in CLI output.


9) san_directoryname should be added to default_attrs in patch 95, not here.

I took the liberty of fixing these myself.

Pushed to master: 48aaf2bbf5df6dcedaa466753c8fafb478cb31b2

Honza

-- 
Jan Cholasta