[Pki-devel] [PATCH] DRM Transport Key Rotation

[Ade Lee]

On Fri, 2013-09-27 at 16:28 -0700, Andrew Wnuk wrote:
> On 09/27/2013 01:45 PM, Andrew Wnuk wrote:
> > On 09/27/2013 10:11 AM, Ade Lee wrote:
> >> Just a few comments/ questions so I can understand the patch better.
> >>
> >> 1. In CAEnrollProfile, you update the request queue only if the
> >> transport cert is invalid.  Why do we need to do this?  Or why do we not
> >> need to do this in all cases here?
> > DRM rejects archival request when invalid transport keys are used by CA.
> > CA marks corresponding enrollment request as rejected providing clear 
> > error message at the end of approval process.
> >>
> >> 2. In EnrollProfile.java, you get the transport cert from
> >> ca.connector.KRA.transportCert.  Is it possible to have more than one CA
> >> connected?  Is that parameter always the correct one to use?
> > Multiple CAs can archive keys using single DRM, but one CA cannot 
> > archive keys using multiple DRMs.
> >>
> >> 3. In EnrollmentService.java, you read the transport cert attribute in
> >> the request, and throw an exception of it is not present (basically
> >> tcert == null).  This will presumably occur if you receive an escrow
> >> request from an older CA, right?  How are we handling this case?
> > Transport certificate verification returns null only for invalid 
> > transport certificate.
> > DRM will use current transport key In case of CA not providing 
> > transport certificate through the connector.
> > I just noticed that attached patch is not the final one. I'll resend 
> > it shortly including (4).
> >> 4.  Incidentally,
> >> transportCert != null && transportCert.length() > 0
> >> can be replaced with ! StringUtils.isEmpty(transportCert)
> >> Same thing in a couple other places.
> I see 3 occurrences in 2 files. None of the two file has dependency on 
> apache packages at this moment.
> Use of StringUtils requires dependency on apache package, so I see no 
> reason to add unnecessary dependency on the foreign package just to save 
> 16 characters

This code is in pki-server which already depends on the apache package
in other locations.  If you do a search for StringUtils, you will see it
in a number of places in the newer code.  

There is value in using some of the newer functions here for type and
null safety.


> > I did not see any documentation specifying such requirement.
> >>
> >> 5. Why do you return true in KRAService.java (serviceRequest) instead of
> >> false?
> > This way CA has a chance to return nice message reporting use of 
> > invalid transport certificate to agent approving enrollment request.
> >>
> >> Ade
> >>
> >>
> >> On Wed, 2013-09-25 at 16:59 -0700, Andrew Wnuk wrote:
> >>>       This patch provides basic support for DRM transport key rotation
> >>> described
> >>>       in http://pki.fedoraproject.org/wiki/DRM_Transport_Key_Rotation
> >>>
> >>>       This patch provides implementation for tickets:
> >>>        - 729 - CA to include transport certificate when submitting
> >>> archival request to DRM
> >>>        - 730 - DRM to detect presence of transport certificate 
> >>> attribute
> >>> in submitted archival
> >>>                request and validate transport certificate against DRM's
> >>> transport key list
> >>>        - 731 - DRM to provide handling for alternative transport key
> >>> based on detected
> >>>                and validated transport certificate arriving as a 
> >>> part of
> >>> extended archival request
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> Pki-devel mailing list
> >>> Pki-devel at redhat.com
> >>> https://www.redhat.com/mailman/listinfo/pki-devel
> >>
> >
> > _______________________________________________
> > Pki-devel mailing list
> > Pki-devel at redhat.com
> > https://www.redhat.com/mailman/listinfo/pki-devel
> 
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel