[Freeipa-devel] V4/Sub-CAs review

Mon Jun 6 06:29:16 UTC 2016

On 1.6.2016 06:49, Fraser Tweedale wrote:
> On Mon, May 23, 2016 at 10:02:44AM +0200, Jan Cholasta wrote:
>>>>>> 2) <http://www.freeipa.org/page/V4/Sub-CAs#Sub-CA_plugin>
>>>>>>
>>>>>> It should be mentioned here that the primary CA is also handled by this
>>>>>> plugin.
>>>>>>
>>>>>> I would like to propose two additional fields:
>>>>>>
>>>>>>   * subject (required) - subject name of the CA, to be able to look up
>>>>>> sub-CA that issued a certificate from its issuer name.
>>>>>>
>>>>>>   * issuer_ca (optional) - name of the sub-CA which issued certificate for
>>>>>> this CA, to have information about the sub-CA hierarchy. If there is no
>>>>>> sub-CA entry for the issuer, it would be unset.
>>>>>>
>>>>> These data exist in the Dogtag database.  Adding them to IPA might
>>>>> be useful for avoiding round trips so it is probably worth doing,
>>>>> but are you aware of use cases that would absolutely require them?
>>>>
>>>> As for subject, we are adding the ability to look up information about
>>>> arbitrary certificates to cert-{show,find} as part of
>>>> <https://fedorahosted.org/freeipa/ticket/5381>. Part of this information
>>>> should be whether the certificate was issued by our CA and what CA it was,
>>>> so that the web UI can present an appropriate "revoke certificate" button
>>>> for the certificate.
>>>>
>>>> As for issuer CA, I believe we need it to fix automatic CA certificate
>>>> renewal. The current renewal code uses virtual "profiles" to handle CA
>>>> certificate renewal, but that turned out to be an issue, especially with
>>>> externally signed CA certificates:
>>>> <https://bugzilla.redhat.com/show_bug.cgi?id=1322963>. Instead it could use
>>>> the issuer CA information from LDAP to figure out what needs to be done.
>>>> (Note that during the renewal, Dogtag is offline.)
>>>>
>>>> Also, both the attributes should be included for compatibility with external
>>>> CAs. At this point, I think it's only a matter of time when support for them
>>>> will be added (there were already several requests for such a feature), and
>>>> I would very much prefer to have to maintain only a single code path for the
>>>> generic stuff (which includes both of the attributes), instead of one for
>>>> Dogtag and one for external CAs.
>>>>
>>> OK, I'll add issuer DN and subject DN attributes to the ipaCa
>>> objectClass.
>>
>> Just to be clear, what I meant is for the issuer attribute to contain the DN
>> of the CA entry in LDAP, not the issuer DN itself.
>>
> I see; thanks for the clarification.

Actually, the more I think of this, I think the attribute should contain 
the issuer DN rather than the CA entry DN. That way it could be 
mandatory, thus simplifying the related logic, and it should make adding 
external CAs easier in the future.

> I'm going to publish the first
> version of the patchset soon - it will not have this implemented
> yet, but I think it's more important for me to get patches out for
> review ASAP, and add this aspect in a subsequent patchset.

OK, but please make sure it is done before 4.4, adding new mandatory 
attributes is a backward incompatible change, and we most certainly 
don't want to deal with that after the release.

>
>>>>>> """
>>>>>> The Python script shall be installed at /usr/libexec/pki-ipa-retrieve-key
>>>>>> and shall be executed as pkiuser.
>>>>>> """
>>>>>>
>>>>>> Could you please use a subdirectory? Like /usr/libexec/pki (if the script is
>>>>>> going to be distributed with Dogtag) or /usr/libexec/ipa (if the script is
>>>>>> going to be distributed with IPA).
>>>>>>
>>>>> What is the rationale - is it a packaging guideline or just common
>>>>> sense?
>>>>
>>>> I'm not sure if it's an actual guideline, but IMHO it's definitely common
>>>> sense - I don't think littering the global namespace (i.e. /usr/libexec) is
>>>> ever preferable to keeping your stuff in your own namespace.
>>>>
>>> I'll drop the script in a subdir.  While I'm at it, I think I will
>>> move it to the IPA codebase, to improve locality of the Python code.
>>> e.g. if CustodiaClient API or any other IPA Python API changes, the
>>> code in pki repo will be too easily missed.
>>
>> OK, makes sense.
>>
> Latest version of patch 0054 installs the helper program under
> /usr/libexec/ipa.

OK.

>
>>>> Please don't use ipaUniqueId as the RDN unless absolutely necessary. Not
>>>> only it makes debugging harder (because you can't tell which object is which
>>>> just by looking at the DN), it also requires the framework to do an extra
>>>> LDAP search every time the DN needs to be translated to primary key.
>>>>
>>> If cn is used in RDN, will changing cn (which then will be a modrdn
>>> operation) correctly update the references from CA ACLs?
>>
>> Yes, the referint DS plugin takes care of that.
>>
> cn it is; no more ipaUniqueId.

OK.

>
>>>
>>>> "host-authority" does not strike me as something familiar, and the "host"
>>>> bit is kind of confusing, since it is not at all related to IPA hosts. Could
>>>> we use something more obvious ("default", "root", ...)?
>>>>
>>> We shouldn't use "root" because it might not be a root CA.
>>>
>>> We probably shouldn't use "default" because we might later want to
>>> allow different default CAs for different profiles or principal
>>> types.
>>
>> What about "main"?
>>
>>>
>>> Perhaps simply "IPA CA", "ipa", or some variation on that theme?
>>
>> Something like this might be the best, as we currently refer to this CA as
>> "IPA CA".
>>
> I went with 'cn=ipa' and 'description=IPA CA'.

OK.

>
>>>
>>> As discussed above, there will now also be attributes for issuer DN
>>> and subject DN.
>>>
>>>> Why does a Dogtag lightweight CA need to be created before the LDAP entry? I
>>>> assume it's because deleting an LDAP entry is easier than deleting a Dogtag
>>>> lightweight CA in case something goes wrong, in which case I think ipaCaId
>>>> should be required and use a placeholder value in the initial LDAP entry.
>>>>
>>> The IPA object is created first to ensure that the user has the
>>> permissions to do so.  Apart from that it is not important which
>>> happens first.  I can check permissions with can_add() but defer IPA
>>> object creation until after the Dogtag LWCA has been created.  In
>>> fact this is a cleaner approach.
>>
>> Yes, I agree.
>>
> Everything works fine with can_add(); design updated.

OK.

>
>>> ca-del results in deletion of the signing key from Dogtag's NSSDB,
>>> and deletion of the Dogtag lightweight authority object.  On IPA
>>> side it removes the ipaCa object, removes references from CA ACLs,
>>> etc.  ca-del should be a command.
>>
>> OK, but should the certificates issued by the CA stay valid? I would think
>> not - when authorization is based on group membership and the group is
>> deleted, it is no longer possible to authorize - when authorization is based
>> on the CA, I assume it should work analogically and authorization should
>> stop working when the CA is deleted. Or am I missing something?
>>
> It makes sense to revoke the CA certificate when the CA is deleted.
> There is a Dogtag ticket: https://fedorahosted.org/pki/ticket/1638.
>
> After that is implemented, I don't think extra behaviour is needed
> in IPA - but we should clearly document what happens with revocation
> when a CA is deleted.
>
> Until it is implemented, revocation can be performed manually.
>
> (I'll add the preceding commentary to the design page.)

OK.

>
>>>
>>> We can add ca-disable and ca-enable - these behaviours are
>>> implemented in Dogtag already.  I prefer the more fine-grained
>>> control that CA ACLs give you, but I suppose people will want
>>> wholesale enable/disable as well?  Or is it premature to assume so?
>>
>> I think it can be added later. The advantage over CA ACLs is that you don't
>> have to go through potentially many ACLs to enable/disable a single CA.
>>
> Agreed.
>
> Thanks,
> Fraser
>

-- 
Jan Cholasta