Updated how batch secrets are rendered in JSON #1989

telday · 2021-01-05T16:14:39Z

What does this PR do?

Changed how binary secrets are rendered in batch requests.

Some Details on the Implementation:

Based on feedback in a slack discussion I have moved to a different solution for this issue.

Now on requests to the batch secrets endpoint Conjur will read the Accept header from the request. Currently if the header is base64 it will encode all of the secrets in base64 and return them in the same JSON format we currently use. If the header is set to anything else (or not set at all) the endpoint will act as it has previously, except now returning a 501 with an error message to the user. The up side of this approach is that it will maintain compatibility and is a simple implementation.

Here is an example of a returned JSON object with the Accept: base64 header set:

{
  "cucumber:variable:secret2": "djI=\n",
  "cucumber:variable:secret3": "+6qohmBo1KyHByGIXejpYQ==\n"
}

The original value of secret2 was "v2" and secret3 was binary data.

I also included a conversion of the header value to lowercase to avoid issues between base64 and Base64 because I know some people prefer one over the other.

What ticket does this PR close?

Resolves #1962

Checklists

Change log

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

izgeri

this should definitely be reviewed by @cyberark/conjur-core-team, but I wanted to note that this dependency is fine to add since it's available under MIT. We just need to add its license to the NOTICES.txt file in this project, and should probably do so in this PR: https://github.com/brianmario/yajl-ruby/blob/1.4.1/LICENSE

the PR is also missing a CHANGELOG message explaining the change that was made and the impact to the end user.

izgeri

@telday are there any tests we can add so that we can verify this functionality corrects the reported issue?

I actually find it really helpful in cases like this to add broken test(s) so that I can be sure that my updated functionality addresses the issue (eg the tests go green with my fix).

in this case, I'd guess we could manually create a truststore.p12 file using openssl pkcs12 -in truststore.p12 -nodes | openssl x509 -noout -enddate, add that to the repo test dir, and then add a test that loads that into a variable and attempts to retrieve it via the batch retrieval endpoint / read the output

Gemfile

telday · 2021-01-06T19:26:02Z

@izgeri @orenbm On a second pass looking at this issue I may need a little more guidance in order to fix it. The biggest issue is encoding secrets in JSON, which has no support for a bytes type. Therefore even if you manage to get a library to just treat your binary data as a string (which this library didn't actually perform correctly as I learned). There are going to be encoding errors in almost all libraries on the client side (I encountered serious issues while attempting to write a cucumber scenario and it doesn't look like Python likes it very much either). I think attempting to encode a binary secret as a badly-encoded string is not the best approach.

The second approach I tried to take was to encode the byte data of the secret as an array of integers in the JSON. However this doesn't seem like a great solution, users would most likely be very confused especially if retrieving the secret individually returns just the raw data. This also forces all binary secrets to have their mime type annotation specified.

Have I missed something that would make this significantly easier?

doodlesbykumbi · 2021-01-11T09:38:41Z

@telday I like the idea of the batch retrieval endpoint specifying the mimetype for each secret retrieved (or grouping them by mimetype to reduce duplication). Such an approach would be consistent with the endpoint for single secret retrieval.

Re: encodings, when I first saw this issue I thought BSON might be useful here, see http://bsonspec.org/implementations.html. It's a lot more flexible than JSON. But it might also be overkill. Things like bytes can be represented as arrays (though maybe not the most efficient representation) as long as there's metadata to allow us to recover the original.

telday · 2021-01-11T18:15:58Z

Just wanted to throw out my implementation plan for fixing this. I looked into using BSON for encoding the result, however because it is not a strict superset of JSON I think it would be too much to ask all users to switch to using a BSON library when they are not often built into languages. So what I am going to implement is checking the mime_type annotation for each secret, and if it is the application/octet-stream type I will encode the value in base64 and include extra encoding and mime_type fields in the secrets return object. This will make sure the user understands that their data has been encoded so it is valid for normal JSON format and what encoding type was used. Grouping them together will reduce the amount of duplication as mentioned by @doodlesbykumbi

