[Freeipa-devel] [PATCH] 210 Allow SAN in IPA certificate profile
Martin Kosek
mkosek at redhat.com
Mon Jun 16 11:31:11 UTC 2014
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
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)
?
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
- raise error when an unknown SAN type is detected
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"
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?
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()
Martin
More information about the Freeipa-devel
mailing list