On 20.1.2014 18:35, Simo Sorce wrote:
On Mon, 2014-01-20 at 17:49 +0100, Jan Cholasta wrote:
On 20.1.2014 16:36, Simo Sorce wrote:
On Mon, 2014-01-20 at 11:07 +0100, Jan Cholasta wrote:
On 17.1.2014 11:39, Jan Cholasta wrote:
On 10.1.2014 13:34, Martin Kosek wrote:
On 01/09/2014 04:49 PM, Simo Sorce wrote:
On Thu, 2014-01-09 at 10:44 -0500, Rob Crittenden wrote:
Martin Kosek wrote:
On 01/09/2014 03:12 PM, Simo Sorce wrote:


Also maybe we should allow admins to bypass the need to have an
actual
object to represent the alt name ?

I'd rather not. This would allow a rogue admin to create a cert for
www.google.com. Sure, they could also create a host for that but
forcing
them to add more entries increases the chances of them getting caught
doing it.

They can remove the host right after they create a cert, I honestly do
not think this is a valid concern. If your admin is rouge he can already
take full ownership of your infrastructure in many ways, preventing
setting a name in a cert doesn't really make a difference IMO.

However I would be ok to limit this to some new "Security Admin/CA
Admin" role that is not assigned by default.

Simo.


Ok, let's reach some conclusion here. I would really like to not defer
this
feature for too long, it is quite wanted. Would creating new virtual
operation
"Request certificate with SAN" make the situation better? It would not
be so
difficult to do, the check_access function can already access virtual
operation
name as a parameter, we just need to call it.

Why don't we treat SAN hostnames the same way as the subject hostname?
The way I see it, with SAN the only difference is that there is a set of
hostnames instead of just a single hostname, so maybe we should support
requesting a certificate for a set of hosts/services instead of just a
single host/service.

As far as authorization is concerned, currently you can request a
certificate for a single host/service, if you have the "Request
certificate" permission and write access to the host/service entry. With
multiple hosts/services, you would be able to request a certificate if
you have the "Request certificate" permission and write access to *all*
of the host/certificate entries you are requesting the certificate for.

Effectively this means that cert-request would accept multiple
principals instead of single principal and the automatic revocation code
in cert-request, host-del and service-del would take into account that a
single certificate might be assigned to multiple entities.


See attachment for patch which implements the above design.

Hi Jan, I am a bit too far removed from the code to fully understand the
change just from the diff.

Could you please add a short explanation in the commit message, a bout
what the code does now differently than it did before.

Done.

For example although I understand the checks you do on subjectname
+subjectaltname I do not know where the principals come from or why you
have a comment that says principals must all be of the same service
type.

Principals come from the --principal option, which can have multiple
values with the patch.

The service name constraint is there so that there is a 1-to-1 mapping
between principals and hostnames in the request (both subject and SAN).
Without this you would be able to request a certificate for completely
unrelated services and I was not sure if it's OK to allow that, since
the use case here is load balancing (right?)


Simo.



Updated patch attached.


Sorry have to NACK.

I was confused by the code so asked on IRC:
<simo> so if I have subectname = one.ipa.lan and altname = two.ipa.net
then the certificate is anchored to both HTTP/one.ipa.net  AND
HTTP/two.ipa.net ?
<jcholast> yep
<simo> and what happens when I create other.ipa.net with altname
two.ipa.net ?
<simo> does HTTP/two.ipa.net now have 2 certificates anchored ?
<jcholast> the old certificate gets revoked and removed from
HTTP/one.ipa.net and HTTP/two.ipa.net, then the new certificate is
issued and anchored to HTTP/two.ipa.net and HTTP/other.ipa.net

This defeats the purpose of having altnames.

There are 2 reasons to have altnames:
1. just add aliases that are not shared by any other service
2. have a common alias between multiple service to allow loadbalancing

(1) will not be affected, but (2) would be impossible, because as soon
as I try to create the cert for one of the other nodes to be balanced
the previous nodes get their certificates revoked, which defeats the
whole point of creating them with a common altname.

Simo.


Updated patches attached.

--
Jan Cholasta
>From 8c0d512b077b1e62af1be5c11f786b8458856e3b Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Thu, 5 Dec 2013 14:34:14 +0100
Subject: [PATCH 1/2] Allow SAN in IPA certificate profile.

https://fedorahosted.org/freeipa/ticket/3977
---
 install/tools/ipa-upgradeconfig |  7 +++++-
 ipaserver/install/cainstance.py | 51 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig
index ed4852c..e0951e1 100644
--- a/install/tools/ipa-upgradeconfig
+++ b/install/tools/ipa-upgradeconfig
@@ -328,9 +328,14 @@ def upgrade_ipa_profile(ca, domain, fqdn):
             root_logger.debug('Subject Key Identifier updated.')
         else:
             root_logger.debug('Subject Key Identifier already set.')
+        san = ca.enable_subject_alternative_name()
+        if san:
+            root_logger.debug('Subject Alternative Name updated.')
+        else:
+            root_logger.debug('Subject Alternative Name already set.')
         audit = ca.set_audit_renewal()
         uri = ca.set_crl_ocsp_extensions(domain, fqdn)
