Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Add endpoint for automation clients to read secret contents #327

Merged
merged 1 commit into from Apr 14, 2017

Conversation

jbpeirce
Copy link
Contributor

Automation clients can already access the contents of any secret by assigning it to themselves (since they have complete control of group, client, and secret ACLs), reading it through the SecretDeliveryResource, and unassigning it from themselves. This new endpoint directly exposes the ability for automation endpoints to read secrets in one call rather than three, and thus avoids the need to write to Keywhiz (with the assigns and unassigns) in order to read from it. It logs all uses of this endpoint to the audit log.

@jbpeirce jbpeirce force-pushed the jessep/add-automation-content-endpoint branch from a46f87e to 2b0beb1 Compare March 31, 2017 01:12
@@ -0,0 +1,47 @@
package keywhiz.api.automation.v2;
Copy link
Contributor Author

@jbpeirce jbpeirce Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed to me that having separate fields for secrets which were successfully read (and their contents) and secrets which were not successfully read (and any error messages thrown in the reading) would be easier than returning a single map of secrets to "the results of the operation" and making the client distinguish success and failure. However, we can update the API to make it more flexible or easier to use.

At the moment, the only error case is that a secret was not found, so errorSecrets could be a list rather than a map; however, having it as a map leaves flexibility to introduce other error cases later on without changing the API.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 73.553% when pulling 2b0beb1 on jessep/add-automation-content-endpoint into 66df21c on master.

* @responseMessage 404 Secret series not found
*/
@Timed @ExceptionMetered
@POST
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like having this method as a POST; it doesn't alter the server, only retrieves information from it, which seems closer to a GET. However, I wanted this to be a bulk endpoint to allow clients to optimize requests to Keywhiz, and I'm not sure how to encode arbitrarily many secret names into the URL's query parameters (or if that's desirable). One site I found (https://evertpot.com/dropbox-post-api/) mentions a REPORT method for GET-with-request-body situations, but it doesn't seem to be supported by the Javax library we're using for the other annotations. Since the consensus seems to be that a request body shouldn't be added to GET (http://stackoverflow.com/questions/978061/http-get-with-request-body ), it looks like either making this a POST or having a complex URL are the best options if we want to keep this as a bulk endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to get fancy with http verbs. Just make everything a POST.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 73.553% when pulling ca20500 on jessep/add-automation-content-endpoint into 66df21c on master.

@mcpherrinm
Copy link
Contributor

I think we should distinguish missing secrets vs. ones that had some other error retrieving.

Alternatively, include an error code that's not just a string (probably an http status code, so a 404 in that specific case).

Alternatively, if there's any unusual error reading secrets, return an error. Then you'd only have secrets & missing secrets. in the response.

@jbpeirce
Copy link
Contributor Author

jbpeirce commented Apr 3, 2017

Returning an error for any unexpected error retrieving secrets and having only secrets and missing secrets in the response would be simplest with the current Keywhiz structure. Retrieving a secret throws an exception if the secret name passed in was an empty string or if the database transaction throws an exception, and otherwise the only error case is a missing secret.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 73.553% when pulling 224a866 on jessep/add-automation-content-endpoint into 66df21c on master.

@jbpeirce jbpeirce force-pushed the jessep/add-automation-content-endpoint branch from 224a866 to 9a2960e Compare April 10, 2017 20:31
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 73.553% when pulling 9a2960e on jessep/add-automation-content-endpoint into e64a0f6 on master.

@jbpeirce jbpeirce force-pushed the jessep/add-automation-content-endpoint branch from 9a2960e to a313d8d Compare April 10, 2017 21:08
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a313d8d on jessep/add-automation-content-endpoint into ** on master**.

@jbpeirce
Copy link
Contributor Author

The code now lists missing secrets in the response, as well as the map of found secret names to their contents. The endpoint returns an error if anything goes wrong other than a requested secret being missing.

Copy link
Contributor

@mcpherrinm mcpherrinm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now, thank you!

@jbpeirce jbpeirce merged commit accf66b into master Apr 14, 2017
@jbpeirce jbpeirce deleted the jessep/add-automation-content-endpoint branch April 14, 2017 20:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants