[Pki-devel] [PATCH] pki-cfu-0066 and pki-cfu-0067 for Ticket 1307 [RFE] Support multiple keySets for different cards for ExternalReg

Fri May 22 00:35:48 UTC 2015

Christina:

Looks like you got the minor issues under control.
We went through a couple of demos and it is tested to work with tpsclient...

ACK

----- Original Message -----
> From: "Christina Fu" <cfu at redhat.com>
> To: pki-devel at redhat.com
> Sent: Thursday, May 21, 2015 4:35:13 PM
> Subject: Re: [Pki-devel] [PATCH] pki-cfu-0066 and pki-cfu-0067 for Ticket 1307 [RFE] Support multiple keySets for
> different cards for ExternalReg
> 
> Thanks for the review Jack.
> Attached please find pki-cfu-0069, which is the revised patch#2.
> 
> Christina
> 
> On 05/20/2015 01:51 PM, John Magne wrote:
> > Looks nice and gives us some good new flexibility with respect to the
> > keySet value.
> >
> >
> > Just a few comments:
> >
> >
> > 1. For each type of "resolver" you have something like:
> >
> > mappingResolver.enrollMappingResolver
> >
> > Previously the whole class name for this was something like
> > "tokenProfileMappingResolver" or some such.
> > This name has been changed to just "mappingResolver". In order to give the
> > user the same info how about
> > something like: "mappingResolver.enollTokenTypeMappingResolver" ??? The
> > same for format and pin reset of course.
> >
> >
> > 2. In MappingResolverManager.java here:
> >
> >    mappingResolvers.put(prInst, resolver);
> >
> >
> > The mappingResolvers property is protected. To make it easier for
> > subclasses to use this,
> > maybe an "addResolver" method for easier use instead of making raw
> > collection calls.
> >
> > 3. In the TKSRemoteRequestHandler.java we have modified a large number of
> > method signatures in order
> > to pass in the newly calculated "keySet" type.
> >
> > Instead of modifying all of the calls inside of TKSRemoteRequestHandler
> > there I suggest this possibility.
> >
> >    1. When we construct the instance of TKSRemoteRequestHandler, we add a
> >    new parameter to the constructor being "keySet",
> > which is invariant per session. Set a private property of the class to it.
> > Also validation can be done in the constructor of this value.
> >
> >     2. IN all the methods where that use the new param, just use the local
> >     property and no need to worry about validation.
> >
> >
> >     3. I see that the TPSEngine methods take the new param as well. It
> >     would be nicer to not have to do that, but I think it might be most
> >     convenient to leave that "keySet" param
> > so it can be used in the constructor to TKSRemoteRequestHandler.
> >
> > 4. In TPSEnrollProcessor I see this block several times in different
> > places.
> >
> > +
> > +            CMS.debug("In TPSEnrollProcessor.enroll isExternalReg: about
> > to process keySet resolver");
> > +            /*
> > +             * Note: externalReg.mappingResolver=none indicates no
> > resolver
> > +             *    plugin used
> > +             */
> > +            try {
> > +            String resolverInstName = getKeySetResolverInstanceName();
> > +
> > +                if (!resolverInstName.equals("none") && (selectedKeySet ==
> > null)) {
> > +                    FilterMappingParams mappingParams =
> > createFilterMappingParams(resolverInstName,
> > +                            appletInfo.getCUIDhexString(),
> > appletInfo.getMSNString(),
> > +                            appletInfo.getMajorVersion(),
> > appletInfo.getMinorVersion());
> > +                    TPSSubsystem subsystem =
> > +                            (TPSSubsystem)
> > CMS.getSubsystem(TPSSubsystem.ID);
> > +                    BaseMappingResolver resolverInst =
> > +
> > subsystem.getMappingResolverManager().getResolverInstance(resolverInstName);
> > +                    String keySet =
> > resolverInst.getResolvedMapping(mappingParams, "keySet");
> > +                    setSelectedKeySet(keySet);
> > +                    CMS.debug(method + " resolved keySet: " + keySet);
> > +                }
> > +            } catch (TPSException e) {
> > +                auditMsg = e.toString();
> > +                tps.tdb.tdbActivity(ActivityDatabase.OP_FORMAT,
> > tokenRecord, session.getIpAddress(), auditMsg,
> > +                        "failure");
> > +
> > +                throw new TPSException(auditMsg, TPSS
> >
> >
> > I think this thing can be made a method of TPSProcessor or something and
> > just called everywhere.
> >
> > 5. This method:
> >
> > protected String getKeySetResolverInstanceName() throws TPSException {
> > +        String method = "TPSProcessor.getKeySetResolverInstanceName: ";
> > +        CMS.debug(method + " begins");
> > +        IConfigStore configStore = CMS.getConfigStore();
> > +        ....
> > ..
> > +
> > +        CMS.debug(method + " config: " + config);
> > +        try {
> > +            resolverInstName = configStore.getString(config, "none");
> > +        } catch (EBaseException e) {
> > +            // not finding it is not an error
> > +        }
> > +        if (resolverInstName.equals(""))
> > +            resolverInstName = "none";
> > +
> > +        CMS.debug(method + " returning: " + resolverInstName);
> > +
> > +        return resolverInstName;
> > +    }
> >
> > We've established that swallowing exceptions is not a good thing to do.
> > Just throw a TPSException because here there is some internal error, since
> > you have
> > already established a default.
> >
> >
> > 6. After doing the above, it might be nice to just try a key changeover
> > operation with tpsclient to make sure
> > everything is kosher after changing the behaviour of the
> > TKSRemoteRequestHandler slightly.
> >
> > thanks,
> > jack
> >
> >
> >
> >
> > ----- Original Message -----
> >> From: "Christina Fu" <cfu at redhat.com>
> >> To: pki-devel at redhat.com
> >> Sent: Monday, May 18, 2015 5:52:20 PM
> >> Subject: [Pki-devel] [PATCH] pki-cfu-0066 and pki-cfu-0067 for Ticket 1307
> >> [RFE] Support multiple keySets for
> >> different cards for ExternalReg
> >>
> >> Please find two patches for the ticket:
> >> https://fedorahosted.org/pki/ticket/1307 [RFE] Support multiple keySets
> >> for different cards for ExternalReg
> >>
> >> Patch pki-cfu-0066 involves only renaming of classes/methods/parameters
> >> and the related config parameters for the Mapping Resolver framework.
> >> (note: after the refactoring, I tested it to work before continuing to
> >> the 2nd part)
> >> It is separated out from the actual code logic changes for ease of review.
> >>     The renaming is necessary as the original framework was intended only
> >> to be used to resolve tokenType, and it is now expanded to be used to
> >> resolve keySet.
> >>
> >> Patch pki-cfu-0067 deals with the actual code changes that adds support
> >> for keySet mapping
> >>
> >> Original design of this add-on ExternalReg feature can be found here:
> >> http://pki.fedoraproject.org/wiki/TPS_-_New_Recovery_Option:_External_Registration_DS#Supporting_multiple_keySets_for_different_cards_for_ExternalReg
> >>
> >> There is no upgrade supported at this point, as this is technology
> >> preview feature.
> >>
> >> Please review.
> >> thanks,
> >> Christina
> >>
> >> _______________________________________________
> >> 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