Bug #21907
closed
Uncached permission lookup in keep-web when handling s3 request
Added by Tom Clegg 10 months ago.
Updated 9 months ago.
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
}
- Related to Bug #21748: awscli downloads from keep-web slowly added
- Target version set to Development 2024-07-03 sprint
- Category changed from API to Keep
- Assigned To set to Brett Smith
- Target version changed from Development 2024-07-03 sprint to Development 2024-07-24 sprint
Some implementation thoughts/hints
- 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 entire APIClientAuthorization object. But that's the only real difference from the proposed plan. Hope that's okay.
- Anything not implemented (discovered or discussed during work) has a follow-up story.
- 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).
- 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, caching the entire APIClientAuthorization object LGTM
- checks3signature() needs to acquire
h.s3secretCacheMtx.Lock() before doing the h.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 to s3SecretCacheTidyInterval?
- it looks like
checks3signature() doesn't write to aca, so it would be safe to make aca 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.
- Status changed from In Progress to Resolved
Also available in: Atom
PDF
Updated by 
Updated by 
Updated by