Skip to content

Commit

Permalink
Separate persistent and ephemeral Redis
Browse files Browse the repository at this point in the history
Add a separate Redis client and connection pool for an ephemeral Redis
instance and move OpenID Connect authentication codes into that Redis,
rather than storing them in the same Redis as persistent data such as
tokens. This ephemeral Redis will soon be used for rate limit data as
well.
  • Loading branch information
rra committed Jan 10, 2025
1 parent 24c6d14 commit fc60e0f
Show file tree
Hide file tree
Showing 9 changed files with 93 additions and 39 deletions.
3 changes: 3 additions & 0 deletions changelog.d/20250110_143131_rra_DM_48390.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
### Other changes

- OpenID Connect authentication codes are now stored in an ephemeral Redis instance rather than in the same database as data, such as tokens, that should persist.
19 changes: 15 additions & 4 deletions src/gafaelfawr/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -855,17 +855,28 @@ class Config(EnvFirstSettings):
validation_alias="GAFAELFAWR_REALM",
)

redis_url: EnvRedisDsn = Field(
redis_ephemeral_url: EnvRedisDsn = Field(
...,
title="Ephemeral Redis DSN",
description="DSN for the Redis server that stores ephemeral data",
validation_alias=AliasChoices(
"GAFAELFAWR_REDIS_EPHEMERAL_URL", "redisEphemeralUrl"
),
)

redis_persistent_url: EnvRedisDsn = Field(
...,
title="Persistent Redis DSN",
description="DSN for the Redis server that stores tokens",
validation_alias=AliasChoices("GAFAELFAWR_REDIS_URL", "redisUrl"),
validation_alias=AliasChoices(
"GAFAELFAWR_REDIS_PERSISTENT_URL", "redisPersistentUrl"
),
)

redis_password: SecretStr | None = Field(
None,
title="Persistent Redis password",
description="Password for the Redis server that stores tokens",
title="Redis password",
description="Password for both Redis servers",
validation_alias=AliasChoices(
"GAFAELFAWR_REDIS_PASSWORD", "redisPassword"
),
Expand Down
55 changes: 37 additions & 18 deletions src/gafaelfawr/factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,11 @@ class ProcessContext:
ldap_pool: AIOConnectionPool | None
"""Connection pool to talk to LDAP, if configured."""

redis: Redis
"""Connection pool to use to talk to Redis."""
ephemeral_redis: Redis
"""Connection pool to use to talk to ephemeral Redis."""

persistent_redis: Redis
"""Connection pool to use to talk to persistent Redis."""

uid_cache: IdCache
"""Shared UID cache."""
Expand Down Expand Up @@ -151,29 +154,39 @@ async def from_config(cls, config: Config) -> Self:
redis_password = None
if config.redis_password:
redis_password = config.redis_password.get_secret_value()
redis_pool = BlockingConnectionPool.from_url(
str(config.redis_url),
backoff = ExponentialBackoff(
base=REDIS_BACKOFF_START, cap=REDIS_BACKOFF_MAX
)
ephemeral_redis_pool = BlockingConnectionPool.from_url(
str(config.redis_ephemeral_url),
password=redis_password,
max_connections=REDIS_POOL_SIZE,
retry=Retry(
ExponentialBackoff(
base=REDIS_BACKOFF_START, cap=REDIS_BACKOFF_MAX
),
REDIS_RETRIES,
),
retry=Retry(backoff, REDIS_RETRIES),
retry_on_timeout=True,
socket_keepalive=True,
socket_timeout=REDIS_TIMEOUT,
timeout=REDIS_POOL_TIMEOUT,
)
redis_client = Redis.from_pool(redis_pool)
ephemeral_redis_client = Redis.from_pool(ephemeral_redis_pool)
persistent_redis_pool = BlockingConnectionPool.from_url(
str(config.redis_persistent_url),
password=redis_password,
max_connections=REDIS_POOL_SIZE,
retry=Retry(backoff, REDIS_RETRIES),
retry_on_timeout=True,
socket_keepalive=True,
socket_timeout=REDIS_TIMEOUT,
timeout=REDIS_POOL_TIMEOUT,
)
persistent_redis_client = Redis.from_pool(persistent_redis_pool)

return cls(
config=config,
firestore=firestore_client,
http_client=await http_client_dependency(),
ldap_pool=ldap_pool,
redis=redis_client,
ephemeral_redis=ephemeral_redis_client,
persistent_redis=persistent_redis_client,
uid_cache=IdCache(),
gid_cache=IdCache(),
ldap_group_cache=LDAPCache(list[Group]),
Expand All @@ -189,7 +202,8 @@ async def aclose(self) -> None:
Called during shutdown, or before recreating the process context using
a different configuration.
"""
await self.redis.aclose()
await self.ephemeral_redis.aclose()
await self.persistent_redis.aclose()
if self.ldap_pool:
await self.ldap_pool.close()
await self.uid_cache.clear()
Expand Down Expand Up @@ -308,9 +322,14 @@ def __init__(
self._logger = logger

@property
def redis(self) -> Redis:
"""Underlying Redis connection pool, mainly for tests."""
return self._context.redis
def ephemeral_redis(self) -> Redis:
"""Underlying ephemeral Redis connection pool, mainly for tests."""
return self._context.ephemeral_redis

@property
def persistent_redis(self) -> Redis:
"""Underlying persistent Redis connection pool, mainly for tests."""
return self._context.persistent_redis

async def aclose(self) -> None:
"""Shut down the factory.
Expand Down Expand Up @@ -442,7 +461,7 @@ def create_oidc_service(self) -> OIDCService:
session_secret = self._context.config.session_secret.get_secret_value()
storage = EncryptedPydanticRedisStorage(
datatype=OIDCAuthorization,
redis=self._context.redis,
redis=self._context.ephemeral_redis,
encryption_key=session_secret,
key_prefix="oidc:",
)
Expand Down Expand Up @@ -562,7 +581,7 @@ def create_token_redis_store(self) -> TokenRedisStore:
session_secret = self._context.config.session_secret.get_secret_value()
storage = EncryptedPydanticRedisStorage(
datatype=TokenData,
redis=self._context.redis,
redis=self._context.persistent_redis,
encryption_key=session_secret,
key_prefix="token:",
)
Expand Down
3 changes: 2 additions & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@ async def empty_database(engine: AsyncEngine, config: Config) -> None:
logger = structlog.get_logger(__name__)
await initialize_gafaelfawr_database(config, logger, engine, reset=True)
async with Factory.standalone(config, engine) as factory:
await factory._context.redis.flushdb()
await factory._context.ephemeral_redis.flushdb()
await factory._context.persistent_redis.flushdb()
await stamp_database_async(engine)


Expand Down
2 changes: 1 addition & 1 deletion tests/handlers/oidc_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ async def test_token_errors(
assert log["error"] == f"Unknown authorization code {bogus_code.key}"

# Corrupt stored data.
await factory.redis.set(bogus_code.key, "XXXXXXX")
await factory.ephemeral_redis.set(bogus_code.key, "XXXXXXX")
r = await client.post("/auth/openid/token", data=request)
assert r.status_code == 400
assert r.json() == {
Expand Down
6 changes: 3 additions & 3 deletions tests/services/oidc_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ async def test_issue_code(
token=token,
scopes=[OIDCScope.openid, OIDCScope.profile],
)
encrypted_code = await factory.redis.get(f"oidc:{code.key}")
encrypted_code = await factory.ephemeral_redis.get(f"oidc:{code.key}")
assert encrypted_code
fernet = Fernet(config.session_secret.get_secret_value().encode())
serialized_code = json.loads(fernet.decrypt(encrypted_code))
Expand Down Expand Up @@ -154,7 +154,7 @@ async def test_redeem_code(
now = current_datetime()
assert now - timedelta(seconds=2) <= access_data.created <= now

assert not await factory.redis.get(f"oidc:{code.key}")
assert not await factory.ephemeral_redis.get(f"oidc:{code.key}")

# If the parent session token is revoked, the oidc token returned as an
# access token should also be revoked.
Expand Down Expand Up @@ -267,7 +267,7 @@ async def test_redeem_code_errors(
# Malformed data in Redis.
fernet = Fernet(config.session_secret.get_secret_value().encode())
raw_data = fernet.encrypt(b"malformed json")
await factory.redis.set(f"oidc:{code.key}", raw_data, ex=expires)
await factory.ephemeral_redis.set(f"oidc:{code.key}", raw_data, ex=expires)
with pytest.raises(InvalidGrantError):
await oidc_service.redeem_code(
grant_type="authorization_code",
Expand Down
2 changes: 1 addition & 1 deletion tests/services/token_cache_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ async def test_expiration(config: Config, factory: Factory) -> None:
logger = structlog.get_logger("gafaelfawr")
storage = EncryptedPydanticRedisStorage(
datatype=TokenData,
redis=factory.redis,
redis=factory.persistent_redis,
encryption_key=config.session_secret.get_secret_value(),
key_prefix="token:",
)
Expand Down
30 changes: 23 additions & 7 deletions tests/services/token_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1139,7 +1139,11 @@ async def test_modify_expires(config: Config, factory: Factory) -> None:
# Check that Redis also has an appropriate TTL.
ttl = delta.total_seconds()
for token in (notebook_token, internal_token, nested_token):
assert ttl - 5 <= await factory.redis.ttl(f"token:{token.key}") <= ttl
assert (
ttl - 5
<= await factory.persistent_redis.ttl(f"token:{token.key}")
<= ttl
)

# Change the expiration of the user token.
new_delta = timedelta(seconds=config.token_lifetime.total_seconds() / 2)
Expand All @@ -1165,7 +1169,11 @@ async def test_modify_expires(config: Config, factory: Factory) -> None:
# Check that the Redis TTL has also been updated.
ttl = new_delta.total_seconds()
for token in (notebook_token, internal_token, nested_token):
assert ttl - 5 <= await factory.redis.ttl(f"token:{token.key}") <= ttl
assert (
ttl - 5
<= await factory.persistent_redis.ttl(f"token:{token.key}")
<= ttl
)


@pytest.mark.asyncio
Expand All @@ -1180,13 +1188,15 @@ async def test_invalid(
assert await token_service.get_data(token) is None

# Invalid encrypted blob.
await factory.redis.set(f"token:{token.key}", "foo", ex=expires)
await factory.persistent_redis.set(f"token:{token.key}", "foo", ex=expires)
assert await token_service.get_data(token) is None

# Malformed session.
fernet = Fernet(config.session_secret.get_secret_value().encode())
raw_data = fernet.encrypt(b"malformed json")
await factory.redis.set(f"token:{token.key}", raw_data, ex=expires)
await factory.persistent_redis.set(
f"token:{token.key}", raw_data, ex=expires
)
assert await token_service.get_data(token) is None

# Mismatched token.
Expand All @@ -1200,7 +1210,9 @@ async def test_invalid(
uid=12345,
)
session = fernet.encrypt(data.model_dump_json().encode())
await factory.redis.set(f"token:{token.key}", session, ex=expires)
await factory.persistent_redis.set(
f"token:{token.key}", session, ex=expires
)
assert await token_service.get_data(token) is None

# Missing required fields.
Expand All @@ -1215,14 +1227,18 @@ async def test_invalid(
"name": "Some User",
}
raw_data = fernet.encrypt(json.dumps(json_data).encode())
await factory.redis.set(f"token:{token.key}", raw_data, ex=expires)
await factory.persistent_redis.set(
f"token:{token.key}", raw_data, ex=expires
)
assert await token_service.get_data(token) is None

# Fix the session store and confirm we can retrieve the manually-stored
# session.
json_data["username"] = "example"
raw_data = fernet.encrypt(json.dumps(json_data).encode())
await factory.redis.set(f"token:{token.key}", raw_data, ex=expires)
await factory.persistent_redis.set(
f"token:{token.key}", raw_data, ex=expires
)
new_data = await token_service.get_data(token)
assert new_data == TokenData.model_validate(json_data)

Expand Down
12 changes: 8 additions & 4 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ setenv =
GAFAELFAWR_ALEMBIC_CONFIG_PATH = {toxinidir}/alembic.ini
GAFAELFAWR_DATABASE_URL = postgresql://gafaelfawr@localhost/gafaelfawr
GAFAELFAWR_DATABASE_PASSWORD = INSECURE-PASSWORD
GAFAELFAWR_REDIS_URL = redis://localhost/0
GAFAELFAWR_REDIS_EPHEMERAL_URL = redis://localhost/1
GAFAELFAWR_REDIS_PERSISTENT_URL = redis://localhost/0
GAFAELFAWR_UI_PATH = {toxinidir}/ui/public

[testenv:alembic]
Expand All @@ -52,7 +53,8 @@ setenv =
GAFAELFAWR_CONFIG_PATH = {toxinidir}/alembic/gafaelfawr.yaml
GAFAELFAWR_DATABASE_URL = postgresql://gafaelfawr@localhost/gafaelfawr
GAFAELFAWR_DATABASE_PASSWORD = INSECURE
GAFAELFAWR_REDIS_URL = redis://localhost/0
GAFAELFAWR_REDIS_EPHEMERAL_URL = redis://localhost/1
GAFAELFAWR_REDIS_PERSISTENT_URL = redis://localhost/0

[testenv:coverage-report]
description = Compile coverage from each test run.
Expand Down Expand Up @@ -93,7 +95,8 @@ setenv =
GAFAELFAWR_CONFIG_PATH = {toxinidir}/alembic/gafaelfawr.yaml
GAFAELFAWR_DATABASE_URL = postgresql://gafaelfawr@localhost/gafaelfawr
GAFAELFAWR_DATABASE_PASSWORD = INSECURE
GAFAELFAWR_REDIS_URL = redis://localhost/0
GAFAELFAWR_REDIS_EPHEMERAL_URL = redis://localhost/1
GAFAELFAWR_REDIS_PERSISTENT_URL = redis://localhost/0
GAFAELFAWR_REDIS_PASSWORD = TOTALLY-INSECURE-test-password

[testenv:lint]
Expand Down Expand Up @@ -130,7 +133,8 @@ setenv =
GAFAELFAWR_ALEMBIC_CONFIG_PATH = {toxinidir}/alembic.ini
GAFAELFAWR_DATABASE_URL = postgresql://gafaelfawr@localhost/gafaelfawr
GAFAELFAWR_DATABASE_PASSWORD = INSECURE-PASSWORD
GAFAELFAWR_REDIS_URL = redis://localhost/0
GAFAELFAWR_REDIS_EPHEMERAL_URL = redis://localhost/1
GAFAELFAWR_REDIS_PERSISTENT_URL = redis://localhost/0
GAFAELFAWR_UI_PATH = {toxinidir}/ui/public
TEST_KUBERNETES = 1

Expand Down

0 comments on commit fc60e0f

Please sign in to comment.