[arvados] created: 2.7.0-5456-g03508043dd
git repository hosting
git at public.arvados.org
Thu Nov 30 20:00:14 UTC 2023
at 03508043dde51678389860fcc7db115f83ed530c (commit)
commit 03508043dde51678389860fcc7db115f83ed530c
Author: Tom Clegg <tom at curii.com>
Date: Thu Nov 30 15:00:05 2023 -0500
21217: Don't retry after "invalid outgoing header" error.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/sdk/go/arvados/client.go b/sdk/go/arvados/client.go
index 735a44d24c..eab61075e6 100644
--- a/sdk/go/arvados/client.go
+++ b/sdk/go/arvados/client.go
@@ -238,6 +238,8 @@ var reqIDGen = httpserver.IDGenerator{Prefix: "req-"}
var nopCancelFunc context.CancelFunc = func() {}
+var reqErrorRe = regexp.MustCompile(`net/http: invalid header `)
+
// Do augments (*http.Client)Do(): adds Authorization and X-Request-Id
// headers, delays in order to comply with rate-limiting restrictions,
// and retries failed requests when appropriate.
@@ -274,6 +276,7 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) {
var lastResp *http.Response
var lastRespBody io.ReadCloser
var lastErr error
+ var checkRetryCalled int
rclient := retryablehttp.NewClient()
rclient.HTTPClient = c.httpClient()
@@ -287,11 +290,15 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) {
rclient.RetryMax = 0
}
rclient.CheckRetry = func(ctx context.Context, resp *http.Response, respErr error) (bool, error) {
+ checkRetryCalled++
if c.requestLimiter.Report(resp, respErr) {
c.last503.Store(time.Now())
}
if c.Timeout == 0 {
- return false, err
+ return false, nil
+ }
+ if respErr != nil && reqErrorRe.MatchString(respErr.Error()) {
+ return false, nil
}
retrying, err := retryablehttp.DefaultRetryPolicy(ctx, resp, respErr)
if retrying {
@@ -322,7 +329,14 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) {
}
resp, err := rclient.Do(rreq)
if (errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled)) && (lastResp != nil || lastErr != nil) {
- resp, err = lastResp, lastErr
+ resp = lastResp
+ err = lastErr
+ if checkRetryCalled > 0 && err != nil {
+ // Mimic retryablehttp's "giving up after X
+ // attempts" message, even if we gave up
+ // because of time rather than maxretries.
+ err = fmt.Errorf("%s %s giving up after %d attempt(s): %w", req.Method, req.URL.String(), checkRetryCalled, err)
+ }
if resp != nil {
resp.Body = lastRespBody
}
diff --git a/sdk/go/arvados/client_test.go b/sdk/go/arvados/client_test.go
index a196003b8f..55e2f998c4 100644
--- a/sdk/go/arvados/client_test.go
+++ b/sdk/go/arvados/client_test.go
@@ -341,6 +341,20 @@ func (s *clientRetrySuite) TestNonRetryableError(c *check.C) {
c.Check(s.reqs, check.HasLen, 1)
}
+// as of 0.7.2., retryablehttp does not recognize this as a
+// non-retryable error.
+func (s *clientRetrySuite) TestNonRetryableStdlibError(c *check.C) {
+ s.respStatus <- http.StatusOK
+ req, err := http.NewRequest(http.MethodGet, "https://"+s.client.APIHost+"/test", nil)
+ c.Assert(err, check.IsNil)
+ req.Header.Set("Good-Header", "T\033rrible header value")
+ err = s.client.DoAndDecode(&struct{}{}, req)
+ c.Check(err, check.ErrorMatches, `.*after 1 attempt.*net/http: invalid header .*`)
+ if !c.Check(s.reqs, check.HasLen, 0) {
+ c.Logf("%v", s.reqs[0])
+ }
+}
+
func (s *clientRetrySuite) TestNonRetryableAfter503s(c *check.C) {
time.AfterFunc(time.Second, func() { s.respStatus <- http.StatusNotFound })
err := s.client.RequestAndDecode(&struct{}{}, http.MethodGet, "test", nil, nil)
commit fb1f664fd1d11ef18cb00bed2ae4b49ce346c477
Author: Tom Clegg <tom at curii.com>
Date: Thu Nov 30 09:58:04 2023 -0500
21217: Strip leading/trailing space chars from incoming tokens.
Depending on OS/browser, trailing spaces and newlines are easy to
include inadvertently when copying, and impossible to detect when
pasting into a password input that displays placeholder symbols
instead of the pasted text.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/sdk/go/auth/auth.go b/sdk/go/auth/auth.go
index f1c2e243b5..da9b4ea5b8 100644
--- a/sdk/go/auth/auth.go
+++ b/sdk/go/auth/auth.go
@@ -54,13 +54,13 @@ func (a *Credentials) LoadTokensFromHTTPRequest(r *http.Request) {
// Load plain token from "Authorization: OAuth2 ..." header
// (typically used by smart API clients)
if toks := strings.SplitN(r.Header.Get("Authorization"), " ", 2); len(toks) == 2 && (toks[0] == "OAuth2" || toks[0] == "Bearer") {
- a.Tokens = append(a.Tokens, toks[1])
+ a.Tokens = append(a.Tokens, strings.TrimSpace(toks[1]))
}
// Load base64-encoded token from "Authorization: Basic ..."
// header (typically used by git via credential helper)
if _, password, ok := r.BasicAuth(); ok {
- a.Tokens = append(a.Tokens, password)
+ a.Tokens = append(a.Tokens, strings.TrimSpace(password))
}
// Load tokens from query string. It's generally not a good
@@ -76,7 +76,9 @@ func (a *Credentials) LoadTokensFromHTTPRequest(r *http.Request) {
// find/report decoding errors in a suitable way.
qvalues, _ := url.ParseQuery(r.URL.RawQuery)
if val, ok := qvalues["api_token"]; ok {
- a.Tokens = append(a.Tokens, val...)
+ for _, token := range val {
+ a.Tokens = append(a.Tokens, strings.TrimSpace(token))
+ }
}
a.loadTokenFromCookie(r)
@@ -94,7 +96,7 @@ func (a *Credentials) loadTokenFromCookie(r *http.Request) {
if err != nil {
return
}
- a.Tokens = append(a.Tokens, string(token))
+ a.Tokens = append(a.Tokens, strings.TrimSpace(string(token)))
}
// LoadTokensFromHTTPRequestBody loads credentials from the request
@@ -111,7 +113,7 @@ func (a *Credentials) LoadTokensFromHTTPRequestBody(r *http.Request) error {
return err
}
if t := r.PostFormValue("api_token"); t != "" {
- a.Tokens = append(a.Tokens, t)
+ a.Tokens = append(a.Tokens, strings.TrimSpace(t))
}
return nil
}
diff --git a/sdk/go/auth/handlers_test.go b/sdk/go/auth/handlers_test.go
index 362aeb7f04..85ea8893a5 100644
--- a/sdk/go/auth/handlers_test.go
+++ b/sdk/go/auth/handlers_test.go
@@ -7,6 +7,8 @@ package auth
import (
"net/http"
"net/http/httptest"
+ "net/url"
+ "strings"
"testing"
check "gopkg.in/check.v1"
@@ -32,9 +34,36 @@ func (s *HandlersSuite) SetUpTest(c *check.C) {
func (s *HandlersSuite) TestLoadToken(c *check.C) {
handler := LoadToken(s)
handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/foo/bar?api_token=xyzzy", nil))
- c.Assert(s.gotCredentials, check.NotNil)
- c.Assert(s.gotCredentials.Tokens, check.HasLen, 1)
- c.Check(s.gotCredentials.Tokens[0], check.Equals, "xyzzy")
+ c.Check(s.gotCredentials.Tokens, check.DeepEquals, []string{"xyzzy"})
+}
+
+// Ignore leading and trailing spaces, newlines, etc. in case a user
+// has added them inadvertently during copy/paste.
+func (s *HandlersSuite) TestTrimSpaceInQuery(c *check.C) {
+ handler := LoadToken(s)
+ handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/foo/bar?api_token=%20xyzzy%0a", nil))
+ c.Check(s.gotCredentials.Tokens, check.DeepEquals, []string{"xyzzy"})
+}
+func (s *HandlersSuite) TestTrimSpaceInPostForm(c *check.C) {
+ handler := LoadToken(s)
+ req := httptest.NewRequest("POST", "/foo/bar", strings.NewReader(url.Values{"api_token": []string{"\nxyzzy\n"}}.Encode()))
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ handler.ServeHTTP(httptest.NewRecorder(), req)
+ c.Check(s.gotCredentials.Tokens, check.DeepEquals, []string{"xyzzy"})
+}
+func (s *HandlersSuite) TestTrimSpaceInCookie(c *check.C) {
+ handler := LoadToken(s)
+ req := httptest.NewRequest("GET", "/foo/bar", nil)
+ req.AddCookie(&http.Cookie{Name: "arvados_api_token", Value: EncodeTokenCookie([]byte("\vxyzzy\n"))})
+ handler.ServeHTTP(httptest.NewRecorder(), req)
+ c.Check(s.gotCredentials.Tokens, check.DeepEquals, []string{"xyzzy"})
+}
+func (s *HandlersSuite) TestTrimSpaceInBasicAuth(c *check.C) {
+ handler := LoadToken(s)
+ req := httptest.NewRequest("GET", "/foo/bar", nil)
+ req.SetBasicAuth("username", "\txyzzy\n")
+ handler.ServeHTTP(httptest.NewRecorder(), req)
+ c.Check(s.gotCredentials.Tokens, check.DeepEquals, []string{"xyzzy"})
}
func (s *HandlersSuite) TestRequireLiteralTokenEmpty(c *check.C) {
@@ -76,4 +105,5 @@ func (s *HandlersSuite) TestRequireLiteralToken(c *check.C) {
func (s *HandlersSuite) ServeHTTP(w http.ResponseWriter, r *http.Request) {
s.served++
s.gotCredentials = CredentialsFromRequest(r)
+ s.gotCredentials.LoadTokensFromHTTPRequestBody(r)
}
diff --git a/services/keep-web/handler_test.go b/services/keep-web/handler_test.go
index eba682149a..c14789889d 100644
--- a/services/keep-web/handler_test.go
+++ b/services/keep-web/handler_test.go
@@ -351,6 +351,27 @@ func authzViaCookieValue(r *http.Request, tok string) int {
return http.StatusUnauthorized
}
+func (s *IntegrationSuite) TestVhostViaHTTPBasicAuth(c *check.C) {
+ s.doVhostRequests(c, authzViaHTTPBasicAuth)
+}
+func authzViaHTTPBasicAuth(r *http.Request, tok string) int {
+ r.AddCookie(&http.Cookie{
+ Name: "arvados_api_token",
+ Value: auth.EncodeTokenCookie([]byte(tok)),
+ })
+ return http.StatusUnauthorized
+}
+
+func (s *IntegrationSuite) TestVhostViaHTTPBasicAuthWithExtraSpaceChars(c *check.C) {
+ s.doVhostRequests(c, func(r *http.Request, tok string) int {
+ r.AddCookie(&http.Cookie{
+ Name: "arvados_api_token",
+ Value: auth.EncodeTokenCookie([]byte(" " + tok + "\n")),
+ })
+ return http.StatusUnauthorized
+ })
+}
+
func (s *IntegrationSuite) TestVhostViaPath(c *check.C) {
s.doVhostRequests(c, authzViaPath)
}
commit 1a61611cbb8ed4471eb2e04e60bad548bff94678
Author: Tom Clegg <tom at curii.com>
Date: Thu Nov 30 09:57:21 2023 -0500
21217: Fix "Authorization: OAuth2 ..." test to actually test that.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/keep-web/handler_test.go b/services/keep-web/handler_test.go
index 5e81f3a01e..eba682149a 100644
--- a/services/keep-web/handler_test.go
+++ b/services/keep-web/handler_test.go
@@ -328,9 +328,10 @@ func (s *IntegrationSuite) TestVhostViaAuthzHeaderOAuth2(c *check.C) {
s.doVhostRequests(c, authzViaAuthzHeaderOAuth2)
}
func authzViaAuthzHeaderOAuth2(r *http.Request, tok string) int {
- r.Header.Add("Authorization", "Bearer "+tok)
+ r.Header.Add("Authorization", "OAuth2 "+tok)
return http.StatusUnauthorized
}
+
func (s *IntegrationSuite) TestVhostViaAuthzHeaderBearer(c *check.C) {
s.doVhostRequests(c, authzViaAuthzHeaderBearer)
}
commit 67c187df5e682807f495c0fa61e77a022f24a284
Author: Tom Clegg <tom at curii.com>
Date: Wed Nov 29 09:30:44 2023 -0500
21217: Fix double slash in API url.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/keep-web/cache.go b/services/keep-web/cache.go
index 604efd29d9..df5705ed32 100644
--- a/services/keep-web/cache.go
+++ b/services/keep-web/cache.go
@@ -221,7 +221,7 @@ func (c *cache) GetSession(token string) (arvados.CustomFileSystem, *cachedSessi
// using the new fs).
sess.inuse.Lock()
if !sess.userLoaded || refresh {
- err := sess.client.RequestAndDecode(&sess.user, "GET", "/arvados/v1/users/current", nil, nil)
+ err := sess.client.RequestAndDecode(&sess.user, "GET", "arvados/v1/users/current", nil, nil)
if he := errorWithHTTPStatus(nil); errors.As(err, &he) && he.HTTPStatus() == http.StatusForbidden {
// token is OK, but "get user id" api is out
// of scope -- use existing/expired info if
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list