Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/api/internal/handlers/admin_api_keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (a *APIStore) DeleteAdminTeamsTeamIDApiKeysApiKeyID(c *gin.Context, teamID
return
}

deleted, err := team.DeleteAPIKey(ctx, a.authDB, teamID, apiKeyUUID)
deleted, err := team.DeleteAPIKey(ctx, a.authDB, a.authService, teamID, apiKeyUUID)
if err != nil {
a.sendAPIStoreError(c, http.StatusInternalServerError, fmt.Sprintf("Error when deleting API key: %s", err))

Expand Down
25 changes: 22 additions & 3 deletions packages/api/internal/handlers/admin_api_keys_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
authtypes "github.com/e2b-dev/infra/packages/auth/pkg/types"
authqueries "github.com/e2b-dev/infra/packages/db/pkg/auth/queries"
"github.com/e2b-dev/infra/packages/db/pkg/testutils"
sharedkeys "github.com/e2b-dev/infra/packages/shared/pkg/keys"
)

func TestPostAdminTeamsTeamIDApiKeysCreatesTeamKey(t *testing.T) {
Expand Down Expand Up @@ -195,9 +196,13 @@ func TestDeleteAdminTeamsTeamIDApiKeysDeletesTeamKey(t *testing.T) {
createCtx.Request = httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/admin/teams/"+teamID.String()+"/api-keys", strings.NewReader(`{"name":"Admin integration"}`))
createCtx.Request.Header.Set("Content-Type", "application/json")

var invalidatedHashes []string
store := &APIStore{
authDB: testDB.AuthDB,
authService: fakeAPIKeyAuthService{team: &authtypes.Team{Team: &authqueries.Team{ID: teamID}}},
authDB: testDB.AuthDB,
authService: fakeAPIKeyAuthService{
team: &authtypes.Team{Team: &authqueries.Team{ID: teamID}},
invalidatedAPIKeyHashes: &invalidatedHashes,
},
}
store.PostAdminTeamsTeamIDApiKeys(createCtx, teamID)
if createRecorder.Code != http.StatusCreated {
Expand Down Expand Up @@ -225,6 +230,12 @@ func TestDeleteAdminTeamsTeamIDApiKeysDeletesTeamKey(t *testing.T) {
if len(keys) != 0 {
t.Fatalf("expected API key to be deleted, got %d keys", len(keys))
}

deletedKeyHash, err := sharedkeys.VerifyKey(sharedkeys.ApiKeyPrefix, created.Key)
if err != nil {
t.Fatalf("failed to hash created API key: %v", err)
}
require.Equal(t, []string{deletedKeyHash}, invalidatedHashes, "expected the deleted key's auth cache entry to be invalidated")
}

func TestDeleteAdminTeamsTeamIDApiKeysRejectsMissingKey(t *testing.T) {
Expand All @@ -233,7 +244,7 @@ func TestDeleteAdminTeamsTeamIDApiKeysRejectsMissingKey(t *testing.T) {
testDB := testutils.SetupDatabase(t)
teamID := testutils.CreateTestTeam(t, testDB)

store := &APIStore{authDB: testDB.AuthDB}
store := &APIStore{authDB: testDB.AuthDB, authService: fakeAPIKeyAuthService{}}
recorder := httptest.NewRecorder()
ctx, _ := gin.CreateTestContext(recorder)
missingKeyID := uuid.New()
Expand All @@ -249,6 +260,8 @@ func TestDeleteAdminTeamsTeamIDApiKeysRejectsMissingKey(t *testing.T) {
type fakeAPIKeyAuthService struct {
team *authtypes.Team
err error

invalidatedAPIKeyHashes *[]string
}

func (f fakeAPIKeyAuthService) ValidateAPIKey(context.Context, *gin.Context, string) (*authtypes.Team, *sharedauth.APIError) {
Expand Down Expand Up @@ -277,6 +290,12 @@ func (f fakeAPIKeyAuthService) GetTeamByID(context.Context, uuid.UUID) (*authtyp

func (f fakeAPIKeyAuthService) InvalidateTeamMemberCache(context.Context, uuid.UUID, string) {}

func (f fakeAPIKeyAuthService) InvalidateAPIKeyCache(_ context.Context, hashedKey string) {
if f.invalidatedAPIKeyHashes != nil {
*f.invalidatedAPIKeyHashes = append(*f.invalidatedAPIKeyHashes, hashedKey)
}
}

func (f fakeAPIKeyAuthService) InvalidateTeamCache(context.Context, uuid.UUID) error {
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion packages/api/internal/handlers/apikey.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func (a *APIStore) DeleteApiKeysApiKeyID(c *gin.Context, apiKeyID string) {

teamID := auth.MustGetTeamID(c)

deleted, err := team.DeleteAPIKey(ctx, a.authDB, teamID, apiKeyIDParsed)
deleted, err := team.DeleteAPIKey(ctx, a.authDB, a.authService, teamID, apiKeyIDParsed)
if err != nil {
a.sendAPIStoreError(c, http.StatusInternalServerError, fmt.Sprintf("Error when deleting API key: %s", err))

Expand Down
13 changes: 10 additions & 3 deletions packages/api/internal/team/apikeys.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

"github.com/google/uuid"

sharedauth "github.com/e2b-dev/infra/packages/auth/pkg/auth"
"github.com/e2b-dev/infra/packages/db/pkg/auth"
"github.com/e2b-dev/infra/packages/db/pkg/auth/queries"
"github.com/e2b-dev/infra/packages/shared/pkg/keys"
Expand Down Expand Up @@ -48,8 +49,8 @@ func CreateAPIKey(ctx context.Context, authDB *authdb.Client, teamID uuid.UUID,
}, nil
}

func DeleteAPIKey(ctx context.Context, authDB *authdb.Client, teamID uuid.UUID, apiKeyID uuid.UUID) (bool, error) {
ids, err := authDB.Write.DeleteTeamAPIKey(ctx, authqueries.DeleteTeamAPIKeyParams{
func DeleteAPIKey(ctx context.Context, authDB *authdb.Client, authService sharedauth.Service, teamID uuid.UUID, apiKeyID uuid.UUID) (bool, error) {
hashes, err := authDB.Write.DeleteTeamAPIKey(ctx, authqueries.DeleteTeamAPIKeyParams{
ID: apiKeyID,
TeamID: teamID,
})
Expand All @@ -59,5 +60,11 @@ func DeleteAPIKey(ctx context.Context, authDB *authdb.Client, teamID uuid.UUID,
return false, fmt.Errorf("error when deleting API key: %w", err)
}

return len(ids) > 0, nil
// Invalidate the auth cache so the deleted key stops authenticating
// immediately instead of after the cache TTL expires.
for _, hash := range hashes {
authService.InvalidateAPIKeyCache(ctx, hash)
Comment thread
tvi marked this conversation as resolved.
Comment thread
jakubno marked this conversation as resolved.
}
Comment thread
tvi marked this conversation as resolved.

return len(hashes) > 0, nil
}
7 changes: 7 additions & 0 deletions packages/auth/internal/service/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@ const (
authInfoExpiration = 5 * time.Minute
refreshInterval = 1 * time.Minute
refreshTimeout = 30 * time.Second
// invalidateTimeout must exceed the cache's write-lock wait so an
// invalidation can outwait any in-flight cache writer instead of degrading
// to a best-effort delete that a stale write could overwrite. The wait is
// bounded by the lock TTL (refreshTimeout + 2x the 2s default Redis
// timeout = 34s) plus the 5s lock-acquire margin; 45s adds headroom for
// the DEL itself.
invalidateTimeout = 45 * time.Second

authCacheRedisPrefix = "auth:team"
)
Expand Down
20 changes: 20 additions & 0 deletions packages/auth/internal/service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ type Service interface {
GetTeamByID(ctx context.Context, teamID uuid.UUID) (*types.Team, error)
InvalidateTeamMemberCache(ctx context.Context, userID uuid.UUID, teamID string)
InvalidateTeamCache(ctx context.Context, teamID uuid.UUID) error
InvalidateAPIKeyCache(ctx context.Context, hashedKey string)
Close(ctx context.Context) error
}

Expand Down Expand Up @@ -277,6 +278,25 @@ func (s *AuthService) InvalidateTeamCache(ctx context.Context, teamID uuid.UUID)
return nil
}

// InvalidateAPIKeyCache removes the cached auth entry for a specific hashed API key.
// This should be called when the key is deleted so revocation takes effect immediately
// instead of after the cache TTL expires.
//
// The call is synchronous and waits for any in-flight cache writer on the key
// (see RedisCache.Delete), so the caller's request can block for up to
// invalidateTimeout in the worst case — only reached when a concurrent
// refresh of the same key is wedged near the full refresh timeout, which
// requires a multi-second DB stall; the typical case returns in milliseconds.
func (s *AuthService) InvalidateAPIKeyCache(ctx context.Context, hashedKey string) {
// The invalidation runs after the key's DB delete has committed; if it were
// skipped because the client disconnected, the revoked key would keep
// authenticating until the cache TTL expires.
ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), invalidateTimeout)
defer cancel()

s.teamCache.Invalidate(ctx, hashedKey)
Comment thread
jakubno marked this conversation as resolved.
}

func teamMemberCacheKey(userID uuid.UUID, teamID string) string {
return fmt.Sprintf("%s-%s", userID.String(), strings.ToLower(teamID))
}
Expand Down
6 changes: 5 additions & 1 deletion packages/auth/internal/service/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,11 @@ func (s *authStoreImpl) GetTeamByHashedAPIKey(ctx context.Context, hashedKey str
ctx, span := tracer.Start(ctx, "get team auth")
defer span.End()

result, err := s.authDB.Read.GetTeamWithTierByAPIKey(ctx, hashedKey)
// Deleting an API key invalidates its cache entry; reading through the
// read replica here races replication lag and could re-cache a
// just-deleted key for the full cache TTL, so key revocation must be
// read-after-write safe.
result, err := s.authDB.Write.GetTeamWithTierByAPIKey(ctx, hashedKey)
Comment thread
jakubno marked this conversation as resolved.
if err != nil {
return nil, fmt.Errorf("failed to get team from API key: %w", err)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -847,6 +847,8 @@ func (noopAuthService) GetTeamByID(context.Context, uuid.UUID) (*authtypes.Team,

func (noopAuthService) InvalidateTeamMemberCache(context.Context, uuid.UUID, string) {}

func (noopAuthService) InvalidateAPIKeyCache(context.Context, string) {}

func (noopAuthService) InvalidateTeamCache(context.Context, uuid.UUID) error {
return nil
}
Expand Down
12 changes: 6 additions & 6 deletions packages/db/pkg/auth/queries/delete_team_api_key.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
-- name: DeleteTeamAPIKey :many
DELETE FROM "public"."team_api_keys"
WHERE id = @id AND team_id = @team_id
RETURNING id;
RETURNING api_key_hash;
38 changes: 31 additions & 7 deletions packages/shared/pkg/cache/redis.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func (rc *RedisCache[V]) GetOrSet(ctx context.Context, key string, dataCallback
ctx := context.WithoutCancel(ctx)

// Acquire distributed lock if enabled
lock, lockErr := rc.acquireLock(ctx, key, redislock.LinearBackoff(rc.config.LockRetryInterval))
lock, lockErr := rc.acquireLock(ctx, key, redislock.LinearBackoff(rc.config.LockRetryInterval), acquireLockTimeout)
defer rc.releaseLock(ctx, lock, key)
// We want to get the results even without the lock to prevent failing all the waiting requests

Expand Down Expand Up @@ -163,10 +163,33 @@ func (rc *RedisCache[V]) Set(ctx context.Context, key string, value V) {
}

// Delete removes a value from Redis.
//
// Writers (the GetOrSet backfill and the background refresh) hold the per-key
// lock across their SET, so Delete waits for that lock to guarantee the DEL
// is ordered after any in-flight write; without this, a writer that read the
// backing store just before the caller's mutation could repopulate the entry
// with stale data for a full TTL after a fire-and-forget delete.
//
// The guarantee comes with constraints the caller must respect:
// - The wait is bounded by LockTTL (RefreshTimeout + 2*RedisTimeout) plus a
// margin, and by ctx — pass a context that survives at least that long
// (and detached from request cancellation when the delete must not be
// skipped), otherwise Delete degrades to the best-effort behavior below.
// - If the lock still cannot be obtained (Redis/lock-service errors, or the
// ctx expiring), Delete falls back to a best-effort DEL and a concurrent
// writer may repopulate the entry with stale data until its TTL expires.
// - Healthy writers always finish inside the wait window: their data
// callback is capped at RefreshTimeout, which is strictly less than the
// lock TTL the wait is derived from.
func (rc *RedisCache[V]) Delete(ctx context.Context, key string) {
lock, err := rc.acquireLock(ctx, key, redislock.LinearBackoff(rc.config.LockRetryInterval))
// Wait past a wedged writer's lock auto-expiry (LockTTL) so lock
// acquisition can only fail on Redis/lock-service errors or ctx expiry,
// not on writer contention.
lock, err := rc.acquireLock(ctx, key, redislock.LinearBackoff(rc.config.LockRetryInterval), rc.config.LockTTL+acquireLockTimeout)
if err != nil {
logger.L().Warn(ctx, "RedisCache - Delete: failed to acquire lock", zap.String("key", key))
logger.L().Warn(ctx, "RedisCache - Delete: failed to acquire lock, deleting best-effort; a concurrent writer may repopulate stale data",
zap.String("key", key),
zap.Error(err))
// Continue without the lock to remove the stale data
// In that case it's just a best effort, the data may get repopulated with stale data
}
Expand Down Expand Up @@ -299,7 +322,7 @@ func (rc *RedisCache[V]) getFromRedis(ctx context.Context, key string) (V, time.
func (rc *RedisCache[V]) refreshRedis(ctx context.Context, key string, dataCallback DataCallback[V]) {
rc.redisRefresh.Do(key, func() (any, error) {
// Acquire lock without retry — if another instance is refreshing, skip.
lock, lockErr := rc.acquireLock(ctx, key, redislock.NoRetry())
lock, lockErr := rc.acquireLock(ctx, key, redislock.NoRetry(), acquireLockTimeout)
if errors.Is(lockErr, redislock.ErrNotObtained) {
logger.L().Debug(ctx, "RedisCache: skipping refresh, lock held by another instance",
zap.String("key", key))
Expand Down Expand Up @@ -350,10 +373,11 @@ func (rc *RedisCache[V]) releaseLock(ctx context.Context, lock redis_utils.Lock,
}
}

// acquireLock attempts to acquire a distributed lock for the given key.
// acquireLock attempts to acquire a distributed lock for the given key,
// retrying per the strategy until maxWait or ctx expires, whichever is first.
// Always returns a non-nil Lock (NoopLock on failure) so callers can defer Release unconditionally.
func (rc *RedisCache[V]) acquireLock(ctx context.Context, key string, retry redislock.RetryStrategy) (redis_utils.Lock, error) {
ctx, cancel := context.WithTimeout(ctx, acquireLockTimeout)
func (rc *RedisCache[V]) acquireLock(ctx context.Context, key string, retry redislock.RetryStrategy, maxWait time.Duration) (redis_utils.Lock, error) {
ctx, cancel := context.WithTimeout(ctx, maxWait)
defer cancel()

lockKey := redis_utils.GetLockKey(rc.RedisKey(key))
Expand Down
47 changes: 47 additions & 0 deletions packages/shared/pkg/cache/redis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,53 @@ func TestRedisCache_Delete(t *testing.T) {
assert.ErrorIs(t, err, redis.Nil)
}

// A writer (GetOrSet backfill) holds the per-key lock across its SET. Delete
// must wait for that lock — even past the 5s acquire timeout used by writers —
// so the DEL is ordered after the in-flight write and stale data cannot be
// repopulated for a full TTL (e.g. a revoked API key resurrected into the auth
// cache).
func TestRedisCache_DeleteWaitsForInflightWriter(t *testing.T) {
t.Parallel()
redisClient := redis_utils.SetupInstance(t)
rc := newTestRedisCache(t, redisClient)
defer rc.Close(t.Context())

key := "key1"
callbackStarted := make(chan struct{})
writerDone := make(chan struct{})

var eg errgroup.Group
eg.Go(func() error {
defer close(writerDone)
_, err := rc.GetOrSet(t.Context(), key, func(_ context.Context, _ string) (testValue, error) {
close(callbackStarted)
// Hold the write lock longer than the acquire timeout writers use,
// simulating a callback stalled on a slow backing store.
time.Sleep(acquireLockTimeout + time.Second)

return testValue{ID: "9", Name: "stale"}, nil
})

return err
})

<-callbackStarted
rc.Delete(t.Context(), key)

// Delete must have waited for the writer's lock, so by the time it
// returns the writer's SET has already happened and been removed.
select {
case <-writerDone:
default:
t.Fatal("Delete returned while the writer still held the lock; the writer's SET could repopulate stale data")
}

require.NoError(t, eg.Wait())

_, err := redisClient.Get(t.Context(), rc.RedisKey(key)).Result()
assert.ErrorIs(t, err, redis.Nil, "the DEL must be ordered after the in-flight writer's SET")
}

func TestRedisCache_SetWritesRedis(t *testing.T) {
t.Parallel()
redisClient := redis_utils.SetupInstance(t)
Expand Down