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