[Freeipa-devel] [PATCH] 210 Allow SAN in IPA certificate profile
Jan Cholasta
jcholast at redhat.com
Mon Jun 16 12:57:00 UTC 2014
On 16.6.2014 13:31, Martin Kosek wrote:
> On 06/11/2014 02:59 PM, Jan Cholasta wrote:
>> On 11.6.2014 13:29, Martin Kosek wrote:
>>> On 06/11/2014 10:58 AM, Jan Cholasta wrote:
>>>> On 10.6.2014 09:55, Martin Kosek wrote:
>>>>> On 06/06/2014 12:50 PM, Jan Cholasta wrote:
>>>>>> On 23.1.2014 14:34, Jan Cholasta wrote:
>>>>>>> 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.
>>>>>>>
>>>>>>
>>>>>> Bump.
>>>>>>
>>>>>> I have added stricter validation of subject alt names as well as certificate
>>>>>> extensions in general to the second patch.
>>>>>
>>>>> Thanks!
>>>>>
>>>>>> Updated patches attached.
>>>>>>
>>>>>> Note that you will need python-nss 0.15 in order to test, you can get a
>>>>>> RPM for
>>>>>> Fedora here: <http://koji.fedoraproject.org/koji/buildinfo?buildID=514739>.
>>>>>
>>>>> John, do you think we could build python-nss 0.15 also for Fedora 20? The fix
>>>>> incorporated in it is pretty important to warrant bugfix update in bodhi. It
>>>>> would also allow FreeIPA 4.0 to be installed on Fedora 20.
>>>>>
>>>>>> Also, resubmitting HTTP and LDAP Server-Cert certmonger requests does not
>>>>>> work,
>>>>>> because <https://fedorahosted.org/freeipa/ticket/4370>.
>>>>>
>>>>> This worked for me, I suspect this is a DS bug. More info in the ticket.
>>>>>
>>>>> Now back to review of the patch:
>>>>>
>>>>> 210.4: looks ok
>>>>> 234.4:
>>>>>
>>>>> 1) Virtual operation "request certificate with subjectaltname" should be a
>>>>> member of Certificate Administrators privilege
>>>>
>>>> OK.
>>>>
>>>>>
>>>>>
>>>>> 2) I would write "Request Certificate With SubjectAltName" with "with" instead
>>>>> of "With". I.e.:
>>>>> Request Certificate with SubjectAltName
>>>>
>>>> OK.
>>>>
>>>>>
>>>>>
>>>>> 3) Why is the extension check only enforced for non-hosts?
>>>>>
>>>>> + if not bind_principal.startswith('host/'):
>>>>> + for ext in extensions:
>>>>> + operation = self._allowed_extensions.get(ext)
>>>>> + if operation:
>>>>> + self.check_access(operation)
>>>>>
>>>>> My tricky certmonger request passed:
>>>>>
>>>>> # ipa-getcert request -d /etc/pki/nssdb/ -n test-san-1 -I test-san-1 -g
>>>>> 2048 -r
>>>>> -N "cn=`hostname`" -K host/`hostname` -D test1.example.com -D
>>>>> test2.example.com
>>>>> -E mkosek at redhat.com
>>>>>
>>>>> while when I posted the same CSR as privileged user, it was rejected:
>>>>>
>>>>> $ klist
>>>>> Ticket cache: KEYRING:persistent:962000003:962000003
>>>>> Default principal: fbar at IDM.LAB.ENG.BRQ.REDHAT.COM
>>>>>
>>>>> $ ipa cert-request --principal test/`hostname` certmonger.csr
>>>>> ipa: ERROR: invalid 'csr': subject alt name type RFC822 Name is forbidden
>>>>
>>>> Right, I meant to NACK myself, but you were faster. Fixed.
>>>>
>>>>>
>>>>>
>>>>> 4) Regular users cannot read Virtual Operations, so even if I assign them a
>>>>> permission, command is refused:
>>>>>
>>>>> $ ipa cert-request --principal test/`hostname` openssl.csr
>>>>> ipa: ERROR: Insufficient access: No such virtual command
>>>>>
>>>>> I think we will need to create new managed permission "Read Virtual
>>>>> Operations"
>>>>> and assign it to "Certificate Administrators" privilege by default as this
>>>>> privilege has the virtual operation permissions assigned. Petr3, what is your
>>>>> take on this?
>>>>>
>>>>> Otherwise the patch worked well for me, the authorization part looked OK. My
>>>>> biggest concern was just the host/user differentiation wrt unknown
>>>>> subjectaltname types.
>>>>>
>>>>> Martin
>>>>>
>>>>
>>>> Updated patches attached.
>>>>
>>>
>>> 1) I could not compile the patch set:
>>>
>>> $ make rpms
>>> ...
>>> if [ "" != "yes" ]; then \
>>> ./makeapi --validate; \
>>> ./makeaci --validate; \
>>> fi
>>> Traceback (most recent call last):
>>> File "./makeapi", line 457, in <module>
>>> sys.exit(main())
>>> File "./makeapi", line 428, in main
>>> api.finalize()
>>> File "/root/freeipa-master/ipalib/plugable.py", line 708, in finalize
>>> self.__do_if_not_done('load_plugins')
>>> File "/root/freeipa-master/ipalib/plugable.py", line 482, in __do_if_not_done
>>> getattr(self, name)()
>>> File "/root/freeipa-master/ipalib/plugable.py", line 645, in load_plugins
>>> self.import_plugins('ipalib')
>>> File "/root/freeipa-master/ipalib/plugable.py", line 689, in import_plugins
>>> __import__(fullname)
>>> File "/root/freeipa-master/ipalib/plugins/cert.py", line 30, in <module>
>>> from ipalib import pkcs10
>>> File "/root/freeipa-master/ipalib/pkcs10.py", line 77
>>> .replace('@', '\\@')
>>> ...
>>>
>>> The rest of the notes are thus only from reading.
>>
>> Sorry, last minute change gone wrong.
>>
>>>
>>> 2) There are lines like
>>>
>>> + if name_type == 'Other Name (OID.1.3.6.1.5.2.2)':
>>>
>>>
>>> or
>>>
>>> + if name_type not in ('DNS name',
>>> + 'Other Name (OID.1.3.6.1.4.1.311.20.2.3)',
>>> + 'Other Name (OID.1.3.6.1.5.2.2)'):
>>>
>>> or
>>>
>>> + elif name_type in ('Other Name (OID.1.3.6.1.4.1.311.20.2.3)',
>>> + 'Other Name (OID.1.3.6.1.5.2.2)'):
>>>
>>> Can we do something better? Getting the extension type based on it's
>>> description seems extremely unstable to me.
>>
>> These are SAN types, not extension types. Unfortunately the textual
>> descriptions are the only SAN type representation available in python-nss which
>> includes OIDs of other names.
>>
>>>
>>> Can we for example modify get_subjectaltname to get the type based on
>>> CertificateExtension object's oid or oid_tag attributes?
>>
>> No, see above.
>>
>>>
>>> I would rather see get_subjectaltname return solid type like
>>> CERT_EXTENSION_DNS_NAME defined in pkcs10.py than consumers comparing
>>> descriptions. It would be more readable, too.
>>
>> Done.
>>
>>>
>>> Martin
>>>
>>
>> Updated patches attached.
>
> 1) We still miss the managed permission allowing Certificate Administrators to
> read Virtual Commands
I was under the impression that Petr would fix this.
>
>
> 2) Extension check
>
> Wouldn't
>
> if any(ext not in self._allowed_extensions for ext in extensions)
>
> be more readable than
>
> + for ext in extensions:
> + if ext not in self._allowed_extensions:
> + raise errors.ValidationError(
> + name='csr', error=_("extension %s is forbidden") % ext)
>
> ?
I don't think so. I don't see how cramming everything into one line
makes anything more readable.
>
> 3) I find this part still not ideal:
>
> + for name_type, name in subjectaltname:
> + if name_type not in (pkcs10.SAN_DNSNAME,
> + pkcs10.SAN_OTHERNAME_KRB5PRINCIPALNAME,
> + pkcs10.SAN_OTHERNAME_UPN):
> + raise errors.ValidationError(
> + name='csr',
> + error=_("subject alt name type %s is forbidden") %
> + name_type)
>
>
> I still think we should instead:
> - change get_subjectaltname's return value to tuples of:
> - numeric type, i.e. SAN_DNSNAME,SAN_OTHERNAME_UPN or
> SAN_OTHERNAME_KRB5PRINCIPALNAME
> - content
It already returns tuples like this. Using a numeric type instead of
string for named constants does not improve anything.
> - raise error when an unknown SAN type is detected
This doesn't really fit with the rest of the pkcs10 module.
>
> This would:
> a) Allow us to be sure that we do not miss any yet unknown SAN types or allow
> us to catch the situation when name_type string changes
> b) Allow more natural comparison of the SAN type "name_type ==
> pkcs10.SAN_DNSNAME" instead of "name_type in pkcs10.SAN_DNSNAME"
The code already does all of this. There is no "name_type in
pkcs10.SAN_DNSNAME".
>
>
> 3) I am thinking why do we need to introduce all the ASN parsing? I am talking
> about _decode_krb5principalname and others. If we do not use the result
> anywhere, why should we include this part at all?
To work around shortcomings of NSS/python-nss. In particular,
_decode_krb5principalname is used to decode KRB5PrincipalName SANs to a
string. This is necessary because certmonger puts such SANs in
certificate requests it generates.
>
>
> 4) I get crash in the certmonger request:
>
> ipa-getcert request -d /etc/pki/nssdb/ -n test-san-1 -I test-san-1 -g 2048 -r
> -N "cn=`hostname`" -K host/`hostname` -D test1.example.com -D test2.example.com
> -E mkosek at redhat.com
>
> # ipa-getcert list -i test-san-1
> Number of certificates and requests being tracked: 8.
> Request ID 'test-san-1':
> status: CA_UNREACHABLE
> ca-error: Server failed request, will retry: -504 (HTTP response code is 500,
> not 200).
> stuck: yes
>
>
> HTTPD traceback
> [Mon Jun 16 13:06:14.528642 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> Traceback (most recent call last):
> [Mon Jun 16 13:06:14.528844 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/share/ipa/wsgi.py", line 49, in application
> [Mon Jun 16 13:06:14.529466 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> return api.Backend.wsgi_dispatch(environ, start_response)
> [Mon Jun 16 13:06:14.529614 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/ipaserver/rpcserver.py", line 251, in
> __call__
> [Mon Jun 16 13:06:14.553116 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> return self.route(environ, start_response)
> [Mon Jun 16 13:06:14.553261 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/ipaserver/rpcserver.py", line 263, in
> route
> [Mon Jun 16 13:06:14.553410 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> return app(environ, start_response)
> [Mon Jun 16 13:06:14.553604 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/ipaserver/rpcserver.py", line 637, in
> __call__
> [Mon Jun 16 13:06:14.553749 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> environ, start_response)
> [Mon Jun 16 13:06:14.553895 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/ipaserver/rpcserver.py", line 397, in
> __call__
> [Mon Jun 16 13:06:14.554023 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> response = self.wsgi_execute(environ)
> [Mon Jun 16 13:06:14.554138 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/ipaserver/rpcserver.py", line 343, in
> wsgi_execute
> [Mon Jun 16 13:06:14.554276 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> result = self.Command[name](*args, **options)
> [Mon Jun 16 13:06:14.554413 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 439, in __call__
> [Mon Jun 16 13:06:14.585933 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> ret = self.run(*args, **options)
> [Mon Jun 16 13:06:14.586170 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 754, in run
> [Mon Jun 16 13:06:14.586305 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> return self.execute(*args, **options)
> [Mon Jun 16 13:06:14.586490 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/ipalib/plugins/cert.py", line 316, in
> execute
> [Mon Jun 16 13:06:14.592390 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> subjectaltname = pkcs10.get_subjectaltname(csr) or ()
> [Mon Jun 16 13:06:14.592565 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/ipalib/pkcs10.py", line 129, in
> get_subjectaltname
> [Mon Jun 16 13:06:14.596411 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> san = decoder.decode(san, asn1Spec=_SubjectAltName())[0]
> [Mon Jun 16 13:06:14.596555 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/pyasn1/codec/ber/decoder.py", line
> 792, in __call__
> [Mon Jun 16 13:06:14.598268 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> stGetValueDecoder, self, substrateFun
> [Mon Jun 16 13:06:14.598404 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/pyasn1/codec/ber/decoder.py", line
> 367, in valueDecoder
> [Mon Jun 16 13:06:14.598534 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> component, head = decodeFun(head, asn1Spec)
> [Mon Jun 16 13:06:14.598633 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> File "/usr/lib/python2.7/site-packages/pyasn1/codec/ber/decoder.py", line
> 798, in __call__
> [Mon Jun 16 13:06:14.598746 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> '%r not in asn1Spec: %r' % (tagSet, asn1Spec)
> [Mon Jun 16 13:06:14.598954 2014] [:error] [pid 12941] [remote 10.34.47.236:12]
> PyAsn1Error: TagSet(Tag(tagClass=128, tagFormat=0, tagId=2)) not in asn1Spec:
> _GeneralName()
What version of certmonger are you using?
>
>
> Martin
>
--
Jan Cholasta
More information about the Freeipa-devel
mailing list