-        if audit or ski or uri:
+        if audit or ski or san or uri:
             return True
     else:
         root_logger.info('CA is not configured')
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 52c91b6..d1a4d45 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -460,6 +460,7 @@ class CAInstance(service.Service):
             self.step("setting up signing cert profile", self.__setup_sign_profile)
             self.step("set certificate subject base", self.__set_subject_in_config)
             self.step("enabling Subject Key Identifier", self.enable_subject_key_identifier)
+            self.step("enabling Subject Alternative Name", self.enable_subject_alternative_name)
             self.step("enabling CRL and OCSP extensions for certificates", self.__set_crl_ocsp_extensions)
             self.step("setting audit signing renewal to 2 years", self.set_audit_renewal)
             self.step("configuring certificate server to start on boot", self.__enable)
@@ -1207,6 +1208,8 @@ class CAInstance(service.Service):
             new_set_list = '1,2,3,4,5,6,7,8,9'
         elif setlist == '1,2,3,4,5,6,7,8,10':
             new_set_list = '1,2,3,4,5,6,7,8,9,10'
+        elif setlist == '1,2,3,4,5,6,7,8,10,11':
+            new_set_list = '1,2,3,4,5,6,7,8,9,10,11'
 
         if new_set_list:
             installutils.set_directive(self.dogtag_constants.IPA_SERVICE_PROFILE,
@@ -1526,6 +1529,54 @@ class CAInstance(service.Service):
         # No update was done
         return False
 
+    def enable_subject_alternative_name(self):
+        """
+        See if Subject Alternative Name is set in the profile and if not, add
+        it.
+        """
+        setlist = installutils.get_directive(
+            self.dogtag_constants.IPA_SERVICE_PROFILE,
+            'policyset.serverCertSet.list', separator='=')
+
+        # this is the default setting from pki-ca/pki-tomcat. Don't touch it
+        # if a user has manually modified it.
+        if setlist == '1,2,3,4,5,6,7,8,10' or setlist == '1,2,3,4,5,6,7,8,9,10':
+            setlist = setlist + ',11'
+            installutils.set_directive(
+                self.dogtag_constants.IPA_SERVICE_PROFILE,
+                'policyset.serverCertSet.list',
+                setlist,
+                quotes=False, separator='=')
+            installutils.set_directive(
+                self.dogtag_constants.IPA_SERVICE_PROFILE,
+                'policyset.serverCertSet.11.constraint.class_id',
+                'noConstraintImpl',
+                quotes=False, separator='=')
+            installutils.set_directive(
+                self.dogtag_constants.IPA_SERVICE_PROFILE,
+                'policyset.serverCertSet.11.constraint.name',
+                'No Constraint',
+                quotes=False, separator='=')
+            installutils.set_directive(
+                self.dogtag_constants.IPA_SERVICE_PROFILE,
+                'policyset.serverCertSet.11.default.class_id',
+                'userExtensionDefaultImpl',
+                quotes=False, separator='=')
+            installutils.set_directive(
+                self.dogtag_constants.IPA_SERVICE_PROFILE,
+                'policyset.serverCertSet.11.default.name',
+                'User Supplied Extension Default',
+                quotes=False, separator='=')
+            installutils.set_directive(
+                self.dogtag_constants.IPA_SERVICE_PROFILE,
+                'policyset.serverCertSet.11.default.params.userExtOID',
+                '2.5.29.17',
+                quotes=False, separator='=')
+            return True
+
+        # No update was done
+        return False
+
     def set_audit_renewal(self):
         """
         The default renewal time for the audit signing certificate is
-- 
1.8.4.2

>From eb06dd1ea6142e640ac14669ee55270f3881ceb7 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 21 Jan 2014 13:38:54 +0100
Subject: [PATCH 2/2] Support requests with SAN in cert-request.

For each SAN in a request there must be a matching service entry. Users can
request certificates with SAN only if they have "Request Certificate With
SubjectAltName" permission. Hosts can request certificates with SAN only if
they have write access to all of the matching service entries.

https://fedorahosted.org/freeipa/ticket/3977
---
 install/updates/40-delegation.update | 15 ++++++++++
 ipalib/plugins/cert.py               | 54 +++++++++++++++++++++++-------------
 2 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/install/updates/40-delegation.update b/install/updates/40-delegation.update
index 3fabdf9..e1addf0 100644
--- a/install/updates/40-delegation.update
+++ b/install/updates/40-delegation.update
@@ -392,3 +392,18 @@ default:ipapermissiontype: SYSTEM
 
 dn: cn=config
 add:aci: '(target = "ldap:///cn=automember rebuild membership,cn=tasks,cn=config")(targetattr=*)(version 3.0;acl "permission:Add Automember Rebuild Membership Task";allow (add) groupdn = "ldap:///cn=Add Automember Rebuild Membership Task,cn=permissions,cn=pbac,$SUFFIX";)'
+
+# Request Certificate With SubjectAltName virtual op
+dn: cn=request certificate with subjectaltname,cn=virtual operations,cn=etc,$SUFFIX
+default:objectClass: top
+default:objectClass: nsContainer
+default:cn: request certificate with subjectaltname
+
+dn: cn=Request Certificate With SubjectAltName,cn=permissions,cn=pbac,$SUFFIX
+default:objectClass: top
+default:objectClass: groupofnames
+default:objectClass: ipapermission
+default:cn: Request Certificate With SubjectAltName
+
+dn: $SUFFIX
+add:aci:'(targetattr = "objectclass")(target = "ldap:///cn=request certificate with subjectaltname,cn=virtual operations,cn=etc,$SUFFIX" )(version 3.0; acl "permission:Request Certificate With SubjectAltName"; allow (write) groupdn = "ldap:///cn=Request Certificate With SubjectAltName,cn=permissions,cn=pbac,$SUFFIX";)'
diff --git a/ipalib/plugins/cert.py b/ipalib/plugins/cert.py
index 762f13b..887e7b5 100644
--- a/ipalib/plugins/cert.py
+++ b/ipalib/plugins/cert.py
@@ -319,15 +319,27 @@ class cert_request(VirtualCommand):
         taskgroup (directly or indirectly via role membership).
         """
 
+        request = None
+        try:
+            request = pkcs10.load_certificate_request(csr)
+            subject = pkcs10.get_subject(request)
+            subjectaltname = pkcs10.get_subjectaltname(request)
+        except NSPRError, e:
+            raise errors.CertificateOperationError(
+                error=_("Failure decoding Certificate Signing Request: %s") % e)
+        finally:
+            if request is not None:
+                del request
+
         bind_principal = getattr(context, 'principal')
         # Can this user request certs?
         if not bind_principal.startswith('host/'):
             self.check_access()
-
-        # FIXME: add support for subject alt name
+            if subjectaltname is not None:
+                self.check_access('request certificate with subjectaltname')
 
         # Ensure that the hostname in the CSR matches the principal
-        subject_host = get_csr_hostname(csr)
+        subject_host = subject.common_name
         if not subject_host:
             raise errors.ValidationError(name='csr',
                 error=_("No hostname was found in subject of request."))
@@ -344,23 +356,21 @@ class cert_request(VirtualCommand):
         # See if the service exists and punt if it doesn't and we aren't
         # going to add it
         try:
-            if not principal.startswith('host/'):
-                service = api.Command['service_show'](principal, all=True)['result']
-                dn = service['dn']
+            if servicename != 'host':
+                service = api.Command['service_show'](principal, all=True)
             else:
-                hostname = get_host_from_principal(principal)
-                service = api.Command['host_show'](hostname, all=True)['result']
-                dn = service['dn']
+                service = api.Command['host_show'](hostname, all=True)
         except errors.NotFound, e:
             if not add:
                 raise errors.NotFound(reason=_("The service principal for "
                     "this request doesn't exist."))
             try:
-                service = api.Command['service_add'](principal, **{'force': True})['result']
-                dn = service['dn']
+                service = api.Command['service_add'](principal, force=True)
             except errors.ACIError:
                 raise errors.ACIError(info=_('You need to be a member of '
                     'the serviceadmin role to add services'))
+        service = service['result']
+        dn = service['dn']
 
         # We got this far so the service entry exists, can we write it?
         if not ldap.can_write(dn, "usercertificate"):
@@ -368,23 +378,27 @@ class cert_request(VirtualCommand):
                 "to the 'userCertificate' attribute of entry '%s'.") % dn)
 
         # Validate the subject alt name, if any
