unseal with wrong key returns HTTP 500 instead of 400 (again?)

[arusso]

Describe the bug

Attempting to unseal with an incorrect but valid key (ie. old key) returns a 500 instead of a 400.

To Reproduce
Steps to reproduce the behavior:

1. vault server -dev
2. VAULT_ADDR=http://localhost:8200 vault operator seal
3. VAULT_ADDR=http://localhost:8200 vault operator unseal and pass in an invalid token.

❯ ./vault server -dev
==> Vault server configuration:

             Api Address: http://127.0.0.1:8200
                     Cgo: disabled
         Cluster Address: https://127.0.0.1:8201
              Go Version: go1.19.2
              Listener 1: tcp (addr: "127.0.0.1:8200", cluster address: "127.0.0.1:8201", max_request_duration: "1m30s", max_request_size: "33554432", tls: "disabled")
               Log Level: info
                   Mlock: supported: true, enabled: false
           Recovery Mode: false
                 Storage: inmem
                 Version: Vault v1.12.1, built 2022-10-27T12:32:05Z
             Version Sha: e34f8a14fb7a88af4640b09f3ddbb5646b946d9c

==> Vault server started! Log data will stream in below:
...

❯ VAULT_ADDR=http://localhost:8200 ./vault status
Key             Value
---             -----
Seal Type       shamir
Initialized     true
Sealed          false
Total Shares    1
Threshold       1
Version         1.12.1
Build Date      2022-10-27T12:32:05Z
Storage Type    inmem
Cluster Name    vault-cluster-7eb7d897
Cluster ID      1ff6d010-8f4d-d22b-081d-6eb6542bcb5c
HA Enabled      false

❯ VAULT_ADDR=http://localhost:8200 ./vault operator seal
Success! Vault is sealed.

❯ VAULT_ADDR=http://localhost:8200 ./vault operator unseal
Unseal Key (will be hidden): <ENTER INCORRECT KEY>
Error unsealing: Error making API request.

URL: PUT http://localhost:8200/v1/sys/unseal
Code: 500. Errors:

* unable to retrieve stored keys: failed to decrypt keys from storage: cipher: message authentication failed

❯ VAULT_ADDR=http://localhost:8200 ./vault operator unseal
Unseal Key (will be hidden): <ENTER CORRECT KEY>
Key             Value
---             -----
Seal Type       shamir
Initialized     true
Sealed          false
Total Shares    1
Threshold       1
Version         1.12.1
Build Date      2022-10-27T12:32:05Z
Storage Type    inmem
Cluster Name    vault-cluster-7eb7d897
Cluster ID      1ff6d010-8f4d-d22b-081d-6eb6542bcb5c
HA Enabled      false

Expected behavior
The error returns is a 400 error, clearly indicating a client/request issue and not a server error.

Environment:

• Vault Server Version (retrieve with vault status):

❯ VAULT_ADDR=http://localhost:8200 ./vault status
Key             Value
---             -----
Seal Type       shamir
Initialized     true
Sealed          false
Total Shares    1
Threshold       1
Version         1.12.1
Build Date      2022-10-27T12:32:05Z
Storage Type    inmem
Cluster Name    vault-cluster-7eb7d897
Cluster ID      1ff6d010-8f4d-d22b-081d-6eb6542bcb5c
HA Enabled      false

• Vault CLI Version (retrieve with vault version):

❯ ./vault version
Vault v1.12.1 (e34f8a14fb7a88af4640b09f3ddbb5646b946d9c), built 2022-10-27T12:32:05Z

• Server Operating System/Architecture:

Vault server configuration file(s):

N/A (default in-memory database with no config)

Additional context
We encountered this on an update from 1.10.3 -> 1.12.1. I confirmed issue is present on both versions.

Looks like this might be a regression? This was previously an issue and fixed in #8425.

[nsimons]

I had a go at this.

I don't think that this was fixed in 1.11.x as the referenced ticket mentions.
The error comes from the fact that the unseal key is first used to decrypt the master key, before the actual unsealing happens (new-style shamir). In legacy shamir, the unseal key was the same as the master key, so the code went straight to unsealing where errors are handled correctly.

[hghaf099]

Thanks for filing this ticket. I just checked the response error code going back to Vault 1.8, and we have always returned 500 error code if the unseal key is a properly b64 encoded key, however, it is invalid. On the other hand, providing an improper key, like unseal-key=1, to the unseal API will return the following though:

Error unsealing: Error making API request.

URL: PUT http://localhost:8200/v1/sys/unseal
Code: 400. Errors:

* 'key' must be a valid hex or base64 string

I am going to close this ticket for now. Please reopen this issue or open a new one for further discussions.

[arusso]

Based on the previous comment, it sounds like this was never actually fixed, and looking at the 1.8.0 behavior confirms that.

Looking at the 1.0.0 behavior, it seems like it used to.

❯ ./vault version
Vault v1.0.0 ('c19cef14891751a23eaa9b41fd456d1f99e7e856')

❯ VAULT_ADDR=http://localhost:8200 vault status
Key                Value
---                -----
Seal Type          shamir
Initialized        true
Sealed             true
Total Shares       1
Threshold          1
Unseal Progress    0/1
Unseal Nonce       n/a
Version            1.0.0
Storage Type       n/a
HA Enabled         false

❯ VAULT_ADDR=http://localhost:8200 ./vault operator unseal
Unseal Key (will be hidden): Zm9vZm9vZm9vZm9vZm9vCg==
Error unsealing: Error making API request.

URL: PUT http://localhost:8200/v1/sys/unseal
Code: 400. Errors:

* Unseal failed, invalid key

[arusso]

@hghaf099 Doesn't seem like I can re-open this issue, but I still feel like this is a bug worth fixing.

The response return, be it status code or message, is inadequate to inform the user of the actual issue -- that the specified key is invalid. A 500 error suggests the server encountered something that was an unexpected condition -- but an invalid key should be an expected condition and return a non-500 error like it used to.

[heatherezell]

@arusso One thing I would note is that Vault has not always returned the "expected" error or code, as a defense against fuzzing. There are no right or wrong answers here, so please assume positive intent on my part when I ask what your expectations may be, for both a positive user experience and a negative fuzzing experience? I am genuinely curious, and I know Hamid and all of our engineering and product crew are too. Thanks!

[arusso]

I understand the point about fuzzing, but in this case unsealing happens infrequently and once unsealed, attempting to further unseal isn't useful. I wouldn't expect vaults to be in an unsealed state for a long time.

I think given that assumption on my part, that the returning of a 400 is still acceptable since it only reveals what's already obvious, either the unseal worked or it didn't.

By returning a 500 error, you're only confusing the vault admin because they've got a generic server side error that, at least to me, seems to indicate the inability to unseal their vault, implying some sort of corruption / etc.

[heatherezell]

Thank you for your input - genuinely! This is always a bit of a gray area for how the Vault engineering teams handle these kinds of cases. There are always arguments both pro and con. Please rest assured that I'll make sure we have a discussion about this with all our stakeholders. I appreciate your patience!

[nsimons]

My use case is that I have several Vault clusters, each with their own unseal keys. When accidentally unsealing a cluster with the wrong unseal key, you get back a cryptic error that seems like it is a temporary issue, so you should retry. Of course, by now I have internalized that 'HTTP 500 - message authentication failed' is an user error and actually means the unseal key is wrong. But still, I think UX takes a hit here.

[hghaf099]

Thanks again for filing this issue. The above discussion make sense and we discussed it internally, and we would like to get the issue fixed.