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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to