e.g.

{
  "cucumber:variable:secret2": "v2",
  "application/octet-stream": {
    "cucumber:variable:secret3": "d/U4bDbw3WQzYW5O7AcZzQ==\n",
    "encoding": "base64"
  }
}

It is not a perfect solution, however I think it is the right way to go for two main reasons:

We wont have to introduce any new libraries into Conjur and most languages have JSON and Base64 encoding libraries built in so users wont need anything extra either.
The increase in return object size is smaller than creating a list of bytes (for very large binary values)

This does mean that all binary secrets will need to have the application/octet-stream annotation set, if it is not set and the server cannot properly encode the secret into JSON it will return a 500 error code indicating the bad encoding of the secret.

Let me know what you think/if anything needs to be changed. I coded up most of this as well as added a cucumber.

app/controllers/secrets_controller.rb

codeclimate · 2021-01-11T21:13:23Z

app/controllers/application_controller.rb

@@ -276,6 +280,16 @@ def render_record_not_found e
 }, status: :not_found
 end

+ def bad_secret_encoding e


Similar blocks of code found in 2 locations. Consider refactoring.

app/controllers/application_controller.rb

cucumber/api/features/secrets_batch.feature

doodlesbykumbi · 2021-01-12T16:39:19Z

app/controllers/secrets_controller.rb

+
+ if mime_type == 'application/octet-stream'
+ unless result.key?('application/octet-stream')
+ result['application/octet-stream'] = { 'encoding': 'base64' }


I think base64 encoding is robust enough to address the current issue and potentially moving forward. With that in mind, perhaps we can always and only base64 encode values and always specify the mimeType. This throws out any space savings etc., but I don't think that's altogether necessary and might really just be premature optimisation. The response payload might look something like this.

By avoiding sending raw values we don't ever have to know or reason about what is inside.

{ "id/of/secret": { "base64value": "", "mimeType": "" }, ... }

I like the idea of encoding all secrets in base64, and we could group by mime type in order to reduce duplication in the JSON. It would be more consistent and resolve this issue in a cleaner manner so I would prefer it. The reason I didn't include it in this PR is because it would definitely be a breaking change for users. If we are willing to deal with the change being breaking then I can definitely put it in.

izgeri · 2021-01-12T17:22:49Z

@telday given what you've implemented, and to help people understand the impact of this change, can you please update the PR description to include an example of what the response looks like now vs what it will look like after this change? I am trying to understand whether this is a breaking change, and it's hard to tell from what's written here.

codeclimate · 2021-01-25T14:56:10Z

app/models/exceptions/bad_secret_encoding.rb

+# frozen_string_literal: true
+
+module Exceptions
+ class BadSecretEncoding < EncodingError


Exceptions::BadSecretEncoding has no descriptive comment

jonahx · 2021-01-25T18:45:28Z

app/controllers/secrets_controller.rb

+
+ secret_value = secret.value
+
+ if request.headers['Accept'].casecmp? 'base64'


CodeClimate should be flagging this for missing parens.

codeclimate · 2021-01-25T18:52:45Z

app/models/exceptions/bad_secret_encoding.rb

+# frozen_string_literal: true
+
+module Exceptions
+ class BadSecretEncoding < EncodingError


Exceptions::BadSecretEncoding has no descriptive comment

codeclimate · 2021-01-25T19:22:26Z

app/models/exceptions/bad_secret_encoding.rb

+# frozen_string_literal: true
+
+module Exceptions
+ class BadSecretEncoding < EncodingError


Exceptions::BadSecretEncoding has no descriptive comment

Allows for binary secrets to be encoded nicely in a batch result without causing a server side error

codeclimate · 2021-01-25T20:14:50Z

Code Climate has analyzed commit 58e6535 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 89.2% (0.0% change).

View more on Code Climate.

izgeri

Great work on this @telday!! Thanks for sticking with it!

saprette · 2021-01-26T10:36:12Z

@telday @izgeri @orenbm Hello, thanks for the fix. Will you update the requests of secrets-provider-for-k8s to add the Accept: base64 header ?

telday · 2021-01-26T13:20:43Z

@saprette going open an issue and change the requests this morning

alexkalish · 2021-03-17T15:57:15Z

@telday @doodlesbykumbi @izgeri: This solution cleverly handles backwards compatibility, which is excellent. But is it slightly abusing the Accept header? If the user adds the Accept: base64, wouldn't their client be expecting that the entire response is base64 encoded, not just the secrets values as see in the example above? Looking at the code, seems like we are still returning a JSON body and JSON Content-Type. Is there a chance that this could confuse some HTTP clients that try to parse the body based on the content type?

Also, as a reminder, please be sure to file a documentation ticket when adding/changing product behavior. Thanks!

izgeri · 2021-03-17T21:15:37Z

@alexkalish and I connected on this, and we're not going to include this in the documentation until we find a satisfying answer to the question he raised above ^

Please also note that the behavior was changed in #2065 to use Accept-Encoding instead of Accept

telday · 2021-03-19T16:29:40Z

@alexkalish I agree that it is to some extent an abuse of the Accept-Encoding header. There may be cleaner solutions to this problem (some of them possible backwards incompatible). Perhaps this warrants opening a techdebt issue?

telday requested a review from a team as a code owner January 5, 2021 16:14

telday marked this pull request as draft January 5, 2021 17:28

telday force-pushed the batch-secrets-rendering branch 2 times, most recently from 7d58399 to 34b4c2f Compare January 5, 2021 18:33

telday marked this pull request as ready for review January 5, 2021 19:05

izgeri reviewed Jan 5, 2021

View reviewed changes

telday force-pushed the batch-secrets-rendering branch from 34b4c2f to 2db4231 Compare January 5, 2021 21:06

izgeri suggested changes Jan 5, 2021

View reviewed changes

orenbm reviewed Jan 6, 2021

View reviewed changes

Gemfile Outdated Show resolved Hide resolved

telday marked this pull request as draft January 11, 2021 13:46

telday force-pushed the batch-secrets-rendering branch from 2db4231 to 529f67a Compare January 11, 2021 18:11

telday force-pushed the batch-secrets-rendering branch 2 times, most recently from 176b0fb to c132903 Compare January 11, 2021 18:20

codeclimate bot reviewed Jan 11, 2021

View reviewed changes

telday force-pushed the batch-secrets-rendering branch from c132903 to fcee1d1 Compare January 11, 2021 19:53

codeclimate bot reviewed Jan 11, 2021

View reviewed changes

telday force-pushed the batch-secrets-rendering branch 2 times, most recently from c3ba6c1 to 2b4b8d2 Compare January 11, 2021 20:49

telday marked this pull request as ready for review January 11, 2021 20:49

telday requested review from izgeri and orenbm January 11, 2021 21:09

codeclimate bot reviewed Jan 11, 2021

View reviewed changes

orenbm reviewed Jan 12, 2021

View reviewed changes

app/controllers/application_controller.rb Outdated Show resolved Hide resolved

orenbm reviewed Jan 12, 2021

View reviewed changes

cucumber/api/features/secrets_batch.feature Show resolved Hide resolved

orenbm reviewed Jan 12, 2021

View reviewed changes

cucumber/api/features/secrets_batch.feature Outdated Show resolved Hide resolved

doodlesbykumbi reviewed Jan 12, 2021

View reviewed changes

telday force-pushed the batch-secrets-rendering branch 4 times, most recently from a444bed to 7b755a1 Compare January 25, 2021 14:30

telday requested review from izgeri and jonahx January 25, 2021 14:31

codeclimate bot reviewed Jan 25, 2021

View reviewed changes

telday force-pushed the batch-secrets-rendering branch from 7b755a1 to bb9dff6 Compare January 25, 2021 18:27

jonahx reviewed Jan 25, 2021

View reviewed changes

codeclimate bot reviewed Jan 25, 2021

View reviewed changes

telday force-pushed the batch-secrets-rendering branch from bb9dff6 to 910ff91 Compare January 25, 2021 18:56

codeclimate bot reviewed Jan 25, 2021

View reviewed changes

telday force-pushed the batch-secrets-rendering branch from 910ff91 to 2bcc031 Compare January 25, 2021 19:49

telday added 2 commits January 25, 2021 14:50

Updated how batch secrets are rendered in JSON

7fec1f4

Allows for binary secrets to be encoded nicely in a batch result without causing a server side error

Added cucumber for binary secret in batch requests

58e6535

telday force-pushed the batch-secrets-rendering branch from 2bcc031 to 58e6535 Compare January 25, 2021 19:50

jonahx approved these changes Jan 25, 2021

View reviewed changes

izgeri approved these changes Jan 25, 2021

View reviewed changes

telday merged commit 87f6140 into master Jan 25, 2021

orenbm deleted the batch-secrets-rendering branch January 26, 2021 13:11

This was referenced Jan 26, 2021

Bump Go client version cyberark/secrets-provider-for-k8s#288

Closed

Update how batch secrets are retrieved cyberark/conjur-api-go#88

Closed

izgeri mentioned this pull request May 19, 2021

Change return code on fail to encode batch secrets #2124

Merged

8 tasks

john-odonnell mentioned this pull request Feb 13, 2023

Enable SP to retrieve and provide German umlaut chars cyberark/secrets-provider-for-k8s#500

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated how batch secrets are rendered in JSON #1989

Updated how batch secrets are rendered in JSON #1989

telday commented Jan 5, 2021 •

edited

Loading

izgeri left a comment

izgeri left a comment

telday commented Jan 6, 2021 •

edited

Loading

doodlesbykumbi commented Jan 11, 2021

telday commented Jan 11, 2021 •

edited

Loading

codeclimate bot Jan 11, 2021

doodlesbykumbi Jan 12, 2021 •

edited

Loading

telday Jan 13, 2021

izgeri commented Jan 12, 2021

codeclimate bot Jan 25, 2021

jonahx Jan 25, 2021

codeclimate bot Jan 25, 2021

codeclimate bot Jan 25, 2021

codeclimate bot commented Jan 25, 2021

izgeri left a comment

saprette commented Jan 26, 2021

telday commented Jan 26, 2021

alexkalish commented Mar 17, 2021

izgeri commented Mar 17, 2021

telday commented Mar 19, 2021


		secret_value = secret.value

		if request.headers['Accept'].casecmp? 'base64'

Updated how batch secrets are rendered in JSON #1989

Updated how batch secrets are rendered in JSON #1989

Conversation

telday commented Jan 5, 2021 • edited Loading

What does this PR do?

Some Details on the Implementation:

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

izgeri left a comment

Choose a reason for hiding this comment

izgeri left a comment

Choose a reason for hiding this comment

telday commented Jan 6, 2021 • edited Loading

doodlesbykumbi commented Jan 11, 2021

telday commented Jan 11, 2021 • edited Loading

codeclimate bot Jan 11, 2021

Choose a reason for hiding this comment

doodlesbykumbi Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

telday Jan 13, 2021

Choose a reason for hiding this comment

izgeri commented Jan 12, 2021

codeclimate bot Jan 25, 2021

Choose a reason for hiding this comment

jonahx Jan 25, 2021

Choose a reason for hiding this comment

codeclimate bot Jan 25, 2021

Choose a reason for hiding this comment

codeclimate bot Jan 25, 2021

Choose a reason for hiding this comment

codeclimate bot commented Jan 25, 2021

izgeri left a comment

Choose a reason for hiding this comment

saprette commented Jan 26, 2021

telday commented Jan 26, 2021

alexkalish commented Mar 17, 2021

izgeri commented Mar 17, 2021

telday commented Mar 19, 2021

telday commented Jan 5, 2021 •

edited

Loading

telday commented Jan 6, 2021 •

edited

Loading

telday commented Jan 11, 2021 •

edited

Loading

doodlesbykumbi Jan 12, 2021 •

edited

Loading