go vault API (*client).Logical().ReadWithData() 404 handling issue

[eau-u4f]

Hello Vault,

I am not sure, I use it for the first time on a small project, I have no knowledge of your products/projects or all the different use cases and/or exceptions, so bear with my humble understanding of your new (to me) codebase.

I am facing a simple issue, where (*client).Logical().ReadWithData() (and per extension (*client).Logical().Read()) returns err == nil when requesting a kv-v2 secret that does not exist (http status code 404):

vault/api/logical.go

Line 91 in c1495da

return nil, nil

The logic in ReadWithData() for handling 404 seems to have a conflicting history:
https://github.com/hashicorp/vault/blame/c1495da531a7bd9c6dda6b3a08a8cbc97c419ea3/api/logical.go#L79

Quickly simulating (*client).Logical().Read() independently calling (*client).RawRequest(req) on a manually forged identical request reveals a 404 is returned and err != nil, but reading the function, err is tested AFTER parsing the response body of the request, which seems erroneous? (dangerous?)
It seems logical to test the error right after the request or I am missing something completely in this case that requires to Parse the body before error checking and especially with a 404 code? do you return secrets to parse when they don't exist?

func (c *Logical) ReadWithData(path string, data map[string][]string) (*Secret, error) {
       //
       // Skipped for brevity
       //

        resp, err := c.c.RawRequestWithContext(ctx, r)
	if resp != nil {
		defer resp.Body.Close()
	}
	if resp != nil && resp.StatusCode == 404 { // why if the response is 404 you parse?
		secret, parseErr := ParseSecret(resp.Body) // ParseSecret returns nil if body len == 0
		switch parseErr {
		case nil:
		case io.EOF:
			return nil, nil
		default:
			return nil, parseErr // this ends up being nil, nil for body len == 0
		}
		if secret != nil && (len(secret.Warnings) > 0 || len(secret.Data) > 0) {
			return secret, nil
		}
		return nil, nil
	}
	if err != nil { // the test if the request fail is here.
		return nil, err
	}

	return ParseSecret(resp.Body)
}

May be this (simplifying a bit) ? :

        resp, err := c.c.RawRequestWithContext(ctx, r)
        if err != nil {
                 return nil, err
        }
	if resp != nil {
		defer resp.Body.Close()
	}
	return ParseSecret(resp.Body)
}

and in

vault/api/secret.go

Line 303 in c1495da

func ParseSecret(r io.Reader) (*Secret, error) {

handle the io.EOF, or leave the handling to encode/json in your jsonutil.DecodeJSONFromReader().

func ParseSecret(r io.Reader) (*Secret, error) {
	// First read the data into a buffer. Not super efficient but we want to
	// know if we actually have a body or not.
	var buf bytes.Buffer
	_, err := buf.ReadFrom(r)
	if err != nil && err != io.EOF {
		return nil, err
	}
	if buf.Len() == 0 {
		return nil, fmt.Errorf("no secret available")
	}

	// First decode the JSON into a map[string]interface{}
	var secret Secret
	if err := jsonutil.DecodeJSONFromReader(&buf, &secret); err != nil {
		return nil, err
	}
	return &secret, nil
}

Also, not related but in ParseSecret() you have an io.Reader parameter and recreate an io.Reader (bytes.Buffer), to feed an API able to use an io.Reader already, wouldn't this work (I have not verified but it seems encoding/json used in jsonutil.DecodeJSONFromReader handles it)?

func ParseSecret(r io.Reader) (*Secret, error) {
	// First decode the JSON into a map[string]interface{}
	var secret Secret
	if err := jsonutil.DecodeJSONFromReader(r, &secret); err != nil {
		return nil, err
	}
	return &secret, nil
}

Did I miss something?
Please advice, thanks!

[VinnyHC]

Hello @eau-u4f and thanks for reaching out!
The commit that adds some of the functionality in question has a pretty good message that I believe answers your question:

On read it'll output data and/or warnings on a 404 if they exist. On
list, the same behavior; the actual 'vault list' command doesn't change
behavior though in terms of output unless there are no actual keys (so
it doesn't just magically show other data).
This corrects some assumptions in response_util and wrapping.go; it also
corrects a few places in the latter where it could leak a (useless)
token in some error cases.

And I believe this code highlights that functionality.

Let us know if I misunderstood your concern!

[eau-u4f]

Hey @VinnyHC !

Thank you for your reply, well the reason for my question is simple:

I use (*client).Logical().Read() on a versioned kv-v2, the underlying request (the whole vault internal dance, ReadWithData(), RawRequestWithContext() followed by ParseSecret()) receive a 404 (the requested secret version does NOT exists), but that call returns a nil, nil (because the 404 response return an empty body and in:

vault/api/secret.go

Line 303 in c1495da

func ParseSecret(r io.Reader) (*Secret, error) {

if buf.Len() == 0 then it returns nil, nil) , which seems erroneous, I read an element that does NOT exist, it should return an error, no?

You end up using what you think is a valid (*Secret) (returned by Logical().Read()) and deref a nil ptr.

This happens when for example you request a kv-v2 value for which the version does not exist,
I would expect the Read() (or ReadWithData() to be exact) to either return (*Secret), nil OR nil, error but NOT nil, nil

Why would an API call return both no pointer and no error? how do you know something went wrong ?!

The rest of my comment/issue was proposing to simplify the code and remove cases that seems unclear/unfit in this whole call chain logic as well as solving what seems to be a bug, but maybe I miss something?

[VinnyHC]

I think there are a few things going on here and unfortunately, none of them are a perfect response.
(*client).Logical().Read() is a general method that can target lots of resources some of which can in fact return a 404 at the HTTP layer (adhering to the typical Not Found response and code you'd expect) but in our API that does not always mean an error occurred, or rather that something went wrong; in some cases, a 404 Not Found is the expected result and thus nil, nil is what consumers of the API would expect, better worded, "have come to expect".
There is also an unfortunate overuse of the term secret in our code-base and in this case, it's simply referring to the response.
So the short of it is when using (*client).Logical().Read() (or one of its derivatives) to consume an expected result, today you should first check that it is not nil. As we start to think about what a new version of our API libraries might look like these improvements will certainly be evaluated.
I'm going to close this issue but if you feel that this explanation is not sufficient feel free to comment and I'll try my best to situate a better response.

[eau-u4f]

Thank you for your answer, I'll check the pointer as well from now on, since the error is not reliable (and dependent on resource type).
I probably missed it, but could you point me to the API documentation describing those resource dependent behaviors for Read() (or "use the source luke!" || trial-error?)

[VinnyHC]

Sorry, I think I mislead you a bit and I don't disagree that this behavior could likely lead to false assumptions but in Vault's API, the code is fairly consistent in that just because something isn't there it's not an error to look for it. An example of this pattern elsewhere is the storage system where again nil, nil is the expected response when the resource is not present.
This issue has definitely sparked some internal discussions about how we can improve this experience!