Thanks Ian!  I'll try and review this in the next couple of days?

Do you use GitHub?  If so, you could create a pull request there,
which will make it more visible, easier to review, and cause CI to
run on your patch.  If not, that's OK.  We are happy to receive your
contribution by any means!

Cheers,
Fraser

On Fri, Mar 09, 2018 at 05:10:52PM -0600, Ian Pilcher via FreeIPA-devel wrote:
> On 03/01/2018 08:52 AM, Rob Crittenden wrote:
> > Ian Pilcher via FreeIPA-devel wrote:
> > 
> > You'd need to ensure that the IP address exists in IPA but that it is
> > owned/managed by the user/host/service making the request.
> 
> Any hints on how to do that?  I don't see any ownership information
> associated with DNS records or IP addresses, but I don't really know
> where I should be looking
> 
> > > FYI, I've been working on the logic for validating the IP addresses in
> > > my not-copious-spare time, and I hope to have something worth discussing
> > > in the next week or so.
> > 
> > Thanks, I look forward to it.
> 
> First draft below.  Looking forward to y'alls' comments.
> 
> From 9e64ec302ad5370a22437d53876ab31f2a237033 Mon Sep 17 00:00:00 2001
> From: Ian Pilcher <arequip...@gmail.com>
> Date: Fri, 9 Mar 2018 12:33:55 -0600
> Subject: [PATCH] Allow issuing certificates with IP addresses in
>  subjectAltName
> 
> Allow issuing certificates with IP addresses in the subject
> alternative name (SAN), if all of the following are true.
> 
> * One of the DNS names in the SAN resolves to the IP address
>   (possibly through a CNAME).
> * All of the DNS entries in the resolution chain are managed by
>   this IPA instance.
> * The IP address has a (correct) reverse DNS entry that is managed
>   by this IPA instance
> ---
>  ipaserver/plugins/cert.py | 92
> +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 89 insertions(+), 3 deletions(-)
> 
> diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py
> index db624357a..452728d21 100644
> --- a/ipaserver/plugins/cert.py
> +++ b/ipaserver/plugins/cert.py
> @@ -22,11 +22,13 @@
>  import base64
>  import collections
>  import datetime
> +import itertools
>  import logging
>  from operator import attrgetter
> 
>  import cryptography.x509
>  from cryptography.hazmat.primitives import hashes, serialization
> +from dns import resolver, reversename
>  import six
> 
>  from ipalib import Command, Str, Int, Flag
> @@ -48,7 +50,7 @@ from .certprofile import validate_profile_id
>  from ipalib.text import _
>  from ipalib.request import context
>  from ipalib import output
> -from ipapython import kerberos
> +from ipapython import dnsutil, kerberos
>  from ipapython.dn import DN
>  from ipaserver.plugins.service import normalize_principal, validate_realm
> 
> @@ -773,9 +775,10 @@ class cert_request(Create, BaseCertMethod,
> VirtualCommand):
>                             "'userCertificate' attribute of entry '%s'.") %
> dn)
> 
>          # Validate the subject alt name, if any
> -        generalnames = []
> +        san_has_ipaddrs = False
> +        generalnames = ()
>          if ext_san is not None:
> -            generalnames = x509.process_othernames(ext_san.value)
> +            generalnames = tuple(x509.process_othernames(ext_san.value))
>          for gn in generalnames:
>              if isinstance(gn, cryptography.x509.general_name.DNSName):
>                  if principal.is_user:
> @@ -858,11 +861,16 @@ class cert_request(Create, BaseCertMethod,
> VirtualCommand):
>                              "subject alt name type %s is forbidden "
>                              "for non-user principals") % "RFC822Name"
>                      )
> +            elif isinstance(gn, cryptography.x509.general_name.IPAddress):
> +                san_has_ipaddrs = True
>              else:
>                  raise errors.ACIError(
>                      info=_("Subject alt name type %s is forbidden")
>                      % type(gn).__name__)
> 
> +        if san_has_ipaddrs:
> +            _validate_san_ips(generalnames)
> +
>          # Request the certificate
>          try:
>              # re-serialise to PEM, in case the user-supplied data has
> @@ -1046,6 +1054,84 @@ def _principal_name_matches_principal(name,
> principal_obj):
>     return principal in principal_obj.get('krbprincipalname', [])
> 
> 
> +def _validate_san_ips(generalnames):
> +    """
> +    Raise a ValidationError if the subjectAltName in a CSR includes
> +    any IP address(es) that do not match a DNS name in the SAN.
> +
> +    """
> +    san_dns_ips = set()
> +    for name in generalnames:
> +        if isinstance(name, cryptography.x509.general_name.DNSName):
> +            san_dns_ips.update(_san_dnsname_ips(name.value))
> +    for name in generalnames:
> +        if isinstance(name, cryptography.x509.general_name.IPAddress):
> +            if unicode(name.value) not in san_dns_ips:
> +                raise errors.ValidationError(
> +                    name='csr',
> +                    error=_(
> +                        "IP address in subjectAltName (%s) does not "
> +                        "match any DNS name"
> +                    ) % name.value
> +                )
> +
> +
> +def _san_dnsname_ips(dnsname, dnsname_is_cname=False):
> +    """
> +    Returns a set of IP addresses, managed by this IPa instance,
> +    that correspond to the DNS name (from the subjectAltName).
> +
> +    """
> +    fqdn = dnsutil.DNSName(dnsname).make_absolute()
> +    if fqdn.__len__() < 4:
> +        logger.debug("Skipping IPs for %s: hostname too short" % dnsname)
> +        return ()
> +    zone = dnsutil.DNSName(resolver.zone_for_name(fqdn))
> +    name = fqdn.relativize(zone)
> +    try:
> +        result = api.Command['dnsrecord_show'](zone, name)['result']
> +    except errors.NotFound as nf:
> +        logger.debug("Skipping IPs for %s: %s" % (dnsname, nf))
> +        return ()
> +    ips = set()
> +    for ip in itertools.chain(result.get('arecord', ()),
> +                              result.get('aaaarecord', ())):
> +        if _ip_rdns_ok(ip, fqdn):
> +            ips.add(ip)
> +    cnames = result.get('cnamerecord', ())
> +    if cnames:
> +        if dnsname_is_cname:
> +            logger.debug("Skipping IPs for %s: chained CNAME" % dnsname)
> +        else:
> +            for cname in cnames:
> +                if not cname.endswith('.'):
> +                    cname = u'%s.%s' % (cname, zone)
> +                ips.update(_san_dnsname_ips(cname, True))
> +    return ips
> +
> +
> +def _ip_rdns_ok(ip, fqdn):
> +    """
> +    Determines whether the IP address has a reverse DNS entry (managed
> +    by this IPA instance) that points to the FQDN.
> +
> +    """
> +    rname = dnsutil.DNSName(reversename.from_address(ip))
> +    zone = dnsutil.DNSName(resolver.zone_for_name(rname))
> +    name = rname.relativize(zone)
> +    try:
> +        result = api.Command['dnsrecord_show'](zone, name)['result']
> +    except errors.NotFound as nf:
> +        logger.debug("Skipping IP %s: reverse DNS record not found" % ip)
> +    # This assumes that the IP only has 1 A/AAAA record pointing to it.
> +    if result['ptrrecord'][0] != fqdn.to_unicode():
> +        logger.debug("Skipping IP: %s: reverse DNS doesn't match FQDN %s"
> +                     % (ip, fqdn))
> +        return False
> +    else:
> +        return True
> +
> +
>  @register()
>  class cert_status(Retrieve, BaseCertMethod, VirtualCommand):
>      __doc__ = _('Check the status of a certificate signing request.')
> -- 
> 2.14.3
> 
> 
> 
> -- 
> ========================================================================
> Ian Pilcher                                         arequip...@gmail.com
> -------- "I grew up before Mark Zuckerberg invented friendship" --------
> ========================================================================

