Bug #17598
closed
[keep-web] should be tolerant of superfluous :443's in the arvados config
Description
With a config like this:
WebDAVDownload:
ExternalURL: https://download.xxxx1.arvadosapi.com:443
keep-web does a literal comparison of ExternalURL.Host with a http.Request.Host in `services/keep-web/handler.go` line 251, and the superfluous :443 in the config trips that up, which causes it to go into "turtle mode" and fall back to the anonymous token. In the normal (i.e. no anonymous access) case, that breaks all downloads with a 401, for example it makes viewing collection contents in WB2 fail.
keep-web should handle the special cases of :443 and :80 correctly. (also :https and :http)
two other lessons learned that need to be fixed:
- if AnonymousUserToken is empty, don't add it to 'tokens' in handler.go:349 (this is #17597)
- if a token was provided but not used (credentialsOK is false on line 295), it should log a warning saying that it is refusing to accept credentials, give some indication why, and (if applicable) indicate it will fall back to trying AnonymousUserToken
Updated by Ward Vandewege over 3 years ago
- Status changed from New to In Progress
Updated by Ward Vandewege over 3 years ago
- Status changed from In Progress to New
- Subject changed from [keep-web to [keep-web] should be tolerant of superfluous :443's in the arvados config
Updated by Peter Amstutz over 3 years ago
- Assigned To set to Peter Amstutz
- Status changed from New to In Progress
Updated by Peter Amstutz over 3 years ago
17598-keep-web-url @ arvados|d48cff711af8255f3b2b69506b54a283c1aab776
- Remove :80 and :443 before doing comparison. This does mean it will treat :80 and :443 as the same vhost. This seems fine.
- Handle the case of "no anonymous user token" better
- Improve error response when an access token is being deliberately ignored by keep-web
- Update tests.
Updated by Tom Clegg over 3 years ago
- should be unexported stripDefaultPort (even though this is package main)
- will do weird things if host is like
[::1]:443... there is some code in net/url we could make use of here, likeu := &url.URL{Host: host} if p := u.Port(); p == "80" || p == "443" { return u.Hostname() } else { return host }
The vhost explanation says "...did not match either Services.WebDAV or Services.WebDAVDownload" which (I think) will be confusing when it does in fact match Services.WebDAV url but doesn't contain a collection ID (cf parseCollectionIDFromDNSName).
Maybe this?
Authorization tokens are not accepted here: vhost %q does not specify a single collection ID or match Services.WebDAVDownload.ExternalURL %q, and Collections.TrustAllContent is false
Updated by Peter Amstutz over 3 years ago
Tom Clegg wrote:
StripDefaultPort()
- should be unexported stripDefaultPort (even though this is package main)
Fixed.
- will do weird things if host is like
[::1]:443... there is some code in net/url we could make use of here, like [...]
Oh, I completely forgot about IPv6 addresses. Fixed.
The vhost explanation says "...did not match either Services.WebDAV or Services.WebDAVDownload" which (I think) will be confusing when it does in fact match Services.WebDAV url but doesn't contain a collection ID (cf parseCollectionIDFromDNSName).
Maybe this?
Authorization tokens are not accepted here: vhost %q does not specify a single collection ID or match Services.WebDAVDownload.ExternalURL %q, and Collections.TrustAllContent is false
Updated message.
17598-keep-web-url @ arvados|c736087b86da2353965e5722a38cff5e7d891fd2
Updated by Peter Amstutz over 3 years ago
- Status changed from In Progress to Resolved
Updated by Peter Amstutz over 3 years ago
- Status changed from Resolved to Feedback
Needs to also account for case because domain names are not supposed to be case sensitive
Updated by Peter Amstutz over 3 years ago
17598-keep-web-url-case @ b77b80821a010b52ee0b74ee8cc82d1ba603027a
Updated by Peter Amstutz over 3 years ago
- Status changed from Feedback to Resolved