maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx