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 secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
