Hardcoded timeout in updater.DownloadTarget

[eikenb]

trafficstars

https://github.com/theupdateframework/go-tuf/blob/990cdb83f6652d4ab430a9a8c9af520d029fd6d9/metadata/updater/updater.go#L248

We're hitting the timeout above when downloading files using the updater and want to raise it. Maybe this should be passed in as a parameter or added to the UpdaterConfig? Our current workaround is a re-implementation of the DownloadTarget() function that lets us set that value.

[rdimitrov]

hey, @eikenb 👋

Thanks for the raising this 👍 So far we haven't had anyone else raise that 15 seconds are not enough to download a target, but I do see how hardcoding it can become an issue (big targets, slow network, etc) 👍

I've briefly checked the specification and there's no explicit mentioning of it, so it's up to the implementations to set this accordingly. That said I agree it's fair to make this configurable 👍

We'll add it to the project planning and we'll try to include in the next release. Of course we'll be more than happy to welcome a PR in case you already have something on your side 🙌 That would make things move up much faster.

[sureshkrishnan-v]

Hi, I’d like to work on this issue as a first-time contributor—could you confirm if it’s available and share any guidelines?

[rdimitrov]

@sureshkrishnan-v - Thanks for volunteering! Let's see in case @eikenb already have something prepared there and if not you'll be safe to proceed 👍

As for the guidelines the idea is to make this timeout configurable and not hardcoded to 15s. If it's not set during initialisation, we can safely default to 15 second, but otherwise we should use the value set in the configuration. Does that make sense?

[sureshkrishnan-v]

Ok, the timeout hardcoded to 15s needs to be configurable else it defaults to 15s am I right? and need to know how can I get time from updaterconfig struct will it be set at initialisation time ?

[rdimitrov]

Yes, it should be available through config.UpdaterConfig 👍

But again, I would wait to see in case @eikenb has anything already prepared so we don't duplicate the same work twice 👍

[sureshkrishnan-v]

ok i will wait

[eikenb]

@rdimitrov @sureshkrishnan-v ... Sorry for the delayed response. I don't have time to write anything for this currently. So no blocker on my end to letting @sureshkrishnan-v work on it.

Thanks for the consideration and great to hear it will be addressed.

[rdimitrov]

Thanks for the reply, @eikenb 🙏

@sureshkrishnan-v - I'll assign it to you 👍

[sureshkrishnan-v]

I believe I have completed the task as specified. Can I proceed with a PR? #660

[rdimitrov]

Hey @sureshkrishnan-v, apologies for replying a bit late 👋

We had a discussion around this with the other maintainers and we were talking that we agree on having this option, but it might be better if we handle this in a slightly different approach.

The reasoning is the way #665 implements it now is by extending the affected functions (i.e. DownloadTarget) which will result in a breaking change from a versioning perspective. This is not a problem per see, but If we decide to extend it again in the future with another option you can see how this becomes a bad practice.

The approach we are proposing is leveraging the options pattern (here's a link - https://dev.to/kittipat1413/understanding-the-options-pattern-in-go-390c in case you're not familiar with it).

For example, in the case of DownloadTarget we'll keep it as it is and add a new one, i.e. DownloadTargetWithOpts where we'll implement the options handling as per your implementation.

• This would help us not have to break the API again the next time we need to extend the options.
• It will let us leave the DownloadTarget as it is for now (not breaking existing clients) and also add a message that it is going to be deprecated in one of our upcoming releases giving enough time for clients to upgrade.

Let us know what you think about this approach? In any case we appreciate a lot that you brought this up and your help to fix it 🙏

[jku]

Are you sure the timeout implementation is useful at all?

https://pkg.go.dev/net/http#Client:

	// Timeout specifies a time limit for requests made by this
	// Client. The timeout includes connection time, any
	// redirects, and reading the response body. The timer remains
	// running after Get, Head, Post, or Do return and will
	// interrupt reading of the Response.Body.

This feels completely inappropriate for especially artifact downloads: there is no reasonable upper limit to artifact size so also no reasonable timeout for the whole operation in every possible network environment.

