Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat] add NewLiveTrustedRootWithClient() #249

Open
ramonpetgrave64 opened this issue Jul 30, 2024 · 7 comments
Open

[feat] add NewLiveTrustedRootWithClient() #249

ramonpetgrave64 opened this issue Jul 30, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@ramonpetgrave64
Copy link

Description

Re: slsa-framework/slsa-verifier#791 (comment)

I'm proposing that we change NewLiveTrustedRoot to accept an existing client, instead of making a new client for each fetch.
Or, we add a new function NewLiveTrustedRootWithClient() that does the same.

My use case is for my library slsa-verifer, where a user may pass in their existing client with custom opts, which slsa-verifier then uses for retrieving a TrustedRoot. Client.opts is private, so my library wouldn't be able to simply pass along the opts.

@ramonpetgrave64 ramonpetgrave64 added the enhancement New feature or request label Jul 30, 2024
@ramonpetgrave64
Copy link
Author

ramonpetgrave64 commented Jul 30, 2024

@codysoyland
Copy link
Member

I'm not opposed to this in principle, but I think I had a good reason not to accept a TUF client in NewLiveTrustedRoot -- maybe something about the TUF client being stateful and not refreshing targets more than once, perhaps?

Currently, NewLiveTrustedRoot does not have any unit test coverage. I would accept this change if we can add unit tests that validate that the targets are indeed updated on each interval.

In order to make this testable, the refresh interval needs to be configurable, which is currently hardcoded to 24 hours. I would be happy to make this interval a required parameter to NewLiveTrustedRoot.

@ramonpetgrave64
Copy link
Author

I'll consider that PR. In the mean time, it seems like it should be more natural for the TUF client itself to do its own update periodically, without the need to instantiate a new TUF client. What's Do you happen to know the reasoning behind why updates are only allowed once per client lifetime?

@codysoyland
Copy link
Member

Do you happen to know the reasoning behind why updates are only allowed once per client lifetime?

Good question... @rdimitrov ?

@rdimitrov
Copy link
Contributor

rdimitrov commented Aug 6, 2024

Do you happen to know the reasoning behind why updates are only allowed once per client lifetime?

Good question... @rdimitrov ?

hey, yes, it is intentional and I can try to find the reasoning @jku has about this (unless he replies here in the meantime).

@jku
Copy link
Member

jku commented Aug 6, 2024

Do you happen to know the reasoning behind why updates are only allowed once per client lifetime?

In python-tuf (where this design likely comes from) the logic is this:

  • python-tuf is a library and does not make decisions about when to do things (like updating metadata or downloading artifacts)
  • the application decides when things happen
  • it's slightly simpler to safely implement the Updater so that it only refreshes metadata once: if the application wants to refresh metadata again, it can just create a new instance

There is nothing that prevents modifying the Updater so that the same instance could refresh metadata from repository multiple times... I just didn't see much advantage to doing so.

@ramonpetgrave64
Copy link
Author

ramonpetgrave64 commented Aug 7, 2024

Thanks for chiming in. Alright, I think the LiveTrustedRoot can call the client's Refresh().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants