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 mko...@redhat.com
>>>>
>>>> while when I posted the same CSR as privileged user, it was rejected:
>>>>
>>>> $ klist
>>>> Ticket cache: KEYRING:persistent:962000003:962000003
>>>> Default principal: f...@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 mko...@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

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

Reply via email to