Bug #17598

[keep-web] should be tolerant of superfluous :443's in the arvados config

Added by Ward Vandewege 14 days ago. Updated 9 days ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
04/29/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

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:

  1. if AnonymousUserToken is empty, don't add it to 'tokens' in handler.go:349 (this is #17597)
  2. 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

Subtasks

Task #17599: Review 17598-keep-web-urlResolvedPeter Amstutz

Associated revisions

Revision 2cced4f5
Added by Peter Amstutz 13 days ago

Merge branch '17598-keep-web-url' refs #17598

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 921047b7
Added by Peter Amstutz 9 days ago

Merge branch '17598-keep-web-url-case' refs #17598

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

History

#1 Updated by Ward Vandewege 14 days ago

  • Status changed from New to In Progress

#2 Updated by Ward Vandewege 14 days 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

#3 Updated by Ward Vandewege 14 days ago

  • Description updated (diff)

#4 Updated by Peter Amstutz 14 days ago

  • Description updated (diff)

#5 Updated by Peter Amstutz 14 days ago

  • Description updated (diff)

#6 Updated by Tom Clegg 14 days ago

  • Description updated (diff)

#7 Updated by Peter Amstutz 14 days ago

  • Assigned To set to Peter Amstutz
  • Status changed from New to In Progress

#8 Updated by Peter Amstutz 13 days 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.

https://ci.arvados.org/view/Developer/job/developer-run-tests/2443/

#9 Updated by Tom Clegg 13 days ago

StripDefaultPort()
  • 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, like
    u := &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

#10 Updated by Peter Amstutz 13 days 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

https://ci.arvados.org/view/Developer/job/developer-run-tests/2444/

#11 Updated by Tom Clegg 13 days ago

LGTM, thanks!

#12 Updated by Peter Amstutz 13 days ago

  • Status changed from In Progress to Resolved

#13 Updated by Peter Amstutz 10 days ago

  • Status changed from Resolved to Feedback

Needs to also account for case because domain names are not supposed to be case sensitive

#15 Updated by Tom Clegg 9 days ago

LGTM, thanks!

#16 Updated by Peter Amstutz 9 days ago

  • Status changed from Feedback to Resolved

Also available in: Atom PDF