[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



thanks!

pushed to master

commit 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 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 redhat com>
To: pki-devel 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 redhat com>
To: pki-devel 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 redhat com
https://www.redhat.com/mailman/listinfo/pki-devel

_______________________________________________
Pki-devel mailing list
Pki-devel redhat com
https://www.redhat.com/mailman/listinfo/pki-devel


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]