Hardcoded timeout in updater.DownloadTarget

[eikenb]

go-tuf/metadata/updater/updater.go

Line 248 in 990cdb8

data, err := update.cfg.Fetcher.DownloadFile(fullURL, targetFile.Length, time.Second*15)

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..... 😉