Re: svn commit: r1839779 - in /apr/apr/trunk/json: apr_json.c apr_json_decode.c
On Sat, Sep 1, 2018 at 1:39 PM Graham Leggett wrote: > > How about this? Works for me, the splitting/factorization I made in my (reverted) commit shouldn't be that bad, and then apr_json_object_set_ex() shouldn't be far from apr__json_object_set() ;) Thanks for the JOSE work anyway, looks very interesting! Regards, Yann.
Re: svn commit: r1839779 - in /apr/apr/trunk/json: apr_json.c apr_json_decode.c
On 01 Sep 2018, at 13:06, Yann Ylavic wrote:
>> Looking at 1839779 and 1839755, both of these need to be reverted as they
>> break RFC compliance with respect to the JSON RFC and JOSE RFCs (I’m -1 on
>> these changes).
>
> There is probably another solution, see below.
>
>>
>> Please can you discuss these changes first before making them, I’ve had my
>> head buried in these RFCs for the past while and I can explain the design
>> choices behind the API, and add any documentation that’s lacking to clarify
>> how this works.
>
> OK, sorry about this.
No worries, it caught me just as I was done on apr_jose. Turns out there are a
whole of of security requirements in the RFCs that depend on JSON, requiring a
whole lot of strictness in base64/base64url processing, JSON processing, etc.
What this shows is I need to document apr_json.h better.
>> JSON whitespace is significant in some RFCs, specifically the JWE and JWS
>> RFCs, and there needs to be a mechanism to formally support whitespace for
>> this API to be be useful in a security sensitive environment.
>
> My changes were meant to allow simple (default) apr_json_object_set
> without having to create a json_string for the key.
> That's IMO how most users want it to be:
>apr_json_object_set(json, "somekey", apr_json__create(...));
> rather than:
>apr_json_object_set(json, apr_json_string_create("somekey"),
> apr_json__create(...));
> …
I’ve at least twice tried to make the same changes you have, and both times
stopped myself going “oh, yeah”. We definitely need a “simple” signature for
the common case as you describe.
How about this?
/**
* Associate a value with a key in a JSON object.
* @param obj The JSON object.
* @param key Pointer to the key string.
* @param klen Length of the key, or APR_JSON_VALUE_STRING if NUL
* terminated.
* @param val Value to associate with the key.
* @param pool Pool to use.
* @return APR_SUCCESS on success, APR_EINVAL if the key is
* NULL or not a string, or the object is not an APR_JSON_OBJECT.
* @remark If the value is NULL the key value pair is deleted.
*/
APU_DECLARE(apr_status_t) apr_json_object_set(apr_json_value_t *obj,
const char *key, apr_ssize_t klen, apr_json_value_t *val,
apr_pool_t *pool) __attribute__((nonnull(1, 2, 5)));
/**
* Associate a value with a key in a JSON object, preserving whitespace.
* @param obj The JSON object.
* @param key Pointer to the key string, including any whitespace
* required.
* @param val Value to associate with the key.
* @param pool Pool to use.
* @return APR_SUCCESS on success, APR_EINVAL if the key is
* NULL or not a string, or the object is not an APR_JSON_OBJECT.
* @remark If the value is NULL the key value pair is deleted.
*/
APU_DECLARE(apr_status_t) apr_json_object_set_ex(apr_json_value_t *obj,
apr_json_value_t *key, apr_json_value_t *val,
apr_pool_t *pool) __attribute__((nonnull(1, 2, 4)));
Regards,
Graham
—
Re: svn commit: r1839779 - in /apr/apr/trunk/json: apr_json.c apr_json_decode.c
On Sat, Sep 1, 2018 at 11:53 AM Graham Leggett wrote:
>
> On 31 Aug 2018, at 19:56, yla...@apache.org wrote:
>
> Author: ylavic
> Date: Fri Aug 31 17:56:40 2018
> New Revision: 1839779
>
> URL: http://svn.apache.org/viewvc?rev=1839779&view=rev
> Log:
> apr_json: follow up to r1839755: preserve formatting when decoding object
> valueT
>
> The object key-value pair parsed by the JSON decoder contained pre/post spaces
> which we lost by changing the key type (to a C string) in
> apr_json_object_set().
>
> Add a new/private apr__json_object_set() helper taking an apr_json_value_t key
> and use it in both apr_json_object_set() and apr_json_decode_object().
>
>
> Looking at 1839779 and 1839755, both of these need to be reverted as they
> break RFC compliance with respect to the JSON RFC and JOSE RFCs (I’m -1 on
> these changes).
There is probably another solution, see below.
>
> Please can you discuss these changes first before making them, I’ve had my
> head buried in these RFCs for the past while and I can explain the design
> choices behind the API, and add any documentation that’s lacking to clarify
> how this works.
OK, sorry about this.
>
> JSON whitespace is significant in some RFCs, specifically the JWE and JWS
> RFCs, and there needs to be a mechanism to formally support whitespace for
> this API to be be useful in a security sensitive environment.
My changes were meant to allow simple (default) apr_json_object_set
without having to create a json_string for the key.
That's IMO how most users want it to be:
apr_json_object_set(json, "somekey", apr_json__create(...));
rather than:
apr_json_object_set(json, apr_json_string_create("somekey"),
apr_json__create(...));
...
>
> I am currently blocked behind these changes, so I’m going to revert them for
> now so that I can get the JOSE code in. We can discuss this in more detail
> once that is done.
I suppose what you want from my changes is to make the private
apr__json_object_set available in the API and use it for apr_jose
(e.g. with a different name like apr_json_object_put or something like
that).
I find apr_json_object_set with a plain C string key more friendly in
the usual case...
Regards,
Yann.
Re: svn commit: r1839779 - in /apr/apr/trunk/json: apr_json.c apr_json_decode.c
On 31 Aug 2018, at 19:56, yla...@apache.org wrote: > Author: ylavic > Date: Fri Aug 31 17:56:40 2018 > New Revision: 1839779 > > URL: http://svn.apache.org/viewvc?rev=1839779&view=rev > Log: > apr_json: follow up to r1839755: preserve formatting when decoding object > valueT > > The object key-value pair parsed by the JSON decoder contained pre/post spaces > which we lost by changing the key type (to a C string) in > apr_json_object_set(). > > Add a new/private apr__json_object_set() helper taking an apr_json_value_t key > and use it in both apr_json_object_set() and apr_json_decode_object(). Looking at 1839779 and 1839755, both of these need to be reverted as they break RFC compliance with respect to the JSON RFC and JOSE RFCs (I’m -1 on these changes). Please can you discuss these changes first before making them, I’ve had my head buried in these RFCs for the past while and I can explain the design choices behind the API, and add any documentation that’s lacking to clarify how this works. JSON whitespace is significant in some RFCs, specifically the JWE and JWS RFCs, and there needs to be a mechanism to formally support whitespace for this API to be be useful in a security sensitive environment. I am currently blocked behind these changes, so I’m going to revert them for now so that I can get the JOSE code in. We can discuss this in more detail once that is done. Regards, Graham —
