From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 2DFA1A0032;
	Fri, 24 Jun 2022 10:14:10 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id D197840A87;
	Fri, 24 Jun 2022 10:14:09 +0200 (CEST)
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
 by mails.dpdk.org (Postfix) with ESMTP id 788F940A82
 for <dev@dpdk.org>; Fri, 24 Jun 2022 10:14:06 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple;
 d=intel.com; i=@intel.com; q=dns/txt; s=Intel;
 t=1656058448; x=1687594448;
 h=date:from:to:cc:subject:message-id:references:
 mime-version:content-transfer-encoding:in-reply-to;
 bh=ME9OFh4FeN0XKdWmmVYZ4cdHJaSMCLXBAcI6GZP+Yyg=;
 b=VOZViOztVyln3G6A5WRODkUKGLj4eYs4S5lvBDdEi9T6bX3iBKLxA0NB
 o2cVGWnkHHicge+XB5+OFXlrwOe/JDrgIg8MrzRHDuUwWRn/MSQM3sxYk
 iJlbxPlzwnUW4DQkvun7F6SluRNEvTfVENh4pf+i7Al0o2OoWngT6vW3g
 sS7jqcjmVcdkakMLB+qBpZxwYO6VB0aY+4rb16+xCGLvfvyVTUglOsF3M
 00diFrmCGPn2UUr55kQuHxksTqupnUUAnkzPItAOHRF+9l38eEvIgzxsj
 PHoFr0Y52L6sgcerYVtzXnRfZ3Pqlc3Zf/QB+IjxQq1T1ks8x9yhhXfcC Q==;
X-IronPort-AV: E=McAfee;i="6400,9594,10387"; a="278499926"
X-IronPort-AV: E=Sophos;i="5.92,218,1650956400"; d="scan'208";a="278499926"
Received: from orsmga008.jf.intel.com ([10.7.209.65])
 by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
 24 Jun 2022 01:14:04 -0700
X-IronPort-AV: E=Sophos;i="5.92,218,1650956400"; d="scan'208";a="615899394"
Received: from bricha3-mobl.ger.corp.intel.com ([10.252.25.171])
 by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA;
 24 Jun 2022 01:14:01 -0700
Date: Fri, 24 Jun 2022 09:13:58 +0100
From: Bruce Richardson <bruce.richardson@intel.com>
To: Morten =?iso-8859-1?Q?Br=F8rup?= <mb@smartsharesystems.com>
Cc: dev@dpdk.org, ciara.power@intel.com, fengchengwen@huawei.com
Subject: Re: [RFC PATCH 0/6] add json string escaping to telemetry
Message-ID: <YrVyRgsDUZvlaHIX@bricha3-MOBL.ger.corp.intel.com>
References: <20220623164245.561371-1-bruce.richardson@intel.com>
 <98CBD80474FA8B44BF855DF32C47DC35D8716D@smartserver.smartshare.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D8716D@smartserver.smartshare.dk>
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org

On Thu, Jun 23, 2022 at 09:04:31PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Thursday, 23 June 2022 18.43
> > 
> > This RFC shows one possible approach for escaping strings for the json
> > output of telemetry library. For now this RFC supports escaping strings
> > for the cases of returning a single string, or returning an array of
> > strings. Not done is escaping of strings in objs/dicts [see more below
> > on TODO]
> 
> Very good initiative.
> 
> > 
> > As well as telemetry lib changes, this patchset includes unit tests for
> > the above and also little bit of cleanup to the json tests.
> > 
> > TODO:
> > Beyond what is here in this RFC:
> > 
> > 1. we need to decide what to do about name/value pairs. Personally, I
> >    think we should add the restriction to the "rte_tel_data_add_obj_*"
> > APIs
> >    to only allow a defined subset of characters in names: e.g.
> > alphanumeric
> >    chars, underscore and dash. That means that we only need to escape
> >    the data part in the case of string returns.
> 
> I agree about only allowing a subset of characters in names, so JSON (and other) encoding is not required.
> 
> However, I think we should be less restrictive, and also allow characters commonly used for separation, indexing and wildcard, such as '/', '[', ']', and '*', '?' or '%'.
> 
> Obviously, we should disallow characters requiring escaping in not just JSON, but also other foreseeable encodings and protocols. So please bring your crystal ball to the discussion. ;-)
> 
Exactly why I am looking for feedback - and why I'm looking to have an
explicit allowed list of characters rather than trying to just block the
known-bad in json ones.

For your suggestions: +1 to separators and indexing, i.e. '[', ']' and '/',
though I would probably also add ',' and maybe '.' (unless it's likely to
cause issues with some protocol we are likely to want to use).
For the wildcarding, I find it hard to see why we would want those?

The other advantage of using an allowlist of characters is that it makes it
possible to expand over time, compared to a blocklist which always runs the
risk of breaking something if you expand it. Therefore I suggest we keep
the list as small as we need right now, and expand it only as we need.

> > 2. once agreed, need to implement a patch to escape strings in
> >    dicts/objs
> 
> Yes.
> 
> > 
> > 3. need to add a patch to escape the input command if it contains
> >    invalid chars
> 
> What do you mean here? You mean unescape JSON encoded input (arriving on the JSON telemetry socket) to a proper binary string?
> 

The thing with the telemetry socket interface right now is that the input
requests are not-json. The reasons for that is that they be kept as simple
as possible, and to avoid needing a full json parser inside DPDK.
Therefore, the input sent by the user could contain invalid characters for
json output so we need to:
1. Guarantee that no command registered with the telemetry library contains
   invalid json characters (though why someone would do so, I don't know!)
2. When we return the command back in the reply, properly escape any
   invalid characters in the error case.

#1 is very important for sanity checking, but now that I think about it #2
is probably optional, since if any user does start sending invalid garbage
input that breaks their json parser on return, they are only hurting
themselves and not affecting anything else on the system.

> > 4. some small refactoring of the main telemetry.c json-encoding
> > function may be possible.
> 
> Perhaps.
> 
I saw some options for cleanup when I was working on the code, so including
this as a note-to-self as much as anything else for feedback. :-)

/Bruce