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

2016-08-26 Thread Jan Cholasta 

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

2016-08-23 Thread Fraser Tweedale 
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(
-

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

2016-08-22 Thread Jan Cholasta 

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

2016-08-16 Thread 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

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

2016-08-15 Thread Petr Spacek 
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

2016-08-15 Thread Fraser Tweedale 
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

2016-08-15 Thread Fraser Tweedale 
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(
-

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

2016-08-14 Thread Jan Cholasta 

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

2016-08-12 Thread 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).

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

2016-08-12 Thread Jan Cholasta 

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

2016-08-04 Thread Petr Vobornik 
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

2016-07-21 Thread Fraser Tweedale 
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))
-),
-