Batch hmac - (#5850) #5875

martinwaite · 2018-11-29T19:31:09Z

Guidance is needed on some points:

batch_input added to pathHMACwrite. Should I extend this to pathHMACverify ? would that extend to path_sign_verify ?
There are several calls to p.Unlock() before returns: is there a reason not to use defer p.Unlock() ?
Test is shallow - returned values should be checked - perhaps using verify ?
batch_input only supports "input" parameters. Should I include other parameters ?

Most of the changes are copied from batch_input in path_encrypt. I was surprised that the framework.Path structure did not mention batch_input. Why is it not there ?

martinwaite · 2018-11-30T06:28:34Z

The transit/path_config_test is failing because path_hmac now returns an error if request "input" field is missing:
return logical.ErrorResponse("missing input for HMAC"), logical.ErrInvalidRequest

(This mimics the behaviour of transit/encrypt when no plaintext is supplied.)

Unfortunately, path_config_test reuses a transit/encrypt request when first calling pathHMAC - and so this carries a plaintext field but not an input field - and in this case returns an error instead of the HMAC of blank.

I can either revert path_hmac to its old behaviour - quietly returning the hmac of an empty input if input is not supplied - or I can keep the new, arguably more correct behaviour of rejecting requests with no input (or batch_input) fields and fix the path_config_test so that the input field is present.

What should I do ?

martinwaite · 2018-11-30T08:38:57Z

For the time being, I have reverted to the existing behaviour of accepting requests without an input field and returning the HMAC of a blank field.

I guess there would be no harm in fixing up the path_config_test case by copying the plaintext field into the input field for the HMAC tests.

In the batch_response, hmac fields are uppercase HMAC rather than lowercase for non-batch. I will fix that later. I was expecting it to be converted to lowercase given the definition:

 HMAC string `json:"hmac,omitempty" structs:"hmac" mapstructure:"hmac"`

I expected the json markup to be honoured in generating the message from the batch_response.

martinwaite · 2018-12-02T12:15:15Z

I have fixed up the transit/path_config_test so that input is supplied as well as plaintext. This allows the test to pass with stricter parameter checking in-place.

I have reverted transit/path_hmac pathHMACWrite to return an error if no input is supplied. When batch_input is supplied, any item in the data array without an input field will receive an error message in the response. For example:

{
  "batch_input": [
    {
      "input": "adba32=="
    },
    {
      "input": "adba32=="
    },
    {},
    {
      "input": ""
    }
  ]
}

Now returns:

{
  "request_id": "2d15cdc3-c26c-8259-b0c4-7251371dbaf8",
  "lease_id": "",
  "renewable": false,
  "lease_duration": 0,
  "data": {
    "batch_results": [
      {
        "hmac": "vault:v1:1jFhRYWHiddSKgEFyVRpX8ieX7UU+748NBwHKecXE3hnGBoAxrfgoD5U0yAvji7b5X6V1fP
      },
      {
        "hmac": "vault:v1:1jFhRYWHiddSKgEFyVRpX8ieX7UU+748NBwHKecXE3hnGBoAxrfgoD5U0yAvji7b5X6V1fP
      },
      {
        "error": "missing input for HMAC"
      },
      {
        "hmac": "vault:v1:/wsSP6iQ9ECO9RRkefKLXey9sDntzSjoiW0vBrWfUsYB0ISroyC6plUt/jN7gcOv9O+Ecow
      }
    ]
  },
  "wrap_info": null,
  "warnings": null,
  "auth": null
}

briankassouf · 2019-01-09T01:16:53Z

Hi @martinwaite, thanks for contributing this change! We'd like to get this merged in soon, so we were wondering if you could add API docs for the new API?

martinwaite · 2019-01-09T19:40:13Z

Hi @briankassouf - I have added some api docs.

builtin/logical/transit/path_hmac.go

briankassouf · 2019-01-14T22:00:06Z

@martinwaite After thinking about it some more it would be really nice to also offer a batch verify and a batch sign API at the same time as this change. Is that something you'd be willing to add to this PR? I imagine the changeset for verify/sign would look very similar to what you did for hmac.

hashicorp-cla · 2019-01-15T17:33:40Z

All committers have signed the CLA.

martinwaite · 2019-01-16T05:49:51Z

@briankassouf - I will extend the PR to cover sign and verify. I should get it done over the weekend.

martinwaite · 2019-01-20T17:29:16Z

@briankassouf - I have added a first draft of the code for supporting batch_input for sign and verify. I still need to test the HMAC verification path, and I expect to have broken the acceptance tests. I will complete this next weekend.

martinwaite · 2019-01-21T06:14:38Z

Refined the error-handling so that when batch mode simulates simple 'input' case, errors are reported in same way as before batch was added. This is working for the Sign path, but needs to be extended to the verify and hmac cases.

martinwaite · 2019-01-27T18:04:59Z

@briankassouf - ~~This is ready for review now.~~ I need to rebase.

resync

martinwaite · 2019-01-28T23:10:10Z

@briankassouf - I need help with the failing tests. 5 of the tests are something to do with the documentation. The logs show a ruby stack trace - I have no idea how to deal with that. One of the CI builds tripped over a JWT issue - I can't tell if that is something I did.

website fix

martinwaite · 2019-01-31T05:52:16Z

@briankassouf - all tests are passing now. This is ready for review.

builtin/logical/transit/path_hmac.go

builtin/logical/transit/path_sign_verify.go

builtin/logical/transit/path_sign_verify_test.go

kalafut · 2019-03-01T21:55:10Z

website/source/api/secret/transit/index.html.md

@@ -747,13 +747,26 @@ be used.

 - `input` `(string: <required>)` – Specifies the **base64 encoded** input data.


This is no longer a required parameter.

I have changed "<required>" to nil - and added a sentence about either input or batch_input being needed. I'm not sure about "nil" being correct .

kalafut · 2019-03-01T21:57:17Z

website/source/api/secret/transit/index.html.md

@@ -806,6 +870,28 @@ supports signing.

 - `input` `(string: <required>)` – Specifies the **base64 encoded** input data.


This is no longer a required parameter.

I have changed "<required>" to nil - and added a sentence about either input or batch_input being needed. I'm not sure about "nil" being correct .

kalafut · 2019-03-01T22:10:06Z

@martinwaite This looks really good! I left a few minor comments but overall I think it's close.

@briankassouf Any thoughts on having a limit (possible pretty high) on the max batch size? I'm wondering if having no limit might lure users into dumping all items into a batch, and then as their batch size grows they might start hitting request timeout errors, or Vault has issues, etc. Enforcing some level of batching on the caller's side now might avoid those issues.

jefferai · 2019-03-02T16:22:14Z

@kalafut we already have a per-listener maximum request size (with a default of about 32 MB). I guess the question would be, in a normal request timeout period, what does this translate to in terms of max batched operations. But that request timeout is also driven by the client, so if the client is hitting errors it can change it...

martinwaite · 2019-03-02T19:33:42Z

@kalafut - I have implemented all of your code suggestions. I'm not sure about the changes I made to the website doc - please check.

Ugh - build fails. I'll see if rebase fixes it.

Lots of build errors concerning caching - for example TestLeaseCache_SendCacheable. I don't think it is anything to do with my changes.

Maybe I should have merged rather than rebased: your review comments no longer refer to valid commits. Sorry.

resync

kalafut · 2019-03-04T18:14:05Z

builtin/logical/transit/path_sign_verify_test.go

 			}
-			return ""
+			t.Fatalf("bad: expected response, got: %#v", *resp)


Suggested change

t.Fatalf("bad: expected response, got: %#v", *resp)

t.Fatalf("bad: expected error response, got: %#v", *resp)

kalafut · 2019-03-04T18:16:18Z

website/source/api/secret/transit/index.html.md

@@ -745,15 +745,29 @@ be used.
    - `sha2-384`
    - `sha2-512`

- `input` `(string: <required>)` – Specifies the **base64 encoded** input data.
+- `input` `(string: nil)` – Specifies the **base64 encoded** input data. One of 


Suggested change

- `input` `(string: nil)` – Specifies the **base64 encoded** input data. One of

- `input` `(string: "")` – Specifies the **base64 encoded** input data. One of

kalafut · 2019-03-04T18:16:33Z

website/source/api/secret/transit/index.html.md

@@ -805,7 +870,30 @@ supports signing.
    - `sha2-384`
    - `sha2-512`

- `input` `(string: <required>)` – Specifies the **base64 encoded** input data.
+- `input` `(string: nil)` – Specifies the **base64 encoded** input data. One of 


Suggested change

- `input` `(string: nil)` – Specifies the **base64 encoded** input data. One of

- `input` `(string: "")` – Specifies the **base64 encoded** input data. One of

kalafut · 2019-03-04T18:22:56Z

website/source/api/secret/transit/index.html.md

@@ -894,6 +1023,31 @@ data.
  `/transit/hmac` function. Either this must be supplied or `signature` must be
  supplied.

+- `batch_input` `(array<object>: nil)` – Specifies a list of items for processing.


(GH won't let me add this comment outside the diff range). Please update the input docs here as you did the others.

jefferai · 2019-03-04T20:09:39Z

builtin/logical/transit/path_hmac.go

@@ -116,70 +136,105 @@ func (b *backend) pathHMACWrite(ctx context.Context, req *logical.Request, d *fr
 		return nil, fmt.Errorf("HMAC key value could not be computed")
 	}

-	var hf hash.Hash
+	var hashAlg func() hash.Hash


This should probably use the keysutil.HashTypeMap stuff we are using elsewhere in transit

jefferai · 2019-03-04T20:11:06Z

builtin/logical/transit/path_hmac.go

-	}
-
-	var hf hash.Hash
+	var hashAlg func() hash.Hash


Same comment

jefferai · 2019-03-04T20:13:05Z

builtin/logical/transit/path_sign_verify.go

+type batchResponseSignItem struct {
+	// signature for the input present in the corresponding batch
+	// request item
+	Signature string `json:"signature,omitempty" structs:"signature" mapstructure:"signature"`


Here and elsewhere, if you are not using the structs package in your tests for this functionality, please don't decorate it. We are generally trying to get rid of usage of that package.

kalafut · 2019-03-04T20:25:26Z

@martinwaite Thanks for addressing the comments and the contribution as a whole. There a few additional comments that have been left, but at this point I'm going to merge your PR and make those changes.

martinwaite changed the title ~~Batch hmac~~ Batch hmac - (#5850) Nov 29, 2018

briankassouf added this to the 1.0.1 milestone Nov 30, 2018

jefferai modified the milestones: 1.0.1, 1.1, 1.0.2 Dec 12, 2018

briankassouf reviewed Jan 10, 2019

View reviewed changes

builtin/logical/transit/path_hmac.go Outdated Show resolved Hide resolved

jefferai modified the milestones: 1.0.2, 1.0.3 Jan 14, 2019

Merge pull request #1 from hashicorp/master

9b978d8

resync

martinwaite force-pushed the batch_hmac branch from a409b8d to c7d84b4 Compare January 28, 2019 06:09

Merge pull request #2 from hashicorp/master

fbaf1b5

website fix

martinwaite force-pushed the batch_hmac branch from 99c94b8 to 1d645bf Compare January 30, 2019 22:18

jefferai modified the milestones: 1.0.3, 1.1 Feb 1, 2019

kalafut self-requested a review March 1, 2019 19:11