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 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 > > 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 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() > > 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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel