[Freeipa-devel] V4/Sub-CAs review

[Jan Cholasta]

On 17.5.2016 14:50, Fraser Tweedale wrote:
> On Tue, May 17, 2016 at 01:28:15PM +0200, Jan Cholasta wrote:
>> On 10.5.2016 12:36, Fraser Tweedale wrote:
>>> Honza, thanks for the review.  Comments inline.
>>>
>>> Copy Nalin, re certmonger discussion at the very bottom.
>>>
>>> On Mon, May 09, 2016 at 08:54:32AM +0200, Jan Cholasta wrote:
>>>> Hi,
>>>>
>>
>> ----8<------
>>
>>>> 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.

>
>>>
>>>>
>>
>> ----8<------
>>
>>>> 4) <http://www.freeipa.org/page/V4/Sub-CAs#Key_replication>
>>>>
>>>> """
>>>> For FreeIPA, Dogtag will provide the IPACustodiaKeyRetriever class, which
>>>> implements the KeyRetriever interface. It invokes a Python script that
>>>> performs the retrieval, reusing existing FreeIPA Custodia client code.
>>>> """
>>>>
>>>> Will this be distributed with Dogtag or with IPA?
>>>>
>>> It's shipped in Dogtag.
>>>
>>>> The Python script bit sounds like an implementation detail rather than an
>>>> actual design. Ideally the whole thing would be done in Java, right?
>>>>
>>> I removed the script from the design page (it had changed, though
>>> not dramatically).  It is written in Python so that we can reuse the
>>> CustodiaClient.
>>
>> I figured as much. My point is that whether you are executing an external
>> binary or using native code is an implementation detail, so it should not be
>> in the "Design" section, but rather the "Implementation" section.
>>
> Fair point; I'll move what remains the Implementation section.
>
>>>
>>>> """
>>>> 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.

>
>>>
>>>> """
>>>> pkiuser does not have read access to either of these locations, so a new
>>>> service principal shall be created for each Dogtag CA instance for the
>>>> purpose of authenticating to Custodia and retrieving lightweight CA private
>>>> keys. Its principal name shall be dogtag-ipa-custodia/<hostname>@REALM. Its
>>>> keytab and Custodia keys shall be stored with ownership pkiuser:pkiuser and
>>>> mode 0600 at /etc/pki/pki-tomcat/dogtag-ipa-custodia.keytab and
>>>> /etc/pki/pki-tomcat/dogtag-ipa-custodia.keys respectively.
>>>> """
>>>>
>>>> Don't forget to update this paragraph with the new principal name.
>>>>
>>> Thanks, I updated it.
>>>
>>>>
>>>> 5) <http://www.freeipa.org/page/V4/Sub-CAs#Installation>
>>>>
>>>> """
>>>> A CA object for the top-level CA will initially be created, with DN
>>>> cn=.,ou=cas,cn=ca,$SUFFIX.
>>>> """
>>>>
>>>> I would rather not use punctuation for the short name, as it can be easily
>>>> overlooked (think logs). (Also it should be '/' if anything, not '.', which
>>>> usually means "current".)
>>>>
>>>> Above you stated that the subject name will be derived from the short name
>>>> of the sub-CA. The top-level CA has subject name of the form "CN=Certificate
>>>> Authority,$SUBJECT_BASE", so its short name should be "Certificate
>>>> Authority".
>>>>
>>>
>>> This section was also part of the old design.  The entry for the
>>> host authority has ``ipaUniqueId=...`` as rightmost RDN, like other
>>> CAs.  The cn is ``host-authority``.  Subject DN is no longer derived
>>> from cn.
>>
>> 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.

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

>
>> In the current schema, there are 3 different key attributes (cn,
>> ipaUniqueId, ipaCaId). Can we reduce the number? I don't see anything that
>> would mandate more than 2 (cn, ipaCaId). Would it be possible to have only 1
>> (cn)?
>>
> If we go with cn in RDN then ipaUniqueId can go away.  ipaCaId is
> needed (stores Dogtag authority ID, which is a UUID and not
> user-controlled).

I see, 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.

>
>>>
>>>>
>>>> 6) <http://www.freeipa.org/page/V4/Sub-CAs#ipa_cert-find_.5Bshortname.5D>
>>>>
>>>> """
>>>> ipa ca-del <shortname>
>>>>
>>>> Delete the given certificate authority. This will remove knowledge of the CA
>>>> from the FreeIPA directory but will not delete the sub-CA from Dogtag.
>>>> Dogtag will still know about the CA and the certificates it issued, be able
>>>> to act at a CRL / OCSP authority for it, etc.
>>>> """
>>>>
>>>> What is the use case for this? Will the certificates issued by the sub-CA
>>>> still be valid after delete or not? Will the sub-CA certificate be
>>>> explicitly distrusted on delete or not?
>>>>
>>>> IMO it should be possible to delete only a leaf sub-CA, i.e. anything but
>>>> the top-level CA in the current design.
>>
>> Judging from the updated design, it seems to me that the only thing ca-del
>> does is that it prevents further certificate requests to the CA. If that's
>> really the case, I think it should be replaced with a "ca-disable" command,
>> since every other -del command makes the deleted object invalid for all
>> uses.
>>
> 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?

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

>
>>>>
>>>> """
>>>> ipa cert-find [shortname]
>>>>
>>>> shortname
>>>>      Optional positional parameter to specify a sub-CA to use (omit to
>>>> specify the top-level CA). The special shortname * is used to search in all
>>>> CAs.
>>>> """
>>>>
>>>> This should be "ipa cert-find [--ca=<shortname>]". At some point, cert-find
>>>> should be fixed to be consistent with every other -find command and have an
>>>> optional 'criteria' positional argument, and there can't be two optional
>>>> positional arguments, as it creates an ambiguity.
>>>>
>>>> I would prefer a separate argument (e.g. --all-cas, or --cacat=all) rather
>>>> than a magic value for an all-CA search. Magic value might work for
>>>> cert-find alone, but you are creating a precedent for the whole framework
>>>> here.
>>>>
>>> Design updated.  No position arg anymore.  No special arg for "all
>>> CAs" - it is implied unless one of ``--issuer=DN`` or ``--ca=NAME``
>>> is given.
>>>
>>>>
>>>> """
>>>> ipa cert-show [shortname]
>>>>
>>>> shortname
>>>>      Optional positional parameter to specify a sub-CA (omit to specify the
>>>> top-level CA).
>>>> --chain
>>>>      Request the certificate chain (when saving via --out <file>, PEM format
>>>> is used; this is the format uesd for the end-entity certificate).
>>>> """
>>>>
>>>> This should be "ipa cert-show [--ca=<shortname>]", for consistency with
>>>> cert-find, see above.
>>>>
>>> Actually, we do not (at the moment) need a CA argument, because
>>> serial numbers are unique in the whole Dogtag instance.  I have
>>> removed the argument but if it makes a comeback in future, it will
>>> be in the form you recommend.
>>
>> This is not right, for a number of reasons.
>>
>> First of all, it violates our object model. It may not be very apparent, as
>> the cert plugin is currently implemented as a bunch of loosely related
>> ad-hoc commands rather than an object with methods, but nonetheless, all of
>> the commands which implement CRUD operations on certificates
>> (cert-{request,status,show,revoke,remove-hold}) should have the same CA
>> argument with the same default value, or no CA argument at all.
>>
>> Second, the design should not revolve around Dogtag implementation details,
>> because implementation details, as well as the circumstances in which the
>> feature is used, may change. In particular, this design would not work well
>> with the aforementioned external CA support, because instead of using
>> issuer+serial as unique identifier of certificates, it uses just serial and
>> assumes it is unique among all issuers, which is generally not true.
>>
> Your reasoning is irrefutable :)  There shall be a CA argument,
> defaulting to the top-level CA.
>
>> Last, -show commands are supposed to implement a retrieve operation, but
>> what you are proposing is effectively a search for all certificates with a
>> given serial number, which actually belongs to cert-find.
>>
>>>
>>>> IMO it would make sense to add --chain to cert-request as well, it should be
>>>> useful for certmonger.
>>>>
>>>>
>>>> 7) <http://www.freeipa.org/page/V4/Sub-CAs#Certmonger>
>>>>
>>>> How is a certificate going to be requested from a specific sub-CA using the
>>>> getcert command?
>>>>
>>> I added a preliminary design; add a new certmonger property and
>>> corresponding getcert-request(1) option for specifying the target
>>> CA.
>>>
>>> http://www.freeipa.org/page/V4/Sub-CAs#Indicating_the_target_CA
>>
>> LGTM.
>>
> Cool, thanks!
>
>>>
>>> Alternative approaches involve overloading an existing property
>>> (e.g. profile) to optionally carry both profile and CA.  The
>>> overloading could be handled in Certmonger or in FreeIPA.  Either
>>> way is feasible - both are nasty hacks! - but it is worth mentioning
>>> the alternatives.
>>
>> I don't think the solution proposed in the design page is a hack.
>> Overloading an existing property definitely is, so thanks for mentioning it,
>> but please don't do it :-)
>>
> Ah, "both" meant the two overloading options - overloading in
> Certmonger or in IPA.  I am encouraged that you do not consider the
> actual proposal a hack.  But the implementation is yet to come ;)
>
> Honza, thank you for your ongoing input.  I will update the design
> page tomorrow.

And thank you for bearing with me and my slow response times :-)

>
> Cheers,
> Fraser
>


-- 
Jan Cholasta