-        request = pkcs10.load_certificate_request(csr)
-        subjectaltname = pkcs10.get_subjectaltname(request)
         if subjectaltname is not None:
             for name in subjectaltname:
                 name = unicode(name)
                 try:
-                    hostentry = api.Command['host_show'](name, all=True)['result']
-                    hostdn = hostentry['dn']
+                    if servicename == 'host':
+                        altservice = api.Command['host_show'](name, all=True)
+                    else:
+                        altprincipal = '%s/%s@%s' % (servicename, name, realm)
+                        altservice = api.Command['service_show'](
+                            altprincipal, all=True)
                 except errors.NotFound:
                     # We don't want to issue any certificates referencing
                     # machines we don't know about. Nothing is stored in this
                     # host record related to this certificate.
-                    raise errors.NotFound(reason=_('no host record for '
-                        'subject alt name %s in certificate request') % name)
-                authprincipal = getattr(context, 'principal')
-                if authprincipal.startswith("host/"):
-                    if not hostdn in service.get('managedby_host', []):
+                    raise errors.NotFound(reason=_('The service principal for '
+                        'subject alt name %s in certificate request does not '
+                        'exist') % name)
+                altservice = altservice['result']
+                altdn = altservice['dn']
+                if bind_principal.startswith("host/"):
+                    if not ldap.can_write(altdn, "usercertificate"):
                         raise errors.ACIError(info=_(
                             "Insufficient privilege to create a certificate "
                             "with subject alt name '%s'.") % name)
-- 
1.8.4.2

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to