[Pki-devel] [PATCH] pki-cfu-0066 and pki-cfu-0067 for Ticket 1307 [RFE] Support multiple keySets for different cards for ExternalReg
Christina Fu
cfu at redhat.com
Thu May 21 23:35:13 UTC 2015
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-cfu-0069-Ticket-1307-part2-keySet-mapping-RFE-Support-multipl.patch
Type: text/x-patch
Size: 73028 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20150521/cc63c2e9/attachment.bin>
More information about the Pki-devel
mailing list