Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
On 23.8.2016 11:46, Fraser Tweedale wrote: Thanks for review; rebased and updated patch attached. Only 0090 has substantive changes. Cheers, Fraser On Mon, Aug 22, 2016 at 09:22:08AM +0200, Jan Cholasta wrote: On 19.8.2016 13:11, Fraser Tweedale wrote: Bump for review. On Mon, Aug 15, 2016 at 05:15:16PM +1000, Fraser Tweedale wrote: Thanks for reviews. Rebased and updated patches attached (and one new patch). No substantive changes to 92..94. Patch order is: 92-2, 93-2, 94-2, 98, 90-3 Other comments inline. Thanks, Fraser On Fri, Aug 12, 2016 at 11:33:28AM +0200, Jan Cholasta wrote: Patch 0092: ACK Patch 0093: ACK Patch 0094: ACK Please fix this PEP8 issue before pushing: ./ipaserver/plugins/cert.py:597:17: W503 line break before binary operator Patch 0098: 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. Updated to use "OID:b64(DER)" as the attribute value. OK. 2) With --all, san_other should be included in the result for all otherNames, even the known ones, to provide (limited) forward compatibility. Done; when --all given, known otherName kinds are included in 'san_other' attribute in addition to their own attribute. OK. 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). Yeah, why not support them all? See also Petr's comments in other branch of thread. Works for me, but see Lukáš's reply, I think he has a point. Maybe we can make a compromise and show only supported name types by default and everything with --all? Now only showing DNSName, RFC822Name, DirectoryName, UPN and KRBPrincipalName unless --all is given. 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). I now pass the value to the constructor of whatever type the parameter uses: attr_value = self.params[attr_name].type(name_formatted) obj.setdefault(attr_name, []).append(attr_value) OK. 5) san_directoryname should be a DNParam rather than Str. Fixed, thanks. 6) Could we use "Subject " instead of "Subject Alternative Name ()" for labels? Or something else which is shorter and has the name type more "visible" than the current form. No worries. 7) The patch needs a rebase. Thanks, ACK, but I have a couple additional nitpicks: 8) I think we should move the san_* param definitions right after subject param definition, so that they are visually close in CLI output. 9) san_directoryname should be added to default_attrs in patch 95, not here. I took the liberty of fixing these myself. Pushed to master: 48aaf2bbf5df6dcedaa466753c8fafb478cb31b2 Honza -- Jan Cholasta -- 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
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
Thanks for review; rebased and updated patch attached. Only 0090 has substantive changes. Cheers, Fraser On Mon, Aug 22, 2016 at 09:22:08AM +0200, Jan Cholasta wrote: > On 19.8.2016 13:11, Fraser Tweedale wrote: > > Bump for review. > > > > On Mon, Aug 15, 2016 at 05:15:16PM +1000, Fraser Tweedale wrote: > > > Thanks for reviews. Rebased and updated patches attached (and one > > > new patch). No substantive changes to 92..94. Patch order is: > > > > > > 92-2, 93-2, 94-2, 98, 90-3 > > > > > > Other comments inline. > > > > > > Thanks, > > > Fraser > > > > > > On Fri, Aug 12, 2016 at 11:33:28AM +0200, Jan Cholasta wrote: > > > > Patch 0092: ACK > > > > > > > > Patch 0093: ACK > > > > > > > > Patch 0094: ACK > > Please fix this PEP8 issue before pushing: > > ./ipaserver/plugins/cert.py:597:17: W503 line break before binary operator > > > Patch 0098: 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. > > > > > > > Updated to use "OID:b64(DER)" as the attribute value. > > OK. > > > > > > > > 2) With --all, san_other should be included in the result for all > > > > otherNames, even the known ones, to provide (limited) forward > > > > compatibility. > > > > > > > Done; when --all given, known otherName kinds are included in > > > 'san_other' attribute in addition to their own attribute. > > OK. > > > > > > > > 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). > > > > > > > Yeah, why not support them all? See also Petr's comments in other > > > branch of thread. > > Works for me, but see Lukáš's reply, I think he has a point. Maybe we can > make a compromise and show only supported name types by default and > everything with --all? > Now only showing DNSName, RFC822Name, DirectoryName, UPN and KRBPrincipalName unless --all is given. > > > > > > > 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). > > > > > > > I now pass the value to the constructor of whatever type the > > > parameter uses: > > > > > > attr_value = self.params[attr_name].type(name_formatted) > > > obj.setdefault(attr_name, []).append(attr_value) > > OK. > > > 5) san_directoryname should be a DNParam rather than Str. > Fixed, thanks. > > 6) Could we use "Subject " instead of "Subject Alternative Name > ()" for labels? Or something else which is shorter and has the > name type more "visible" than the current form. > No worries. > > 7) The patch needs a rebase. > > > -- > Jan Cholasta From 9dbe4c3cebb1279aefefe3dae9f3da2232d9c12f Mon Sep 17 00:00:00 2001 From: Fraser Tweedale Date: Thu, 21 Jul 2016 16:27:49 +1000 Subject: [PATCH 92/94] Move GeneralName parsing code to ipalib.x509 GeneralName parsing code is primarily relevant to X.509. An upcoming change will add SAN parsing to the cert-show command, so first move the GeneralName parsing code from ipalib.pkcs10 to ipalib.x509. Part of: https://fedorahosted.org/freeipa/ticket/6022 --- ipalib/pkcs10.py | 93 ++--- ipalib/x509.py| 114 +- ipaserver/plugins/cert.py | 8 ++-- 3 files changed, 120 insertions(+), 95 deletions(-) diff --git a/ipalib/pkcs10.py b/ipalib/pkcs10.py index e340c1a2005ad781542a32a0a76753e80364dacf..158ebb3a25be2bd292f3883545fe8afe49b7be8c 100644 --- a/ipalib/pkcs10.py +++ b/ipalib/pkcs10.py @@ -22,9 +22,10 @@ from __future__ import print_function import sys import base64 import nss.nss as nss -from pyasn1.type import univ, char, namedtype, tag +from pyasn1.type import univ, namedtype, tag from pyasn1.codec.der import decoder import six +from ipalib import x509 if six.PY3: unicode = str @@ -32,11 +33,6 @@ if six.PY3: PEM = 0 DER = 1 -SAN_DNSNAME = 'DNS name' -SAN_RFC822NAME = 'RFC822 Name' -SAN_OTHERNAME_UPN = 'Other Name (OID.1.3.6.1.4.1.311.20.2.3)' -SAN_OTHERNAME_KRB5PRINCIPALNAME = 'Other Name (OID.1.3.6.1.5.2.2)' - def get_subject(csr, datatype=PEM): """ Given a CSR return the subject value. @@ -72,78 +68,6 @@ def get_extensions(csr, datatype=PEM): return tuple(get_prefixed_oid_str(ext)[4:] for ext in request.extensions) -class _PrincipalName(univ.Sequence): -componentType = namedtype.NamedTypes( -namedtype.NamedType('name-ty
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
On 19.8.2016 13:11, Fraser Tweedale wrote: Bump for review. On Mon, Aug 15, 2016 at 05:15:16PM +1000, Fraser Tweedale wrote: Thanks for reviews. Rebased and updated patches attached (and one new patch). No substantive changes to 92..94. Patch order is: 92-2, 93-2, 94-2, 98, 90-3 Other comments inline. Thanks, Fraser On Fri, Aug 12, 2016 at 11:33:28AM +0200, Jan Cholasta wrote: Patch 0092: ACK Patch 0093: ACK Patch 0094: ACK Please fix this PEP8 issue before pushing: ./ipaserver/plugins/cert.py:597:17: W503 line break before binary operator Patch 0098: 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. Updated to use "OID:b64(DER)" as the attribute value. OK. 2) With --all, san_other should be included in the result for all otherNames, even the known ones, to provide (limited) forward compatibility. Done; when --all given, known otherName kinds are included in 'san_other' attribute in addition to their own attribute. OK. 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). Yeah, why not support them all? See also Petr's comments in other branch of thread. Works for me, but see Lukáš's reply, I think he has a point. Maybe we can make a compromise and show only supported name types by default and everything with --all? 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). I now pass the value to the constructor of whatever type the parameter uses: attr_value = self.params[attr_name].type(name_formatted) obj.setdefault(attr_name, []).append(attr_value) OK. 5) san_directoryname should be a DNParam rather than Str. 6) Could we use "Subject " instead of "Subject Alternative Name ()" for labels? Or something else which is shorter and has the name type more "visible" than the current form. 7) The patch needs a rebase. -- Jan Cholasta -- 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
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
Bump for review. On Mon, Aug 15, 2016 at 05:15:16PM +1000, Fraser Tweedale wrote: > Thanks for reviews. Rebased and updated patches attached (and one > new patch). No substantive changes to 92..94. Patch order is: > > 92-2, 93-2, 94-2, 98, 90-3 > > Other comments inline. > > Thanks, > Fraser > > On Fri, Aug 12, 2016 at 11:33:28AM +0200, Jan Cholasta wrote: > > 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. > > > Updated to use "OID:b64(DER)" as the attribute value. > > > 2) With --all, san_other should be included in the result for all > > otherNames, even the known ones, to provide (limited) forward compatibility. > > > Done; when --all given, known otherName kinds are included in > 'san_other' attribute in addition to their own attribute. > > > 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). > > > Yeah, why not support them all? See also Petr's comments in other > branch of thread. > > > 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). > > > I now pass the value to the constructor of whatever type the > parameter uses: > > attr_value = self.params[attr_name].type(name_formatted) > obj.setdefault(attr_name, []).append(attr_value) > From 17e5515ab0eeb92d87091eb00a26dcf358060dba Mon Sep 17 00:00:00 2001 > From: Fraser Tweedale > Date: Thu, 21 Jul 2016 16:27:49 +1000 > Subject: [PATCH 92/94] Move GeneralName parsing code to ipalib.x509 > > GeneralName parsing code is primarily relevant to X.509. An > upcoming change will add SAN parsing to the cert-show command, so > first move the GeneralName parsing code from ipalib.pkcs10 to > ipalib.x509. > > Part of: https://fedorahosted.org/freeipa/ticket/6022 > --- > ipalib/pkcs10.py | 93 ++--- > ipalib/x509.py| 114 > +- > ipaserver/plugins/cert.py | 8 ++-- > 3 files changed, 120 insertions(+), 95 deletions(-) > > diff --git a/ipalib/pkcs10.py b/ipalib/pkcs10.py > index > e340c1a2005ad781542a32a0a76753e80364dacf..158ebb3a25be2bd292f3883545fe8afe49b7be8c > 100644 > --- a/ipalib/pkcs10.py > +++ b/ipalib/pkcs10.py > @@ -22,9 +22,10 @@ from __future__ import print_function > import sys > import base64 > import nss.nss as nss > -from pyasn1.type import univ, char, namedtype, tag > +from pyasn1.type import univ, namedtype, tag > from pyasn1.codec.der import decoder > import six > +from ipalib import x509 > > if six.PY3: > unicode = str > @@ -32,11 +33,6 @@ if six.PY3: > PEM = 0 > DER = 1 > > -SAN_DNSNAME = 'DNS name' > -SAN_RFC822NAME = 'RFC822 Name' > -SAN_OTHERNAME_UPN = 'Other Name (OID.1.3.6.1.4.1.311.20.2.3)' > -SAN_OTHERNAME_KRB5PRINCIPALNAME = 'Other Name (OID.1.3.6.1.5.2.2)' > - > def get_subject(csr, datatype=PEM): > """ > Given a CSR return the subject value. > @@ -72,78 +68,6 @@ def get_extensions(csr, datatype=PEM): > return tuple(get_prefixed_oid_str(ext)[4:] > for ext in request.extensions) > > -class _PrincipalName(univ.Sequence): > -componentType = namedtype.NamedTypes( > -namedtype.NamedType('name-type', univ.Integer().subtype( > -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 0)) > -), > -namedtype.NamedType('name-string', > univ.SequenceOf(char.GeneralString()).subtype( > -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 1)) > -), > -) > - > -class _KRB5PrincipalName(univ.Sequence): > -componentType = namedtype.NamedTypes( > -namedtype.NamedType('realm', char.GeneralString().subtype( > -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 0)) > -), > -namedtype.NamedType('principalName', _PrincipalName().subtype( > -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 1)) > -), > -) > - > -def _decode_krb5principalname(data): > -principal = decoder.decode(data, asn1Spec=_KRB5PrincipalName())[0] > -realm = (str(principal['realm']).replace('\\', '') > -.replace('@', '\\@')) > -name = principal['principalName']['name-string'] > -name = '/'.join(str(n).replace('\\', '') > - .replace('/', '\\/') > -
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
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
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
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
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
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". > > 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. > (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). > > > > > -- > Jan Cholasta > > -- > 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 -- 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
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
Thanks for reviews. Rebased and updated patches attached (and one new patch). No substantive changes to 92..94. Patch order is: 92-2, 93-2, 94-2, 98, 90-3 Other comments inline. Thanks, Fraser On Fri, Aug 12, 2016 at 11:33:28AM +0200, Jan Cholasta wrote: > 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. > Updated to use "OID:b64(DER)" as the attribute value. > 2) With --all, san_other should be included in the result for all > otherNames, even the known ones, to provide (limited) forward compatibility. > Done; when --all given, known otherName kinds are included in 'san_other' attribute in addition to their own attribute. > 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). > Yeah, why not support them all? See also Petr's comments in other branch of thread. > 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). > I now pass the value to the constructor of whatever type the parameter uses: attr_value = self.params[attr_name].type(name_formatted) obj.setdefault(attr_name, []).append(attr_value) From 17e5515ab0eeb92d87091eb00a26dcf358060dba Mon Sep 17 00:00:00 2001 From: Fraser Tweedale Date: Thu, 21 Jul 2016 16:27:49 +1000 Subject: [PATCH 92/94] Move GeneralName parsing code to ipalib.x509 GeneralName parsing code is primarily relevant to X.509. An upcoming change will add SAN parsing to the cert-show command, so first move the GeneralName parsing code from ipalib.pkcs10 to ipalib.x509. Part of: https://fedorahosted.org/freeipa/ticket/6022 --- ipalib/pkcs10.py | 93 ++--- ipalib/x509.py| 114 +- ipaserver/plugins/cert.py | 8 ++-- 3 files changed, 120 insertions(+), 95 deletions(-) diff --git a/ipalib/pkcs10.py b/ipalib/pkcs10.py index e340c1a2005ad781542a32a0a76753e80364dacf..158ebb3a25be2bd292f3883545fe8afe49b7be8c 100644 --- a/ipalib/pkcs10.py +++ b/ipalib/pkcs10.py @@ -22,9 +22,10 @@ from __future__ import print_function import sys import base64 import nss.nss as nss -from pyasn1.type import univ, char, namedtype, tag +from pyasn1.type import univ, namedtype, tag from pyasn1.codec.der import decoder import six +from ipalib import x509 if six.PY3: unicode = str @@ -32,11 +33,6 @@ if six.PY3: PEM = 0 DER = 1 -SAN_DNSNAME = 'DNS name' -SAN_RFC822NAME = 'RFC822 Name' -SAN_OTHERNAME_UPN = 'Other Name (OID.1.3.6.1.4.1.311.20.2.3)' -SAN_OTHERNAME_KRB5PRINCIPALNAME = 'Other Name (OID.1.3.6.1.5.2.2)' - def get_subject(csr, datatype=PEM): """ Given a CSR return the subject value. @@ -72,78 +68,6 @@ def get_extensions(csr, datatype=PEM): return tuple(get_prefixed_oid_str(ext)[4:] for ext in request.extensions) -class _PrincipalName(univ.Sequence): -componentType = namedtype.NamedTypes( -namedtype.NamedType('name-type', univ.Integer().subtype( -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 0)) -), -namedtype.NamedType('name-string', univ.SequenceOf(char.GeneralString()).subtype( -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 1)) -), -) - -class _KRB5PrincipalName(univ.Sequence): -componentType = namedtype.NamedTypes( -namedtype.NamedType('realm', char.GeneralString().subtype( -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 0)) -), -namedtype.NamedType('principalName', _PrincipalName().subtype( -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 1)) -), -) - -def _decode_krb5principalname(data): -principal = decoder.decode(data, asn1Spec=_KRB5PrincipalName())[0] -realm = (str(principal['realm']).replace('\\', '') -.replace('@', '\\@')) -name = principal['principalName']['name-string'] -name = '/'.join(str(n).replace('\\', '') - .replace('/', '\\/') - .replace('@', '\\@') for n in name) -name = '%s@%s' % (name, realm) -return name - -class _AnotherName(univ.Sequence): -componentType = namedtype.NamedTypes( -namedtype.NamedType('type-id', univ.ObjectIdentifier()), -namedtype.NamedType('value', univ.Any().subtype( -explicitTag=tag.Tag(tag.tagClassContext, ta
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
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 -- 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
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
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). -- 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
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
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). 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 -- 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
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
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 -- Petr Vobornik -- 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
Re: [Freeipa-devel] [PATCH] 0090, 0092..0094 cert-show: show subject alternative names
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 From 0f85eba4efdd9281725c54268e8d213c412edebf Mon Sep 17 00:00:00 2001 From: Fraser Tweedale Date: Thu, 21 Jul 2016 16:27:49 +1000 Subject: [PATCH 92/94] Move GeneralName parsing code to ipalib.x509 GeneralName parsing code is primarily relevant to X.509. An upcoming change will add SAN parsing to the cert-show command, so first move the GeneralName parsing code from ipalib.pkcs10 to ipalib.x509. Part of: https://fedorahosted.org/freeipa/ticket/6022 --- ipalib/pkcs10.py | 93 ++--- ipalib/x509.py| 114 +- ipaserver/plugins/cert.py | 8 ++-- 3 files changed, 120 insertions(+), 95 deletions(-) diff --git a/ipalib/pkcs10.py b/ipalib/pkcs10.py index e340c1a2005ad781542a32a0a76753e80364dacf..158ebb3a25be2bd292f3883545fe8afe49b7be8c 100644 --- a/ipalib/pkcs10.py +++ b/ipalib/pkcs10.py @@ -22,9 +22,10 @@ from __future__ import print_function import sys import base64 import nss.nss as nss -from pyasn1.type import univ, char, namedtype, tag +from pyasn1.type import univ, namedtype, tag from pyasn1.codec.der import decoder import six +from ipalib import x509 if six.PY3: unicode = str @@ -32,11 +33,6 @@ if six.PY3: PEM = 0 DER = 1 -SAN_DNSNAME = 'DNS name' -SAN_RFC822NAME = 'RFC822 Name' -SAN_OTHERNAME_UPN = 'Other Name (OID.1.3.6.1.4.1.311.20.2.3)' -SAN_OTHERNAME_KRB5PRINCIPALNAME = 'Other Name (OID.1.3.6.1.5.2.2)' - def get_subject(csr, datatype=PEM): """ Given a CSR return the subject value. @@ -72,78 +68,6 @@ def get_extensions(csr, datatype=PEM): return tuple(get_prefixed_oid_str(ext)[4:] for ext in request.extensions) -class _PrincipalName(univ.Sequence): -componentType = namedtype.NamedTypes( -namedtype.NamedType('name-type', univ.Integer().subtype( -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 0)) -), -namedtype.NamedType('name-string', univ.SequenceOf(char.GeneralString()).subtype( -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 1)) -), -) - -class _KRB5PrincipalName(univ.Sequence): -componentType = namedtype.NamedTypes( -namedtype.NamedType('realm', char.GeneralString().subtype( -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 0)) -), -namedtype.NamedType('principalName', _PrincipalName().subtype( -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 1)) -), -) - -def _decode_krb5principalname(data): -principal = decoder.decode(data, asn1Spec=_KRB5PrincipalName())[0] -realm = (str(principal['realm']).replace('\\', '') -.replace('@', '\\@')) -name = principal['principalName']['name-string'] -name = '/'.join(str(n).replace('\\', '') - .replace('/', '\\/') - .replace('@', '\\@') for n in name) -name = '%s@%s' % (name, realm) -return name - -class _AnotherName(univ.Sequence): -componentType = namedtype.NamedTypes( -namedtype.NamedType('type-id', univ.ObjectIdentifier()), -namedtype.NamedType('value', univ.Any().subtype( -explicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 0)) -), -) - -class _GeneralName(univ.Choice): -componentType = namedtype.NamedTypes( -namedtype.NamedType('otherName', _AnotherName().subtype( -implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 0)) -), -namedtype.NamedType('rfc822Name', char.IA5String().subtype( -implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 1)) -), -namedtype.NamedType('dNSName', char.IA5String().subtype( -implicitTag=tag.Tag(tag.tagClassContext, tag.tagFormatSimple, 2)) -), -namedtype.NamedType('x400Address', univ.Sequence().s