[Pki-devel] [PATCH] 0026-5, 0044-3 Lightweight sub-CAs

[Fraser Tweedale]

Thanks for the detailed review Ade!  Comments inline.

On Tue, Aug 25, 2015 at 02:40:11PM -0400, Ade Lee wrote:
> Some initial comments and questions. Will have more after playing with
> it and testing. Looks pretty good so far.
> 
> 1. what is org.apache.tomcat.util.buf.UDecoder.ALLOW_ENCODED_SLASH=true
> and why is it needed?
> 
caRef / handle components are separated by "/".  The Java client
code translates e.g. "sub1/sub2" into the path
"https://.../subca/sub1%3Fsub2", which Tomcat does not like
(immediately rejects).  The above config allows this.

I contemplated changing the separator character - I am still open to
it - but I made this smaller change for now.  From ongoing IRC
discussions there are other situations that might warrant changing
the separator char.

> 2. In CAService.java: issueX509Cert() - if ca is null, this throws an
> ECAException - which, if I understand the code correctly, ends up being
> some kind of BaseException.  So, if someone requests a cert and
> provides a bogus caref, it will be reported as a server error instead
> of a installation error?
> 
Installation error?  Did you mean client/enrolment error?  Anyhow,
I'll have to check the behaviour here, but I thought bogus carefs
were resulting in the appropriate error (404); but I may have missed
some cases.

> 
> 3. CertificateAuthority.java
> -- subCAHier --> rename to subCAHierarchy.
> 
OK.

> -- In the constructor, it would be useful to have example inputs in the
> method documentation so that we can see what data is expected to be in
> each of the fields.  Its hard to review/understand/maintain otherwise.
> 
OK.

> -- you have extracted initCetDatabase() and initCRLDB(), also extract
> the replica repo initialization code into initReplicaRepo() or similar.
> 
Fair enough.

> -- loadSubCAs - could use some description in method comments about the
> structure we expect to construct here.  An example config would help
> understanding.
> 
OK.

> -- createSubCA 
>    -- returns ECAException() in many different cases ie.
>    if the CA exists, if the parent does not exist, if there is an 
>    error in CA creation.  This makes it difficult to separate out the 
>    things that are likely client request issues vs. server issues.  
>    More likely, we want different exceptions that can be caught by the 
>    caller and handled appropriately.
>
No problemo.

>    -- what about uniqueness checks for the issuer DN?
> 
This was on the TODO list.  Now that we have live info on all
sub-CAs it will be straightforward and I will include it in the next
patchset.

> 
> 4. SubCAResource.java 
> -- path should be subcas ? not subca ..
>
OK.

> -- acls needed of course
>
Yep; next patchset.

> 
> 
> 5. CAEnrollProfile.java 
> --> could not reach CA -- that ends up being a ProfileException -- 
>     which maps to a server error?  Is this a case where we need a
>     bad request exception instead?
> 
Yes, user requesting nonexistant CA should be a 404.  Will confirm /
fix for next patchset.

> 6. subca.schema 
> -- why is it specified in a separate file? (and also in schema.ldif)
>
FreeIPA will use this file to update the DS schema.  Similar thing
was done with schema for LDAP-based profiles.  Necessary workaround
until Dogtag has its own schema update mechanism.

> -- the subca is uniquely defined by only one MAY attribute for 
>    nickname (which is in printable string format)?  Is that sufficient
>    and should that be a required attribute?  Do you need to store the
>    issuerDN for uniqueness checks? 
> 
Well, it is uniquely defined ("distinguished" ^_^) by its DN.  I see
no harm in adding issuerDN to the object though; it may turn out to
be useful for other things.

> 7.  SubCAService.java
>  -- lets remove the commented out audit messages and functions
>
They are there to remind me to put audit messages in :)

>  -- createSubCA() 
>      -- should be some checks here on the data -- rather than passing 
>         through to the lower level. Bad data should return 
>         BadRequestException, including for cases where we have 
>         existing issuerDN or caRef.
> 
OK

> 8.  SubCACreateCLI -- this code would be confusing:
> 
>  if (cmdArgs.length < positionalArgNames.length) {
>             System.err.println("Error: No "
>                     + positionalArgNames[cmdArgs.length]
>                     + " specified.");
> 
>  Just specify "Insufficient params .."
> 
OK, thanks.

> 9. It would be good to have a DN check here on client side -- this can be added in a separate patch.
> 
Makes sense.

> 10. Should users be able to define the nickname?  I would say "yes" because it might make it easier to notify services like custodia for instance to distribute keys.
> 
Not sure about that.  Custodia will just replicate any new subCA
keys it finds (by monitoring / polling the directory).

If we choose a sensible name that indicates which CA they key is
associated with, I'm not sure what benefit it would have to allow
users to choose the nickname - in fact, I see it as a potential
source of user confusion / errors.

This is not a "no" - it's certainly doable; just want to make sure
there's a good use case.

> 11. Should we also have the option to define the token in which the key is generated and stored?  ( I think yes - in case for instance, your HSM has limited keys).  Where do the subCA keys get generated by default -- in the software or hardware token?
> 
Currently in softtoken.  Need to think about this; I'll put an item
on TODO list so it doesn't get forgotten.

> 12.  To help clarify this, please describe what would be created
> if one were to add a new subCA using the CLI at reference caRef
> 
> Specifically, 
> a) what database entry is created?
> b) what is the nickname for the key/cert?
> c) Where is the repo (ie. the suffix) for this ca's certs?
> d) Exactly which resources belong to the subCA only?
> 
In brief (I will add detailed commentary in next patchset):

a) An entry is created at ``ou=ca1,ou=subCAs,ou=ca,{rootSuffix}``
   (prepend more ou=foo for nested sub-CAs), with objectClass
   ``subCA``.

b) Nickname is the signing key nickname with the caRef appended,
   except slashes are replaced with ",".

   e.g for subCA "puppet/master" the nickname is
   "caSigningCert cert-pki-ca puppet,master"

c) All certs live in the one repo, and the one repo is used for
    assigning serial numbers.

d) SigningUnit (and associated key in NSSDB), and the sub-CA's
   LDAP entry.  That *should* be all - other resources should
   be shared with top-level CA.

   (Side-note: each subca *temporarily* holds an LDAP conn factory
   and connection during initialisation and when creating a subca,
   but destroys them when done)

> MISSING ITEMS:
> These can be in a separate patch, but if so, we need tickets to track
> them:
> 
> 1. acls
> 2. auditing
> 3. key parameters
> 4. migration scripts (i.e. how to add db entries)
> 5. tests  - we need unit/functional tests to show the data that is 
>    expected and to do basic error checking in the client.  These are
>    very important.  Eventually, we would like tests to be a required
>    part of any check-in.
> 
acls and (probably) audit will be in upcoming revision of this
patch.  I'll file tickets for other bits now.

Thanks!
Fraser

> Ade  
> 
> On Mon, 2015-08-24 at 17:27 +1000, Fraser Tweedale wrote:
> > Hi team,
> > 
> > The latest sub-CAs patches are attached.  It has been a while since
> > the last patchset (that was posted here, anyway) and there have been
> > some significant changes, outlined below.  (The patchset version
> > skipped a couple numbers due to versions distributed privately that
> > I felt were not stable enough to warrant posting to pki-devel.)
> > 
> > Major changes:
> > 
> > - The Java client and CLI were extracted to a separate patch (0044).
> > - An LDAP entry for each sub-CA is written to database.
> >   - Database searched and sub-CAs are initialised at startup
> >   - Key nickname is store in / read from LDAP entry
> > - Sub-CA "list" API call, client method and CLI was added
> > - More resources are shared between top-level CA and sub-CAs
> >   - Suprious task threads and LDAP connections hunted down :)
> > 
> > Dependencies:
> > 
> > - Patch 0026-5 probably depends on 0045[1] for a clean merge.
> > - Patch 0044-3 depends on my patch 0046[2].
> > 
> > [1] 
> > https://www.redhat.com/archives/pki-devel/2015-August/msg00072.html
> > [2] 
> > https://www.redhat.com/archives/pki-devel/2015-August/msg00073.html
> > 
> > Cheers,
> > Fraser