Compare this to the e.g. Python urllib3 timeout: "The maximum amount of time (in seconds) to wait between consecutive read operations for a response from the server" -- a much more reasonable value to set for undefined download sizes but we still set this value to 30 secs by default since VMs in CI systems sometimes just freeze for long periods.

Relying on default system timeouts (IOW not setting a timeout specifically) is IMO a really good choice: In Go I think the relevant ones are in DefaultTransport. Users with special needs can always make their own Fetcher right?

---

I'll add my opinion on endless data attacks (since that is why these timeout shenanigans get added to TUF clients): In my opinion a TUF client library cannot effectively protect against endless data attacks, it is impossible: Default timeouts in client code should be seen as just "best practices" not protections against attacks. A specific client implementation might be able to use timeout settings for some protection but I doubt even that happens in practice.

[udf2457]

In Go I think the relevant ones are in DefaultTransport

Yes.... https://go.dev/src/net/http/transport.go

var DefaultTransport RoundTripper = &Transport{
	Proxy: ProxyFromEnvironment,
	DialContext: defaultTransportDialContext(&net.Dialer{
		Timeout:   30 * time.Second,
		KeepAlive: 30 * time.Second,
	}),
	ForceAttemptHTTP2:     true,
	MaxIdleConns:          100,
	IdleConnTimeout:       90 * time.Second,
	TLSHandshakeTimeout:   10 * time.Second,
	ExpectContinueTimeout: 1 * time.Second,
}

@jku To be fair, perhaps the more interesting discussion would be not timeouts but whether there should be any retry logic in go-tuf since the default Go client does not retry.

Users with special needs can always make their own Fetcher right?

Theoretically yes ....but per #670 the code needs a bit of an overhaul to properly support users bringing their own Fetcher.

updater.go is effectively hard-coded towards DefaultFetcher. Even this hard-coded timeout we are discussing here does not really belong in updater.go, it should be in DefaultFetcher..... 😉

[jku]

Jut making sure I wasn't too wishy washy in my original comment:

• I think this is not just an enhancement but a real bug: go-tuf seems broken for users downloading actually large artifacts
• go-tuf likely should not be using http.Client timeout argument: it does not seem practically useful at all

[malancas]

I'm running into the timeout limit and would be interested in contributing a pull request if this is not being actively worked on. I'm also interested in the possibility of adding retry logic to the Fetcher#DownloadFile method.

Reading through the comments, I see we want to avoid making breaking changes like updating Fetcher#DownloadFile method signature but we probably want to switch to using the http.RoundTripper timeout configuration instead of the http.Client Timeout field.

To avoid breaking the Fetcher interface, would the maintainers be open to allowing users to optionally set a custom http.Client or http.RoundTripper within the DefaultFetcher struct? The DefaultFetcher#DownloadFile method would then either use the custom http.Client or use a http.Client built by the fetcher that uses the custom RoundTripper implementation. So something like:

type DefaultFetcher struct {
   client http.Client
   ...
}

func (d *DefaultFetch) WithHTTPClient(client http.Client) {
	d.client = client
}

or

func (d *DefaultFetch) WithRoundTripper(rt http.RoundTripper) {
        client := http.DefaultClient
        client.Transport = rt
}

The DownloadFile method would check for a custom created http.Client created by either of the above methods. If found, it would use the custom http client for the request. If not found, it would create a default client and set Timeout field on http.Client as it does today.

Another option would be to create a new implementation of Fetcher allows users to configure the HTTP client or transport. Its DownloadFile method would ignore the timeout parameter. Users could provide this as the custom fetcher to the Config object with a new constructor.

[udf2457]

@malancas just from my perspective (speaking not a maintainer, just someone who has previously contributed to go-tuf) ....

I have been busy with $work so have not had the time to work on any go-tuf contributions, and so have certainly not been working on this specific topic.

All I would say is I stand behind my original comment from January, i.e. that this should not really be fixed as a standalone item but rather within the context of fixing the hard-coding and lack of flexibility in the present implementation.

But reading your comment I think we're (roughly) on the same page in that respect ?