[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
Fri May 22 01:01:52 UTC 2015
thanks!
pushed to master
commit fe9e2d9a677317585db34ac5131d17f696c1e09e
<https://fedorahosted.org/pki/changeset/fe9e2d9a677317585db34ac5131d17f696c1e09e/>
Author: Christina Fu <cfu@…> Date: Mon May 18 16:14:47 2015 -0700
Ticket 1307 (part2 keySet mapping) [RFE] Support multiple keySets
for different cards fo
commit 2e6537e80d42c208a96e218d84ed4fb5c6b7a9d4
<https://fedorahosted.org/pki/changeset/2e6537e80d42c208a96e218d84ed4fb5c6b7a9d4/>
Author: Christina Fu <cfu@…> Date: Wed May 13 08:35:34 2015 -0700
Ticket 1307 (part1 refactoring) [RFE] Support multiple keySets for
different cards for E
On 05/21/2015 05:35 PM, John Magne wrote:
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20150521/1698b169/attachment.htm>
More information about the Pki-devel
mailing list