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

Mon Jun 16 14:08:36 UTC 2014

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.

...
>> 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.

>> 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?

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