[Freeipa-devel] [PATCH] 210 Allow SAN in IPA certificate profile

Wed Jun 18 12:09:23 UTC 2014

On 16.6.2014 16:08, Martin Kosek wrote:
> On 06/16/2014 02:57 PM, Jan Cholasta wrote:
>> 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.
>
> Previously I was asking for consultancy, new managed permissions should make it
> much easier to add new ACIs, so I think you should be able to do it. If not,
> you can ask Petr for help.

Added, see patch 300.

>
> ...
>>> 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.
>
> I know, but we do not use the values besides DNS SAN type, right? I was just
> afraid that a decode error in a decoding of an unused SAN type would cause the
> entire CSR processing to crash.

If we do not allow principal name SANs, requests from certmonger will 
fail. If we allow them, but ignore them, you could request a certificate 
for an arbitrary unrelated principal. Thus, the only thing we can do is 
allow them and validate them, which is what the patch does and why 
decoding KRB5PrincipalName is necessary.

>
>
>>> 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?
>
> Latest Fedora 20, i.e. should be certmonger-0.71.2-1.fc20 (do not have access
> to my VM atm). Is this a bug in certmonger?

No, it's bug in my code. Fixed.

>
> I would like FreeIPA 4.0 to run on vanilla Fedora 20, so far it does, the
> required python-nss 0.15 is already on it's way to stable updates.
>
> Thanks,
> Martin
>

Also added patch 301 which fixes a bug in ldap2.get_effective_rights I 
was hitting with patch 234.

Updated patches attached.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-210.7-Allow-SAN-in-IPA-certificate-profile.patch
Type: text/x-patch
Size: 5003 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140618/e362d7d9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-234.7-Support-requests-with-SAN-in-cert-request.patch
Type: text/x-patch
Size: 16019 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140618/e362d7d9/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-300-Add-virtual-operations-read-permission.patch
Type: text/x-patch
Size: 4537 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140618/e362d7d9/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-301-Remove-GetEffectiveRights-control-when-ldap2.get_eff.patch
Type: text/x-patch
Size: 1240 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140618/e362d7d9/attachment-0003.bin>