Tweaks to better interop with go-tuf #377

lukesteensen · 2022-08-30T17:19:16Z

👋 I've been working on integrating the Vector project with Datadog's TUF/Uptane implementation, and this PR contains the handful of tweaks I've had to make to get everything interoperating happily:

A spec_version of "1.0" is not actually valid according to SemVer. It should probably be "1.0.0" everywhere, but I've kept a check for the old value so that it continues to work with existing metadata.
The datetime parsing was overly strict relative to the spec, which only specifies ISO8601. Here it's adjusted to use a full RFC3339 parser that handles the slightly differing format that go-tuf can emit.
Allowing * in paths, which is legal according to the spec and used somewhat heavily when specifying delegations.
Adding a couple of Send bounds, which isn't related to interop but made the library easier to use.

The spec_version change does seem to break the interop tests, but I wanted to make sure we want to move forward with the change before doing the work of regenerating the test data.

If any these seem undesirable or warrant more discussion, I'd be happy to split them into separate PRs.

/cc @Zenithar @cedricvanrompay-datadog

Signed-off-by: Luke Steensen <[email protected]>

erickt

Hello! Great patch, thanks! Beyond my comments, it looks like there's some test failures from us changing the spec_version. It also might be time to update our golden test files from python-tuf and go-tuf.

--

And sorry that I keep putting out beta versions without cutting a formal release. I think with my delegation fix, and fleshing out the builder more I'm almost ready to commit to not breaking the API. I was tracking that work in #355. One thing to note though I'm not super confident in our delegation support. We aren't currently using it, so outside of the tests here we don't have a lot of real world use cases. For example, I just found that we were using the wrong field name last week in #373. I'd love any help trying to harden that.

tuf/src/interchange/cjson/shims.rs

tuf/src/metadata.rs

Signed-off-by: Luke Steensen <[email protected]>

lukesteensen · 2022-08-30T20:45:57Z

Thanks! I've updated the lib tests, so those should now be passing, but left the interop tests since I'm not sure of the best procedure for generating new fixtures.

As far as testing delegation support, I'll see if some of our TUF experts can have a look and give some feedback. I'm pretty sure we use them at Datadog, but my team is still pretty new to TUF.

trishankatdatadog · 2022-08-30T20:54:44Z

First, nice work!

As far as testing delegation support, I'll see if some of our TUF experts can have a look and give some feedback. I'm pretty sure we use them at Datadog, but my team is still pretty new to TUF.

Second, yes, we do use delegations for full verification. I recommend taking a look at how go-tuf or python-tuf does it.

cedricvanrompay-datadog · 2022-08-31T08:37:29Z

Interesting, it seems that go-tuf has it wrong as well for the spec_version format: https://github.com/theupdateframework/go-tuf/blob/ac7b5d7bce18cca5a84a28b021bd6372f450b35b/data/types.go#L112

trishankatdatadog · 2022-08-31T13:57:28Z

Interesting, it seems that go-tuf has it wrong as well for the spec_version format: https://github.com/theupdateframework/go-tuf/blob/ac7b5d7bce18cca5a84a28b021bd6372f450b35b/data/types.go#L112

If so, would you help us to create a new issue there? Thanks!

trishankatdatadog · 2022-09-01T14:53:21Z

BTW, we don't have keyid_hash_algorithms in the metadata here, do we?

Signed-off-by: Luke Steensen <[email protected]>

lukesteensen · 2022-09-01T21:26:49Z

I've added one more commit here to add a catch-all additional_fields property to ~~TargetsMetadata~~ all of the top level metadata structs. It turns out that go-tuf has a custom field here that is not mentioned in the spec. I didn't necessarily want to duplicate that, but the spec does also say that implementations can use additional fields and they should be maintained, so that's what I did. If someone has a better solution for that I'd be happy to discuss it.

Signed-off-by: Luke Steensen <[email protected]>

trishankatdatadog

IDK Rust, but the ideas therein seems good enough to me 👍🏽

erickt · 2022-09-08T01:57:45Z

It looks like we also got some test failures. I'd also like the additional_fields to have some tests too. It might be better to factor that out into a separate PR so it doesn't conflate with the other changes.

erickt · 2022-09-12T18:56:51Z

FYI I did find a bug in rust-tuf's delegation #381.

@lukesteensen

In theupdateframework#377, @lukesteensen noticed that we created metadata with the wrong spec version. We used `1.0`, but the proper form is `1.0.0`. Unfortunately landing this fix will be non-trivial, since old versions of rust-tuf will error out if the spec version is not `1.0`. As a stopgap, this patch changes rust-tuf to allow either a spec version of `1.0` or `1.0.0` so that we can switch to the proper schem once all the old clients have upgraded, or we come up with another way to gracefully perform this migration.

erickt · 2022-09-23T04:55:48Z

I just realized that switching our metadata generation to building metadata with the spec_version of 1.0.0 would cause old clients to reject the now spec-conformant metadata, so we need to come up with a way to do a graceful migration.

As a stopgap, I factored out some of your fix in #386 to make sure new clients will at least be able to support the proper format once we start producing it.

@lukesteensen

This extracts @lukesteensen's RFC 3339 timestamp parser from theupdateframework#377. This allows rust-tuf to interoperate better with other TUF implementations, which might include things like microseconds, or not be in the UTC timezone.

@lukesteensen

From theupdateframework#377, the [TUF spec] allows for paths to contain `*`. According to @lukesteensen, this is routinely used in delegations. Note though this does not attempt to implement delegation path globs. That will be implemented in theupdateframework#388. [TUF spec]: https://theupdateframework.github.io/specification/latest/#targetpath

