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

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(a)redhat.com&gt; > To: pki-devel(a)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(a)redhat.com&gt; >>> To: pki-devel(a)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_Reg... >>> >>> There is no upgrade supported at this point, as this is technology >>> preview feature. >>> >>> Please review. >>> thanks, >>> Christina >>> >>> _______________________________________________ >>> Pki-devel mailing list >>> Pki-devel(a)redhat.com >>> https://www.redhat.com/mailman/listinfo/pki-devel > > _______________________________________________ > Pki-devel mailing list > Pki-devel(a)redhat.com > https://www.redhat.com/mailman/listinfo/pki-devel