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

[Jan Cholasta]

On 12.8.2016 18:57, Petr Spacek wrote:
> On 12.8.2016 11:33, Jan Cholasta wrote:
>> On 4.8.2016 18:18, Petr Vobornik wrote:
>>> On 07/22/2016 07:13 AM, Fraser Tweedale wrote:
>>>> On Tue, Jul 19, 2016 at 08:50:34AM +0200, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> On 14.7.2016 13:44, Fraser Tweedale wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> The attached patch includes SANs in cert-show output.  If you have
>>>>>> certs with esoteric altnames (especially any that are more than just
>>>>>> ASN.1 string types), please test with those certs.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/6022
>>>>>
>>>>> I think it would be better to have a separate attribute for each supported
>>>>> SAN type rather than cramming everything into subject_alt_name. That way if
>>>>> you care only about a single specific type you won't have to go through all
>>>>> the values and parse them. Also it would allow you to use param types
>>>>> appropriate to the SAN types (DNSNameParam for DNS names, Principal for
>>>>> principal names, etc.)
>>>>>
>>>>> Nitpick: please don't mix moving existing stuff and adding new stuff in a
>>>>> single patch.
>>>>>
>>>> Updated patches attached.
>>>>
>>>> Patches 0092..0094 are refactors and bugfixes.
>>>> Patch 0090-2 is the main feature (depends on 0092..0094).
>>>>
>>>> Thanks,
>>>> Fraser
>>>>
>>>
>>> bump for review
>>
>> Patch 0092: ACK
>>
>> Patch 0093: ACK
>>
>> Patch 0094: 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.
>>
>> 2) With --all, san_other should be included in the result for all otherNames,
>> even the known ones, to provide (limited) forward compatibility.
>>
>> 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).
>
> As far as I remember this reasoning usually comes back to bite us into butt.
>
> - "Implement only this subset, nobody else needs the other the of ...
> (whatever, e.g. DNS record type)."
> - "Yes my lord."
>
> Two months later:
>
> - "We need to support for XYZ. It was (already) late yesterday!"
>
> :-)

Care to give a concrete example of when this actually happened? Because 
IIRC this happened once or twice, not "usually".

Anyway, I'm fine with whatever, my point was that additional effort 
needs to be put in to support "everything" and even then, we wouldn't 
actually support everything (two months later: "we need to support 
extension XYZ!").

(FWIW, I think it would be more useful to add support for Basic 
constraints rather than X.400 SANs.)

>
> Petr^2 Spacek
>
>>
>> 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).
>


-- 
Jan Cholasta