On 06/24/2014 11:30 AM, Jan Cholasta wrote: > On 23.6.2014 13:01, Martin Kosek wrote: >> On 06/18/2014 02:09 PM, Jan Cholasta wrote: >> ... >>>>>> 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. >> >> True, we need to make sure the principal type SANs match. ack on the intent. >> >>>>>> 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 >> ... >>>>>> 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. >> >> Works. >> >>> Also added patch 301 which fixes a bug in ldap2.get_effective_rights I was >>> hitting with patch 234. >>> >>> Updated patches attached. >>> >> >> Thanks. Functionally, the patch looks ok to me, we are close to final ack. >> Just >> couple more minor changes need to happen: >> >> 1) We decided to drop ipaVirtualOperation concept in the end and allow people >> reading this list. This implies following changes: >> >> 234 - drop ipaVirtualOperation from cn=request certificate with >> subjectaltname,cn=virtual operations,cn=etc,$SUFFIX >> 300 - drop the entire patch (sorry) > > OK. > >> >> 2) I would like to have more confidence that no unauthorized SAN extension >> gets >> in. I know we have the >> >> + for name_type, name in subjectaltname: >> + if name_type not in (pkcs10.SAN_DNSNAME, >> >> loop, but I would also like to add "else" path to the SAN type validation >> loop, >> in case somebody just expands the part above, but forgot to also add >> validation. I want us to be explicit in this case: >> >> + for name_type, name in subjectaltname: >> + if name_type == pkcs10.SAN_DNSNAME: >> ... >> + elif name_type in (pkcs10.SAN_OTHERNAME_KRB5PRINCIPALNAME, >> + pkcs10.SAN_OTHERNAME_UPN): >> ... >>> else: >>> raise errors.ACIError(info=_( >>> "Unauthorized subject alt name '%s'.") % name) >> > > Fixed. > >> >> When this is fixed, we should be done. >> >> Thanks, >> Martin >> > > Updated and rebased patches attached. >
This looks good. We can finally push this beast. Note that we should also add proper tests in future exercising the new functionality. ACK. Pushed to master: 8b8774d138348ab4b938f98dc106ea983e261262 Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel