Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reversion or Enhancement of Secret Display Post-PR #24290 for Improved Security and Usability #24839

Closed
chichi13 opened this issue Jan 12, 2024 · 9 comments · Fixed by #25235
Closed
Labels
reproduced This issue has been reproduced by a Vault engineer ui

Comments

@chichi13
Copy link

Is your feature request related to a problem? Please describe.

It's related to PR #24290 or improvement is regression for me. The default JSON display is a regression in terms of security (all secrets are now visible, whereas before we could select only the secret we wanted). Now it's not even possible to see our secrets like before (< 1.15.4).

Describe the solution you'd like

Enable a global parameter that allows or disallows default display in JSON or not.

Describe alternatives you've considered

At the very least, allow us to have the old presentation when there's JSON in our secrets.

Additional context

For me, the change made in the PR is a huge step backwards in terms of safety and practicality. We need at least one solution that allows 2 views: JSON or no. Even if it means having a global parameter in the account that enables or disables the JSON view when there's JSON in the secret.

@Monkeychip
Copy link
Contributor

@chichi13 thank you for the issue. To clarify, we refactored a lot of the KV engine in 1.15.x, and the PR you're referencing was a PR to bring back a functionality we missed in that refactor. So prior to 1.15.x whenever you had a complex secret it would default to the JSON editor view like it does now in 1.15.4 +.

On your point:

(all secrets are now visible, whereas before we could select only the secret we wanted).

PR #24530 in 1.16.x will now mask the secrets in the json view. This is an improvement as we have never done this in KV before.

One question, I'm a little confused by this statement:

Now it’s not even possible to see our secrets like before (< 1.15.4).

Are you referring to us disabling the json editor toggle button? We do this because you cannot see a nested secret in a normal input field. This was the same behavior in 1.14.x. Here's a screenshot of 1.14.x. The only time where we have not done this is from 1.15.0 - 1.15.3, which was a miss on our part. If you have a complex secret like below it's not possible to view it in our regular key value form fields.

image

Let me know if I've misunderstood, or you have any other questions.

@chichi13
Copy link
Author

@Monkeychip This is strange, we've always had complex secrets (nested JSON) and we've always had the "default" display with the key visible and the value hidden, without being displayed in JSON, like this:

image

PR #24530 in 1.16.x will now mask the secrets in the json view. This is an improvement as we have never done this in KV before.

Unfortunately, this improvement, although it will hide the secrets, remains a problem in our case where we have complex keys (nested JSON) mixed with "normal" keys (without JSON). We only use Hashicorp Vault via the interface, so copying a JSON or non-JSON value has become very complicated.

In the following example, how can I simply copy my key "my_json_key" (as before, by clicking on the "copy" button on the key) without having the "\n" via the interface? (but keeping the correct formatting)

image

Are you referring to us disabling the json editor toggle button? We do this because you cannot see a nested secret in a normal input field. This was the same behavior in 1.14.x. Here's a screenshot of 1.14.x. The only time where we have not done this is from 1.15.0 - 1.15.3, which was a miss on our part. If you have a complex secret like below it's not possible to view it in our regular key value form fields.

That's what I find strange, we've been using Hashicorp Vault for a year and a half now in the same way, with the same style of KV with nested JSON and we've never had the JSON presentation like you mention. We've always had the choice between JSON or not. So we always left without the JSON view in order to easily copy values, but also not show the key value on our screen.

@Monkeychip
Copy link
Contributor

@chichi13 Thank you for the extra information. I confirmed that the copy paste with the line breaks has been fixed as of 1.15.3 by PR #24224. And I tested the following secret in both 1.14.x and the latest 1.15.x and they have the same behavior; both displaying JSON with the JSON toggle disabled.

{
  "foo": {
    "bar": "baz"
  },
  "hello": "world"
}

That being said, I do want to figure out how you might have had the ability to show a complex secret in a non-json view, just to make sure I'm not missing a use case. I realize since you've upgraded it might not be possible to provide a screen shot of such a case, but if you do that would be helpful!

@chichi13
Copy link
Author

chichi13 commented Jan 22, 2024

@Monkeychip I can confirm that for a year now (since we've been using Hashicorp Vault) we've been able to copy complex JSON secrets directly via the interface without having the JSON version displayed (we're 3 to confirm this behaviour). Like this:

image

This was very useful, so I think it would be nice to be able to choose between JSON or not via the toggle. And if possible, to have a profile parameter that allows you to force the default view. That would be a big plus!

@chichi13
Copy link
Author

Hi @Monkeychip , have you had a chance to have a look? I can see that I'm not the only one following the various reactions (emoji) from other users

@Monkeychip
Copy link
Contributor

@chichi13 thank you for following up. For your screenshot example what does the json format of those key-values look like? I want to make sure I'm reproducing correctly.

E.g. is the screenshot you showing something like #1 or #2 in json format?

# example 1
{
  "password_1": {
    "foo": "bar"
  },
  "password_2": "secret"
}

# example 2
{
  "password_1": "secret-1",
  "password_2": "secret-2"
}

@chichi13
Copy link
Author

@Monkeychip here is a real JSON example (with wrong values):

{
	"address": "random address", 
	"example1": {
		"type": "name/AleXander",
		"value": "SSdtIGVuY29kZWQgaW4gYmFzZTY0Cg=="
	},
	"example2": {
		"type": "name/DiDier",
		"value": "SSBhbSBlbmNvZGVkIGluIGJhc2U2NCwgcGFydDIK"
	}
}

Here is the result in v1.15.4, which was not the case before this upgrade:

image

@Monkeychip
Copy link
Contributor

Monkeychip commented Jan 31, 2024

@chichi13

Edit: Are you using versions of your secret? And is one of those versions a simple key/value without nested values while another version is a nested secret? On 1.15.5, if I create a non-complex secret and then create a new version of that secret and make it complex I see the key/value view that you're seeing!

Regardless if the above scenario is your workflow, I think that the best solution is to not disable the JSON toggle—what you were originally asking for. The reason we forced the JSON view was to match previous Vault behavior (or what we thought we were matching). Apologies for it taking a long time for me to get there, but I agree forcing the view of JSON does not make sense. I'll push for trying to revert that change. Stay tuned.

using your example json, I went back to release 1.13.x and this is the details view. It too is a json only view.
image

And here is 1.15.5 (not yet released).
image

One thought is that what I'm calling a nested secret and what your calling a nested secret might be a little different. I was just made aware of a bug fix to the KV json view in 1.15.5 that I could solve your issue? #PR 24513 - when a key value string had a { in it we were accidentally forcing the JSON view, this was fixed here. This is a string, not a nested secret, but potentially that was your use case?

And finally, since I can't seem to replicate (aside from the regression between 1.15.0 -1.15.3) what you were seeing, I do understand the request and I can propose to our team that we figure out a way to make something like that work for the 1.17.0 feature release (1.16.0 is coming out too soon and we couldn't make the deadline).

@chichi13
Copy link
Author

chichi13 commented Feb 1, 2024

@Monkeychip That could indeed explain the problem. However, I can't confirm that my first version was without nested JSON because the display doesn't change if I select version 1 or version X.
I have noticed this:

  • If I have a nested JSON, the display of version 1 or version X does not change
  • If I don't have a nested JSON ("normal" display), version 1 is different from version X.

So there seems to be a problem with the display of JSON versions.

Regardless if the above scenario is your workflow, I think that the best solution is to not disable the JSON toggle—what you were originally asking for. The reason we forced the JSON view was to match previous Vault behavior (or what we thought we were matching). Apologies for it taking a long time for me to get there, but I agree forcing the view of JSON does not make sense. I'll push for trying to revert that change. Stay tuned.

Not disabling the JSON button would be perfect for us. If we could also define the default display of complex JSON in our settings, that would be perfect! No worries about the time it took :)

@Monkeychip Monkeychip added the reproduced This issue has been reproduced by a Vault engineer label Feb 5, 2024
@hashishaw hashishaw linked a pull request Feb 6, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reproduced This issue has been reproduced by a Vault engineer ui
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants