9:19 p.m.
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