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

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? 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? 3. CertificateAuthority.java -- subCAHier --> rename to subCAHierarchy. -- 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. -- you have extracted initCetDatabase() and initCRLDB(), also extract the replica repo initialization code into initReplicaRepo() or similar. -- loadSubCAs - could use some description in method comments about the structure we expect to construct here. An example config would help understanding. -- 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. -- what about uniqueness checks for the issuer DN? 4. SubCAResource.java -- path should be subcas ? not subca .. -- acls needed of course 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? 6. subca.schema -- why is it specified in a separate file? (and also in schema.ldif) -- 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? 7. SubCAService.java -- lets remove the commented out audit messages and functions -- 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. 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 .." 9. It would be good to have a DN check here on client side -- this can be added in a separate patch. 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. 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? 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? 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 .." 9. It would be good to have a DN check here on client side -- this can be added in a separate patch. 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. 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? 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? 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. Ade On Mon, 2015-08-24 at 17:27 +1000, Fraser Tweedale wrote: