Hi Aurélien,
On Thu, Apr 19, 2018 at 06:16:37PM +0200, Aurélien Nephtali wrote:
> I guess I was sent off-track by the will to make something very flexible
> (at least in my mind) despite your multiple good examples, sorry about
> that.
No problem. That's exactly what design discussions are made for : more talk,
less code at the beginning, resulting in less talk and less regrets at the
end.
> Anyway, here are three patches that, I hope, will do what is needed:
> - add a way to specify an optional payload by using << at the end of
> the first line of a command
> - add payload support to "add map"
> - add payload support to "set ssl ocsp-response"
>
> More commands could benefit from using a payload but for a start I only
> did these two.
That's nice. I think we're good here. I have some comments (questions)
but nothing important :
> In order to use arbitrary data in the CLI (multiple lines or group of words
> that must be considered as a whole, for example), it is now possible to add a
> payload to the commands. To do so, the first line needs to end with a special
> pattern: <<\n.
It seems to me that some places in the parser look for "<<" anywhere on the
line (mainly the strstr() which even skips trailing spaces/tabs), and some
parts of the logic only expect it at the end.
I'm personally perfectly fine with being strict at the beginning and doing
as you documented, ie using "<<\n" as the delimiter so that users don't get
used to start to put it anywhere. I noticed anyway that it's not recognized
as the delimiter if not at the end, but the logic seems to check for it.
> + p = appctx->chunk->str;
> + end = p + appctx->chunk->len;
> +
> + /* look for a payload */
> + if (appctx->st1 & APPCTX_CLI_ST1_PAYLOAD) {
> + payload = strstr(p, PAYLOAD_PATTERN);
> + end = payload;
> + /* skip the pattern */
> + payload += strlen(PAYLOAD_PATTERN);
> + /* skip whitespaces */
> + payload += strspn(payload, " \t");
> + }
Be extremely careful with the str* functions in haproxy. Due to being used
to process buffers in-place, most of the time our strings are *not* zero-
terminated, which is why they're often put in chunks made of (str,len),
or the new immediate strings (ist) also made of (ptr,len). The internal
API is not very rich regarding this so some functions are implemented
like strnistr() and others using ist like istist() which does the same
as strstr() but on ist strings. It's among the things we have to do to
unify the internal strings API as for now many operations are simply
open-coded. At the very least, wherever the chunk is filled, a comment
should indicate that the trailing zero is assumed to be present in the
rest of the code, because the risk of the code being changed without
keeping it is high.
> + chunk_appendf(appctx->chunk, "%s", trash.str);
Given that this will result in appctx->chunk always being equal to trash.str,
I think you should take a look at cli_io_handler() to check if it still makes
sense at all to use the trash there. Maybe you should simply rely on this
allocated chunk all the time and save this copy and formatting.
> From 63439f2b089613973f72a37dd5dda17f45f26545 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <[email protected]>
> Date: Wed, 18 Apr 2018 14:04:47 +0200
> Subject: [PATCH 2/3] MINOR: map: Add payload support to "add map"
(...)
> diff --git a/src/map.c b/src/map.c
> index d02a0255c..64222dc2e 100644
> --- a/src/map.c
> +++ b/src/map.c
> + const char *end = payload + strlen(payload);
> +
> + while (payload < end) {
(...)
> + /* value */
> + payload += strspn(payload, " \t");
> + value = payload;
> + l = strcspn(value, "\n");
> + value[l] = 0;
> + payload += l + 1;
I think this one is not exactly good, as a string ending in "123\0" will
cause payload to point past the \0. While there's theorically nothing
wrong with this, and we know that on all supported architecture, ~0 is
already not a valid pointer so this will not cause payload to become < end,
it can at least be confusing when debugging. I'd rather do :
payload += l;
if (*payload)
payload++;
> From 1b7f6c40b010521a8b55103c984d3c8f305e818c Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <[email protected]>
> Date: Wed, 18 Apr 2018 14:04:58 +0200
> Subject: [PATCH 3/3] MINOR: ssl: Add payload support to "set ssl
(...)
> --- a/src/ssl_sock.c
> +++ b/src/ssl_sock.c
> @@ -8565,16 +8565,27 @@ static int cli_parse_set_ocspresponse(char **args,
> char *payload, struct appctx
> {
> #if (defined SSL_CTRL_SET_TLSEXT_STATUS_REQ_CB && !defined OPENSSL_NO_OCSP)
> char *err = NULL;
> + int i, j;
> +
> + if (!payload)
> + payload = args[3];
>
> /* Expect one parameter: the new response in base64 encoding */
> - if (!*args[3]) {
> + if (!*payload) {
> appctx->ctx.cli.severity = LOG_ERR;
> appctx->ctx.cli.msg = "'set ssl ocsp-response' expects response
> in base64 encoding.\n";
> appctx->st0 = CLI_ST_PRINT;
> return 1;
> }
>
> - trash.len = base64dec(args[3], strlen(args[3]), trash.str, trash.size);
> + /* remove \r and \n from the payload */
> + for (i = 0, j = 0; payload[i]; i++) {
> + if (payload[i] == '\r' || payload[i] == '\n')
> + continue;
> + payload[j++] = payload[i];
> + }
> +
> + trash.len = base64dec(payload, strlen(payload), trash.str, trash.size);
Here there is an issue, if you have some '\n' or '\r' in the payload, then
strlen() will be wrong and a few extra chars will be sent :
azert\nyuiop\nqsdfg\nhjklm\n
will become :
azertyuiopqsdfghjklmklm\n
<------------------> j
<-----------------------> strlen(payload)
Note that it will not be noticeable on small strings spanning over 2 or 3
lines as the trailing "=" will simply be replicated at the end, still
looking like a valid end of string, but it will randomly break for longer
strings.
So you should use "j" instead of strlen(payload) here.
So please take a look at these points, and once you think they are OK
(or don't need to be changed), please mention the command changes in the
management doc for "add map" and "set ssl ocsp-response", and we can merge
this.
Thanks!
Willy
