JSON canonicalization uses multiple libraries.

[vembacher]

Description

Different parts of code use different libraries for JSON canonicalization.

Examples:

sigstore-rs/src/sign.rs

Lines 327 to 332 in d5ba303

	let canonicalized_body = {
	let mut body = json_syntax::to_value(self.log_entry.body)
	.expect("failed to parse constructed Body!");
	body.canonicalize();
	Some(base64.encode(body.compact_print().to_string()))
	};

sigstore-rs/src/cosign/bundle.rs

Lines 81 to 88 in d5ba303

	let mut buf = Vec::new();
	let mut ser = serde_json::Serializer::with_formatter(&mut buf, CanonicalFormatter::new());
	bundle.payload.serialize(&mut ser).map_err(|e| {
	SigstoreError::UnexpectedError(format!(
	"Cannot create canonical JSON representation of bundle: {e:?}"
	))
	})?;

[tnytown]

I was responsible for pulling in the second canonicalizer (json_syntax).

For context: the Rekor OpenAPI spec mentions that RFC 8785 style canonicalization should be used for SET verification. The cosign code uses OLPC style canonicalization. Investigating further, it seems that the major differences between these two canonicalization schemes are in floating-point numbers (OLPC doesn't allow them) and strings (RFC 8785 escapes control characters).

Suggestion: Can we standardize on json_syntax (or another RFC 8785 canonicalizer) across the codebase? We should never see these differences, but I figure that we are better safe than sorry :)

xref #285 (comment)

[flavio]

I don't think it would be worth to drop the olpc-cjson dependency since it's a a transitive dependency of oci-distribution and tough.

json-syntax is a first level one. I don't think we can use olpc-cjson to validate Rekor's data. Maybe we could make this crate optional and require it only when the rekor feature is enabled

[heysujal]

It seems this issue has been resolved already in 68e640b

I saw that json-syntax has been made optional in depedencies. But it is not added as an optional dependency on rekor. Also, it was not removed from feature group sign and verify.

I think this above discussion is outdated and recent commit I mentioned has already audited the need of these dependencies.
Still, if there is any change required. I would like to work on it.