-
Notifications
You must be signed in to change notification settings - Fork 0
๐ก๏ธ Sentinel: [CRITICAL] hmac.compare_digest ์ ๋์ฝ๋ DoS ์ทจ์ฝ์ ์์ #421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,7 +114,7 @@ async def require_api_key(request: Request, call_next): | |
| if configured_keys and not (request.method == "GET" and request.url.path == "/"): | ||
| provided_key = request.headers.get("x-api-key", "") | ||
| if not any( | ||
| hmac.compare_digest(provided_key, key) for key in configured_keys | ||
| hmac.compare_digest(provided_key.encode("utf-8"), key.encode("utf-8")) for key in configured_keys | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. UTF-8 encode-before-compare is the right DoS fix. Prefer #376: it encodes the presented key once, has doctoring, CHANGELOG, and a Korean-header TestClient case without Do not land #421 and #376 together. Credential-registry migration (issues 329/373) is a later slice, not a reason to reject this encode. |
||
| ): | ||
| return JSONResponse( | ||
| status_code=401, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1222,6 +1222,25 @@ def test_video_content_type_accepted_by_validator(self): | |
| ) | ||
| ) | ||
|
|
||
| @patch.dict(os.environ, {"CODEC_CARVER_API_KEYS": "test-key"}) | ||
| def test_unicode_api_key_dos(self): | ||
| from starlette.requests import Request | ||
| import asyncio | ||
| async def mock_call_next(request): | ||
| return "SUCCESS" | ||
| scope = { | ||
| 'type': 'http', | ||
| 'method': 'POST', | ||
| 'path': '/shrink', | ||
| 'headers': [(b'x-api-key', 'test-key๐'.encode('utf-8'))], | ||
| } | ||
| req = Request(scope) | ||
| try: | ||
| res = asyncio.run(saas_web.require_api_key(req, mock_call_next)) | ||
| self.assertEqual(res.status_code, 401) | ||
| except Exception as e: | ||
| self.fail(f"Middleware crashed with exception: {e}") | ||
|
Comment on lines
+1238
to
+1242
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ Maintainability & Code Quality | ๐ก Minor | โก Quick win ๊ด๋ฒ์ํ ์์ธ ํฌ์ฐฉ์ ์ ๊ฑฐํ์ธ์. Line 1241์ ๐งฐ Tools๐ช Ruff (0.16.1)[warning] 1241-1241: Do not catch blind exception: (BLE001) ๐ค Prompt for AI AgentsSource: Linters/SAST tools There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Drop the |
||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
๐ Security & Privacy | ๐ Major | ๐๏ธ Heavy lift
๋ฐํ์ API ํค์ ์์ค๋ฅผ credential registry๋ก ์ด์ ํ์ธ์.
Line 117์ UTF-8 ๋ฐ์ดํธ ๋น๊ต๋ ๋นASCII ์ ๋ ฅ์์
TypeError๋ฅผ ๋ง์ต๋๋ค. ๊ทธ๋ฌ๋ ๋น๊ต ๋์์ธconfigured_keys๋get_configured_api_keys()๋ฅผ ํตํดCODEC_CARVER_API_KEYSํ๊ฒฝ ๋ณ์์์ ๊ณ์ ์ฝ์ต๋๋ค. API ํค ์ธ์ฆ ๊ฒฝ๋ก๋ฅผ credential registry/KV๋ก ๋ณ๊ฒฝํ๊ณ , ํ๊ฒฝ ๋ณ์๋ KV ๋ถํธ์คํธ๋ฉ์๋ง ์ฌ์ฉํ์ธ์.As per coding guidelines,
saas_web.py๋ ๋ฐํ์ API ํค๋ฅผCODEC_CARVER_API_KEYS๋๋ ์ง์ ํ๊ฒฝ ๋ณ์์์ ์ฝ์ง ์๊ณ credential registry/KV์์ ์ฝ์ด์ผ ํฉ๋๋ค.๐ค Prompt for AI Agents
Source: Coding guidelines