> From 9e64ec302ad5370a22437d53876ab31f2a237033 Mon Sep 17 00:00:00 2001
> From: Ian Pilcher <arequip...@gmail.com>
> Date: Fri, 9 Mar 2018 12:33:55 -0600
> Subject: [PATCH] Allow issuing certificates with IP addresses in
>  subjectAltName
> 
> Allow issuing certificates with IP addresses in the subject
> alternative name (SAN), if all of the following are true.
> 
> * One of the DNS names in the SAN resolves to the IP address
>   (possibly through a CNAME).
> * All of the DNS entries in the resolution chain are managed by
>   this IPA instance.
> * The IP address has a (correct) reverse DNS entry that is managed
>   by this IPA instance
> ---
>  ipaserver/plugins/cert.py | 92 
> +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 89 insertions(+), 3 deletions(-)
> 
> diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py
> index db624357a..452728d21 100644
> --- a/ipaserver/plugins/cert.py
> +++ b/ipaserver/plugins/cert.py
> @@ -22,11 +22,13 @@
>  import base64
>  import collections
>  import datetime
> +import itertools
>  import logging
>  from operator import attrgetter
>  
>  import cryptography.x509
>  from cryptography.hazmat.primitives import hashes, serialization
> +from dns import resolver, reversename
>  import six
>  
>  from ipalib import Command, Str, Int, Flag
> @@ -48,7 +50,7 @@ from .certprofile import validate_profile_id
>  from ipalib.text import _
>  from ipalib.request import context
>  from ipalib import output
> -from ipapython import kerberos
> +from ipapython import dnsutil, kerberos
>  from ipapython.dn import DN
>  from ipaserver.plugins.service import normalize_principal, validate_realm
>  
> @@ -773,9 +775,10 @@ class cert_request(Create, BaseCertMethod, 
> VirtualCommand):
>                             "'userCertificate' attribute of entry '%s'.") % 
> dn)
>  
>          # Validate the subject alt name, if any
> -        generalnames = []
> +        san_has_ipaddrs = False
> +        generalnames = ()
>          if ext_san is not None:
> -            generalnames = x509.process_othernames(ext_san.value)
> +            generalnames = tuple(x509.process_othernames(ext_san.value))
>          for gn in generalnames:
>              if isinstance(gn, cryptography.x509.general_name.DNSName):
>                  if principal.is_user:
> @@ -858,11 +861,16 @@ class cert_request(Create, BaseCertMethod, 
> VirtualCommand):
>                              "subject alt name type %s is forbidden "
>                              "for non-user principals") % "RFC822Name"
>                      )
> +            elif isinstance(gn, cryptography.x509.general_name.IPAddress):
> +                san_has_ipaddrs = True
>              else:
>                  raise errors.ACIError(
>                      info=_("Subject alt name type %s is forbidden")
>                      % type(gn).__name__)
>  
> +        if san_has_ipaddrs:
> +            _validate_san_ips(generalnames)
> +
>          # Request the certificate
>          try:
>              # re-serialise to PEM, in case the user-supplied data has
> @@ -1046,6 +1054,84 @@ def _principal_name_matches_principal(name, 
> principal_obj):
>      return principal in principal_obj.get('krbprincipalname', [])
>  
>  
> +def _validate_san_ips(generalnames):
> +    """
> +    Raise a ValidationError if the subjectAltName in a CSR includes
> +    any IP address(es) that do not match a DNS name in the SAN.
> +
> +    """
> +    san_dns_ips = set()
> +    for name in generalnames:
> +        if isinstance(name, cryptography.x509.general_name.DNSName):
> +            san_dns_ips.update(_san_dnsname_ips(name.value))
> +    for name in generalnames:
> +        if isinstance(name, cryptography.x509.general_name.IPAddress):
> +            if unicode(name.value) not in san_dns_ips:
> +                raise errors.ValidationError(
> +                    name='csr',
> +                    error=_(
> +                        "IP address in subjectAltName (%s) does not "
> +                        "match any DNS name"
> +                    ) % name.value
> +                )
> +
> +
> +def _san_dnsname_ips(dnsname, dnsname_is_cname=False):
> +    """
> +    Returns a set of IP addresses, managed by this IPa instance,
> +    that correspond to the DNS name (from the subjectAltName).
> +
> +    """
> +    fqdn = dnsutil.DNSName(dnsname).make_absolute()
> +    if fqdn.__len__() < 4:
> +        logger.debug("Skipping IPs for %s: hostname too short" % dnsname)
> +        return ()
> +    zone = dnsutil.DNSName(resolver.zone_for_name(fqdn))
> +    name = fqdn.relativize(zone)
> +    try:
> +        result = api.Command['dnsrecord_show'](zone, name)['result']
> +    except errors.NotFound as nf:
> +        logger.debug("Skipping IPs for %s: %s" % (dnsname, nf))
> +        return ()
> +    ips = set()
> +    for ip in itertools.chain(result.get('arecord', ()),
> +                              result.get('aaaarecord', ())):
> +        if _ip_rdns_ok(ip, fqdn):
> +            ips.add(ip)
> +    cnames = result.get('cnamerecord', ())
> +    if cnames:
> +        if dnsname_is_cname:
> +            logger.debug("Skipping IPs for %s: chained CNAME" % dnsname)
> +        else:
> +            for cname in cnames:
> +                if not cname.endswith('.'):
> +                    cname = u'%s.%s' % (cname, zone)
> +                ips.update(_san_dnsname_ips(cname, True))
> +    return ips
> +
> +
> +def _ip_rdns_ok(ip, fqdn):
> +    """
> +    Determines whether the IP address has a reverse DNS entry (managed
> +    by this IPA instance) that points to the FQDN.
> +
> +    """
> +    rname = dnsutil.DNSName(reversename.from_address(ip))
> +    zone = dnsutil.DNSName(resolver.zone_for_name(rname))
> +    name = rname.relativize(zone)
> +    try:
> +        result = api.Command['dnsrecord_show'](zone, name)['result']
> +    except errors.NotFound as nf:
> +        logger.debug("Skipping IP %s: reverse DNS record not found" % ip)
> +    # This assumes that the IP only has 1 A/AAAA record pointing to it.
> +    if result['ptrrecord'][0] != fqdn.to_unicode():
> +        logger.debug("Skipping IP: %s: reverse DNS doesn't match FQDN %s"
> +                     % (ip, fqdn))
> +        return False
> +    else:
> +        return True
> +
> +
>  @register()
>  class cert_status(Retrieve, BaseCertMethod, VirtualCommand):
>      __doc__ = _('Check the status of a certificate signing request.')
> -- 
> 2.14.3
> 

> _______________________________________________
> FreeIPA-devel mailing list -- freeipa-devel@lists.fedorahosted.org
> To unsubscribe send an email to freeipa-devel-le...@lists.fedorahosted.org
_______________________________________________
FreeIPA-devel mailing list -- freeipa-devel@lists.fedorahosted.org
To unsubscribe send an email to freeipa-devel-le...@lists.fedorahosted.org

Reply via email to