[Freeipa-devel] V4/Sub-CAs review

Tue May 17 11:28:15 UTC 2016

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.

>
>>

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

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

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

"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", ...)?

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)?

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.

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

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

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.

>
> 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 :-)

>
> Cheers,
> Fraser
>

-- 
Jan Cholasta