maria-developers team mailing list archive
Mailing list archive
Re: 387839e44cc: Plugin for Hashicorp Vault Key Management System
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
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
> >> + 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
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.
VP of MariaDB Server Engineering