Compare node API keys in constant time on /upload
/upload compared the per-node API key with a plain !=, which short-circuits on the first differing byte and so leaks a little information about how much of a guess was correct. The reason to fix it is less the timing channel itself -- an HTTP round trip is noisy -- than the inconsistency: enrollment.py and dynsec.py both went out of their way to use secrets.compare_digest for the same class of credential, so the codebase contradicted itself on whether this mattered. Now it does not. Also coalesces a missing api_key field to "" so compare_digest is never handed None, which would raise TypeError and turn a malformed node_keys document into a 500 instead of a 401. Closes logan/server-26#12
This commit is contained in:
@@ -1,3 +1,4 @@
|
||||
import secrets
|
||||
from typing import Optional
|
||||
from datetime import datetime, timezone
|
||||
from fastapi import APIRouter, BackgroundTasks, UploadFile, File, Form, HTTPException, Security
|
||||
@@ -36,7 +37,11 @@ async def upload_call_audio(
|
||||
if not key_doc:
|
||||
logger.warning(f"Upload 401: no key_doc in Firestore for node_id={node_id!r}")
|
||||
raise HTTPException(401, "Invalid node API key")
|
||||
if key_doc.get("api_key") != credentials.credentials:
|
||||
# compare_digest, not !=, so the comparison cost does not depend on how many
|
||||
# leading characters matched. enrollment.py and dynsec.py were explicit about
|
||||
# this for the same class of credential; this route was the odd one out.
|
||||
stored_key = key_doc.get("api_key") or ""
|
||||
if not secrets.compare_digest(stored_key, credentials.credentials):
|
||||
logger.warning(
|
||||
f"Upload 401: key mismatch for node_id={node_id!r} "
|
||||
f"(received prefix: {credentials.credentials[:8]}...)"
|
||||
|
||||
Reference in New Issue
Block a user