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? Petr^2 Spacek >> (FWIW, I think it would be more useful to add support for Basic constraints >> rather than X.400 SANs.) >> > I can only agree, and say: another RFE for another day :) > >>> >>> 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). -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
