Actions
Bug #21907
closedUncached permission lookup in keep-web when handling s3 request
Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Target version:
Story points:
-
Release:
Release relationship:
Auto
Description
In arvados:source:services/keep-web/s3.go
if len(key) == 27 && key[5:12] == "-gj3su-" {
// Access key is the UUID of an Arvados token, secret
// key is the secret part.
ctx := arvados.ContextWithAuthorization(r.Context(), "Bearer "+h.Cluster.SystemRootToken)
err = client.RequestAndDecodeContext(ctx, &aca, "GET", "arvados/v1/api_client_authorizations/"+key, nil, nil)
secret = aca.APIToken
} else {
// Access key and secret key are both an entire
// Arvados token or OIDC access token.
ctx := arvados.ContextWithAuthorization(r.Context(), "Bearer "+unescapeKey(key))
err = client.RequestAndDecodeContext(ctx, &aca, "GET", "arvados/v1/api_client_authorizations/current", nil, nil)
secret = key
}
Updated by Tom Clegg 7 months ago
- Related to Bug #21748: awscli downloads from keep-web slowly added
Updated by Peter Amstutz 7 months ago
- Target version set to Development 2024-07-03 sprint
Updated by Peter Amstutz 7 months ago
- Target version changed from Development 2024-07-03 sprint to Development 2024-07-24 sprint
Updated by Tom Clegg 6 months ago
Some implementation thoughts/hints
- Pretty similar to #21901
- Define a
cachedS3Secret
structtype cachedS3Secret struct { secret string expiry time.Time }
- Add a
map[string]cachedS3Secret
tohandler
- Expiry time should be earlier of
aca.ExpiresAt
(assuming!aca.ExpiresAt.IsZero()
) andtime.Now().Add(h.Cluster.Collections.WebDAVCache.TTL.Duration())
Updated by Brett Smith 6 months ago
- Status changed from New to In Progress
21907-cache-s3-token @ 54d80de3f87fde4bfb83c3e1ad8161b595e6cf3c - developer-run-tests: #4350
- All agreed upon points are implemented / addressed.
- Yes. Because the current
checks3signature
method needs both the authorization UUID and token, as well as the ability to normalize it into a v2 token, it seemed most straightforward to cache the entireAPIClientAuthorization
object. But that's the only real difference from the proposed plan. Hope that's okay.
- Yes. Because the current
- Anything not implemented (discovered or discussed during work) has a follow-up story.
- N/A
- Code is tested and passing, both automated and manual, what manual testing was done is described
- See above. The branch includes a bunch of new tests.
- Documentation has been updated.
- N/A, pure performance improvement.
- Behaves appropriately at the intended scale (describe intended scale).
- No change in scale.
- Considered backwards and forwards compatibility issues between client and server.
- No compatibility change. Note there are zero changes to existing tests.
- Follows our coding standards and GUI style guidelines.
- Yes
Updated by Tom Clegg 6 months ago
- yes, caching the entire APIClientAuthorization object LGTM
- checks3signature() needs to acquire
h.s3secretCacheMtx.Lock()
before doing theh.s3SecretCache[unescapedKey]
lookup - NewCachedS3Secret should be unexported (i.e., newCachedS3Secret)
- less critical: IsValidAt should be isValidAt (since the struct itself isn't exported the method is effectively private anyway, but in general methods like this can end up being called by other modules by virtue of satisfying an interface, so if that's not intended, I'd default to an unexported name)
- maybe rename
s3SecretCacheTidyDuration
tos3SecretCacheTidyInterval
? - it looks like
checks3signature()
doesn't write toaca
, so it would be safe to makeaca
a pointer instead of a value, and avoid the extra copy when using a cached auth. That's a pretty tiny optimization, though, so leaving it alone would also be fine with me.
Updated by Brett Smith 6 months ago
I had to rebase because I noticed the commit message had the wrong ticket number. 9e21b7cdd3e51afd02acdb8b2989dbe949da2680 is identical to what you reviewed before except for that edit to the commit message.
Everything you suggested sounds good to me. Made those changes and now at 24f9b12ff94f2725e4b797f92738ebd2e2b96852 - developer-run-tests: #4351
Updated by Brett Smith 6 months ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|e3c39d18dcbb61ece0889838acc54cb81f50b889.
Actions