Hi, Julius! On Nov 28, Julius Goryavsky wrote:

Hi, Sergey,

many thanks for the detailed review, I fixed all the flaws and left only what raises questions or where I have explanations:

...

...

+ if ((list= curl_slist_append(list, token_header.c_str())) == NULL || + (list= curl_slist_append(list, "Content-Type: application/json")) == NULL ||

Why do you specify Content-Type header if you aren't sending any content? Seems redundant.

I have already come across servers that respond with the wrong content-type if the input request had a different content-type or if the input request does not contain a content-type at all. Fortunately, the Hashicorp server works without setting the correct content-type in the input, but may be more reliable to set it explicitly?

I don't understand. Content-Type in the request header does not mean that you *accept* application/json, it means you are *sending* application/json. And you are not, you are sending no body at all, just the header.

...

...

+ (vault_ca != NULL && strlen(vault_ca) != 0 && (curl_res= curl_easy_setopt(curl, CURLOPT_CAINFO , vault_ca)) != CURLE_OK) ||

...

How can vault_ca == NULL here? I think it cannot, the check is redundant too.

I don’t know, we have a guarantee that if this option is not set, then the corresponding variable will always point to an empty zero-terminated string, but will never just be NULL instead? (This is the path to the file with the CA bundle. In principle, a Hashicorp Vault can work without it, it is needed only if there is no root or intermediate certificates on the other side.)

Yes, I believe vault_ca can never be NULL. The default value is "", so it'll be either that or a string specified on the command line.

...

...

+ std::string ver_str = std::string(ver, ver_len); + return strtoul(ver_str.c_str(), NULL, 10);

...

Why do you need to do that? create std::string, copy/reallocate it? What's the point? Just do atol(ver)

Does json_get_object_key return a new zero-terminated string? atol cannot be applied to a fragment with a length (without trailing zero), as far as I know.

No, it doesn't return a zero-terminated string. It returns a pointer into a valid json string. So you can be sure that ver[ver_len] is a non-digit character (it'll be '\n' or, may be, '}'). Which is enough for atol().

...

...

+ return ENCRYPTION_KEY_VERSION_INVALID; + return decode_data(key, key_len, dstbuf, buflen);

...

Why? I don't see that in hashicorp docs that the key is base64-encoded

But after all, as far as I understand at this level, the key for us is arbitrary binary data? What if it contains all kinds of quotation marks, commas, or, UTF-8 escape codes and all kinds of unprintable characters like a zero? I thought that it should be base64-endoded if it is binary data, so that we can pass it through JSON encoding in the form of text that will not cause a failure in the JSON parser.

I believe that with the proper escaping json can handle any binary data just fine. With your base64 requirement you basically mean that one cannot use 3rd party tools to create/manage keys, unless the tool has an option to store the keys base64-encoded. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org