@lukesteensen

In #377, @lukesteensen noticed that we created metadata with the wrong spec version. We used `1.0`, but the proper form is `1.0.0`. Unfortunately landing this fix will be non-trivial, since old versions of rust-tuf will error out if the spec version is not `1.0`. As a stopgap, this patch changes rust-tuf to allow either a spec version of `1.0` or `1.0.0` so that we can switch to the proper schem once all the old clients have upgraded, or we come up with another way to gracefully perform this migration.

@lukesteensen

This extracts @lukesteensen's RFC 3339 timestamp parser from theupdateframework#377. This allows rust-tuf to interoperate better with other TUF implementations, which might include things like microseconds, or not be in the UTC timezone.

@lukesteensen

This extracts @lukesteensen's RFC 3339 timestamp parser from #377. This allows rust-tuf to interoperate better with other TUF implementations, which might include things like microseconds, or not be in the UTC timezone.

@lukesteensen

From theupdateframework#377, the [TUF spec] allows for paths to contain `*`. According to @lukesteensen, this is routinely used in delegations. Note though this does not attempt to implement delegation path globs. That will be implemented in theupdateframework#388. [TUF spec]: https://theupdateframework.github.io/specification/latest/#targetpath

@lukesteensen

From #377, the [TUF spec] allows for paths to contain `*`. According to @lukesteensen, this is routinely used in delegations. Note though this does not attempt to implement delegation path globs. That will be implemented in #388. [TUF spec]: https://theupdateframework.github.io/specification/latest/#targetpath

lukesteensen · 2022-10-01T18:48:46Z

@erickt Thanks so much for taking most of these across the line! Apologies for the radio silence, I had a couple weeks of travel there and some conflicting work priorities. It seems like the additional_fields piece, as well as maybe the Send bound tweak, are all that's left? This coming week I'm happy to open a more specific PR for just that.

erickt · 2022-10-01T18:51:00Z

Yeah, although I saw and landed a patch that removed a number of unnecessary trait bounds. It might be that inheriting Send might not be necessary anymore.

lukesteensen added 4 commits August 30, 2022 11:57

adjust spec_version to proper semver format

3d8cb88

Signed-off-by: Luke Steensen <[email protected]>

use RFC3339 functions for datetime handling

4b48acf

Signed-off-by: Luke Steensen <[email protected]>

add Send bound to repository types

70283a7

Signed-off-by: Luke Steensen <[email protected]>

don't consider asterisks illegal in paths

c38603b

Signed-off-by: Luke Steensen <[email protected]>

erickt requested changes Aug 30, 2022

View reviewed changes

tuf/src/interchange/cjson/shims.rs Show resolved Hide resolved

tuf/src/interchange/cjson/shims.rs Show resolved Hide resolved

tuf/src/metadata.rs Show resolved Hide resolved

lukesteensen added 3 commits August 30, 2022 15:30

back out datetime formatting change since it doesn't actually match spec

c1313e1

Signed-off-by: Luke Steensen <[email protected]>

add tests for datetime parsing, spec_version, and path asterisks

4dfc36e

Signed-off-by: Luke Steensen <[email protected]>

adjust existing tests for new spec_version

94ccc7f

Signed-off-by: Luke Steensen <[email protected]>

collect additional fields on TargetsMetadata

f941bab

Signed-off-by: Luke Steensen <[email protected]>

add additional_fields handling to root, timestamp, and snapshot metas

f0d6ca3

Signed-off-by: Luke Steensen <[email protected]>

trishankatdatadog reviewed Sep 3, 2022

View reviewed changes

erickt mentioned this pull request Sep 23, 2022

Treat spec_version 1.0.0 as valid #386

Merged

erickt mentioned this pull request Sep 23, 2022

Support RFC3339 timestamps #387

Merged

erickt mentioned this pull request Sep 23, 2022

Allow paths to contain * #389

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweaks to better interop with go-tuf #377

Tweaks to better interop with go-tuf #377

lukesteensen commented Aug 30, 2022 •

edited

Loading

erickt left a comment

lukesteensen commented Aug 30, 2022

trishankatdatadog commented Aug 30, 2022 •

edited

Loading

cedricvanrompay-datadog commented Aug 31, 2022

trishankatdatadog commented Aug 31, 2022

trishankatdatadog commented Sep 1, 2022

lukesteensen commented Sep 1, 2022 •

edited

Loading

trishankatdatadog left a comment

erickt commented Sep 8, 2022

erickt commented Sep 12, 2022

erickt commented Sep 23, 2022

lukesteensen commented Oct 1, 2022

erickt commented Oct 1, 2022

Tweaks to better interop with go-tuf #377

Are you sure you want to change the base?

Tweaks to better interop with go-tuf #377

Conversation

lukesteensen commented Aug 30, 2022 • edited Loading

erickt left a comment

Choose a reason for hiding this comment

lukesteensen commented Aug 30, 2022

trishankatdatadog commented Aug 30, 2022 • edited Loading

cedricvanrompay-datadog commented Aug 31, 2022

trishankatdatadog commented Aug 31, 2022

trishankatdatadog commented Sep 1, 2022

lukesteensen commented Sep 1, 2022 • edited Loading

trishankatdatadog left a comment

Choose a reason for hiding this comment

erickt commented Sep 8, 2022

erickt commented Sep 12, 2022

erickt commented Sep 23, 2022

lukesteensen commented Oct 1, 2022

erickt commented Oct 1, 2022

lukesteensen commented Aug 30, 2022 •

edited

Loading

trishankatdatadog commented Aug 30, 2022 •

edited

Loading

lukesteensen commented Sep 1, 2022 •

edited

Loading