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

[Jan Cholasta]

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?

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


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.


7) The patch needs a rebase.


-- 
Jan Cholasta