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

[Lukas Slebodnik]

On (15/08/16 15:30), Petr Spacek wrote:
>On 15.8.2016 15:07, Fraser Tweedale wrote:
>> On Mon, Aug 15, 2016 at 07:48:22AM +0200, Jan Cholasta wrote:
>>> 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".
>
>I do not have list at hand, sorry. It is just my feeling.
>
>
>>> 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!").
>>>
>> Sure, but in this case it was minimal additional effort to support
>> even the esoteric name types - it is only for display after all; if
>> an uncommon altname is (somehow) there we can display it.
>
>That is the main point. The cost is minimal so why not to do it?
>
maybe because there would not be anyone who would write a test?
And feature without tests is bad feature.

Petr, are you volunteer?
If no then please stop bikeshading.

LS