[Pki-devel] [PATCH] 159, 160 - allow for automated generation of shared secrets for TKS/TPS connectors

[Endi Sukma Dewata]

On 9/26/2013 11:12 AM, Ade Lee wrote:
> Patch 159:
>
>   Add service to generate and retrieve a shared secret
>
>      A new REST service has been added to the TKS to manage shared secrets.
>      The shared secret is tied to the TKS-TPS connector, and is created at the
>      end of the TPS configuration.  At this point, the TPS contacts the TKS and
>      requests that the shared secret be generated.  The secret is returned to the
>      TPS, wrapped using the subsystem certificate of the TPS.
>
>      The TPS should then decrypt the shared secret and store it in its certificate
>      database.  This operations requires JSS changes, though, and so will be deferred
>      to a later patch.  For now, though, if the TPS and TKS share the same certdb, then
>      it is sufficient to generate the shared secret.
>
>      Clients and CLI are also provided.  The CLI in particular is used to remove the
>      TPSConnector entries and the shared secret when the TPS is pkidestroyed.

In addition to the things that were discussed over IRC earlier I have 
some comments:

1. The createConnector() probably should return HTTP 201 since it's 
creating a new entry. This would be consistent with other resources.

2. The capitalization of "connector" in the help message is not consistent:

   tks-tpsconnector        TPS Connector management commands
   tks-tpsconnector-add    Add TPS Connector to TKS
   tks-tpsconnector-find   Find TPS connector details on TKS
   tks-tpsconnector-del    Remove TPS connector from TKS

3. The tks-tpsconnector-find output should have a header and footer like 
other *-find commands:

   ----------------------
   1 connector(s) matched
   ----------------------
     Connector ID: 0
     Host: server.example.com
     Port: 8443
     User ID: TPS-server.example.com-8443
     Nickname: TPS-server.example.com-8443 sharedSecret
   ----------------------------
   Number of entries returned 1
   ----------------------------

4. Currently the access to the connector is limited to the subsystem 
user, which doesn't correspond to a real person. It should allow the TKS 
administrator to add/delete the connectors. This can be done by checking 
the principal.getRoles() in validateUser().

5. The tks-tpsconnector-add/del parameters right now are passed as 
arguments. It probably should be passed using options because it may 
change in the future:

   tks-tpsconnector-add --host <host> --port <port>
   tks-tpsconnector-del --connector-id <connector ID>

6. We discussed about changing the error code for non-existent entries 
to 204 (https://fedorahosted.org/pki/ticket/746). However, now I think 
404 actually makes more sense. See 
http://stackoverflow.com/questions/11746894/what-is-the-proper-rest-response-code-for-a-valid-request-but-an-empty-data. 
Both of the following URL's should return the same error code because 
the client shouldn't need to know which path is mapped to REST method:
* /rest/users/wronguser
* /rest/wrongpath

So methods like deleteConnector(), deleteSharedSecret(), etc. should 
throw ResourceNotFoundException instead of returning silently if the 
entry doesn't exist.

I'll continue the review tomorrow.

-- 
Endi S. Dewata