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

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-respo.... 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.