Do we want to use serde_json::Value in the public API?

[mkulke]

kbs-types/src/lib.rs

Line 49 in ec9b1b4

pub extra_params: Value,

I guess the idea is to allow arbitrary, untyped json-like data on the extra_params field. I'm not sure that using the type of a serialization library on a public API is a good idea, since this will introduce a transitive dependency on serde_json and consumers might have to deal with version mismatches or having to perform extra conversion when using a different serialization library.

Alternatives would be to provie some sort of flexible tree structure, which doesn't have to be full json, or just type extra_params as Vec<u8>, since processors of extra_params have to perform a parsing step anyway, also with serde_json::Value.

[larrydewey]

For most of the packages in VirTEE we have been placing any serde related dependencies behind a feature flag. I am definitely open to either of these ideas. Let me look into this a bit deeper. It might make sense to allow various methods of passing the extra_params based upon the use-case.

[Xynnn007]

@mkulke I think it would be fine to have serde_json::Value, but we need to pub use serde_json::Value to reexport the struct in kbs-types lib. Btw, the version of serde_json is marked "1" and no Cargo.lock given, thus the concrete version of serde_json would be determined by concrete project that imports kbs-types.

Another observation is that there are two parts that will consume the messages. The version of serde_json or serde differs between sender and receiver does not matter as the two codes does not belong to the same process/binary. wdyt?

[mkulke]

if we use a serialization type as a data-type on the public structure, it will force consumers to pull in serde, serde_json, and serde_derive into their projects, which might be unacceptable in certain situations (restricted environments like fw or a limited set of allowed dependencies).

another nuisance could be version mismatch on a consuming crate if you have to use a pinned version and there are incompatibilities in the patch version.

error: failed to select a version for `serde_json`.
    ... required by package `kbs-types v0.7.0`
    ... which satisfies dependency `kbs-types = "^0.7.0"` of package `bla v0.1.0 (/home/magnuskulke/tmp/bla)`
versions that meet the requirements `^1.0` are: 1.0.45, 1.0.122, ...
the package `kbs-types` depends on `serde_json`, with features: `alloc` but `serde_json` does not have these features.

maybe the ergonomic improvements between raw bytes and untyped json aren't so big either, in both cases you have to serialize/deserialize:

let extra_params_json= json!({
    "extra": "data",
});
request.extra_params = extra_params_json;

...vs

#[repr(C)]
struct ExtraParams {
   "extra": String,    
}
let extra_params = ExtraParams { extra: "data".into() };
request.extra_params = bincode::serialize(&extra_params).unwrap();

..and

let extra_params = serde_json::from_value::<ExtraParams>(request.extra_params).unwrap()
...vs
let extra_params: ExtraParams =  bincode::deserialize(&request.extra_params).unwrap();

[Xynnn007]

@mkulke Yea, actually the change does not import any new crates like serde and serde_json as kbs-types already imports them for a long time. See the change

About dependency version conflict, the version is "1.0" in Cargo.toml. Due to the document, if kbs-types and another project both intend to use serde/serde_json, a version 1.x.x will be used. If you get any errors on it, just try cargo update -p serde_json and it will work I think.

[mkulke]

I think the problem is putting a serialization type on the public api, not using serde in the project (although it should behind a feature flag, imo). We know how to work around this, the issues I'm referencing do not appear in unrestricted high-level code (like trustee) where you can just bump your dependencies freely, it's more about restricted low-level projects (like coconut svsm). If that's not a problem scope for kbs-types, we can close this issue. cc @larrydewey

[Xynnn007]

Might be off of the topic -- in sigstore-rs, we use a same name "shim type" to "re-export" types from 3rd dep, like https://github.com/sigstore/sigstore-rs/blob/main/src/registry/config.rs#L33-L42