On 22.1.2014 16:43, Simo Sorce wrote:
On Wed, 2014-01-22 at 16:05 +0100, Jan Cholasta wrote:
On 22.1.2014 15:34, Simo Sorce wrote:
On Wed, 2014-01-22 at 10:40 +0100, Jan Cholasta wrote:
On 21.1.2014 17:12, Simo Sorce wrote:
Later in the patch you seem to be changing from needing managedby_host
to needing write access to an entry, I am not sure I understand why that
was changed. not saying it is necessarily wrong,  but why the original
check is not right anymore ?


The original check is wrong, see
<https://fedorahosted.org/freeipa/ticket/3977#comment:23>.

The check in my patch allows SAN only if the requesting host has write
access to all of the SAN services. I'm not entirely sure if this is
right, but even if it is not, I think we should still check for write
access to the SAN services, so that access control can be (partially)
handled by ACIs.

Right, I remembered that comment, but it just says to check the right
object's managed-by, here instead you changed it to check if you can
write the usercertificate.

I guess it is the same *if* there is an ACI that gives write permission
when the host is in the managed-by attribute, is that the reasoning ?

Exactly. The ACIs that allow this by default are named "Hosts can manage
service Certificates and kerberos keys" and "Hosts can manage other host
Certificates and kerberos keys".

I think the check can be extended to users as well, so that requesting
certificate with SAN is allowed only to users which have write access to
the SAN services.

I have done the modification, see attached patches.


Sounds good to me then, thanks for explaining.

The patches also look good, but I would like someone to give them a try
for a formal ack.

OK, thanks.


Simo.


--
Jan Cholasta
>From 49c49dbd4cb004934cbc3b66aad382086a5a7b8a 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 b281eb4..a9220c1 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.5.3

>From 48589dfd97a54837834464364c093c91a6b8f552 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               | 59 ++++++++++++++++++++++--------------
 2 files changed, 51 insertions(+), 23 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..3cf0a44 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,26 +378,29 @@ 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.ACIError(info=_(
-                            "Insufficient privilege to create a certificate "
-                            "with subject alt name '%s'.") % name)
+                    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 not ldap.can_write(altdn, "usercertificate"):
+                    raise errors.ACIError(info=_(
+                        "Insufficient privilege to create a certificate with "
+                        "subject alt name '%s'.") % name)
 
         if 'usercertificate' in service:
             serial = x509.get_serial_number(service['usercertificate'][0], datatype=x509.DER)
-- 
1.8.5.3

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

Reply via email to