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

[Petr Spacek]

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!"

:-)

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