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

[Martin Kosek]

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