Repository navigation
feat: authenticate incoming requests (issue #112) - #120
jennifer-bowser wants to merge 9 commits into
Conversation
korikuzma
left a comment
korikuzma
left a comment
There was a problem hiding this comment.
This is a quick review from Codex. I haven’t reviewed the PR myself yet, but I can take a look later this week.
Please resolve the authentication bypass and failure paths, update deployment configuration, and add isolated API-level regression tests.
| token_provider: TokenProvider = TokenProvider() | ||
| vlm_client: VlmClient = VlmClient(token_provider) | ||
| node_registry: NodeRegistry = NodeRegistry(vlm_client) |
There was a problem hiding this comment.
Codex finding: Important: Pass the newly required credentials into Compose
Startup now contacts Auth0, but the Compose services do not receive VLM_CLIENT_ID or VLM_CLIENT_SECRET, and the Dockerfile does not copy .env. Setting these in the host .env does not automatically inject them into the container, so the documented development startup fails during token retrieval. Forward the credentials into the service and update the setup instructions.
| try: | ||
| scheme, token = ( | ||
| request.headers.get("Authorization", "").strip().split(sep=" ") | ||
| ) | ||
| except ValueError as e: | ||
| raise AuthError(error_reason=AuthErrorReason.INVALID_HEADER) from e |
There was a problem hiding this comment.
Codex finding: Normal: Return unauthorized when the bearer token is missing
A request without an Authorization header enters this parsing error branch and returns 400. Issue #112 requires missing tokens to be rejected as unauthorized, and MISSING_TOKEN already maps to 401. Handle the missing header separately and add an API-level assertion for the status and authentication challenge.
korikuzma
left a comment
korikuzma
left a comment
There was a problem hiding this comment.
Please validate the intended token audience and cover it with real-token tests.
| algorithms=["RS256"], | ||
| issuer=get_config().base_vlm_network_url, | ||
| options={ | ||
| "verify_signature": False, # See PR comment for more details: https://github.com/GenomicMedLab/AnyVLM/pull/120 | ||
| "verify_iss": True, | ||
| "verify_exp": True, | ||
| "require": ["iss", "exp", "azp"], |
There was a problem hiding this comment.
Codex finding: Important: Validate the intended token audience
No expected audience is supplied, and disabling signature verification also disables audience verification by default, allowing tokens intended for another API to pass these checks. Simply enabling signatures would instead reject tokens containing aud because no audience is configured (PyJWT documentation). Configure the agreed VLM audience and test real tokens with matching and mismatched audiences.
closes #112
We had specific instructions re: how to validate an incoming request from another node from the VLM network. That info is not public, so I'm not posting it here, but I've shared a copy of the doc with you via Google Drive. Notably, they indicate that tokens aren't generated with a secret key, so we need to skip signature verification. I'm very confused by this, so I'm reaching out to Hana for more info. In the meantime, I've written the code as instructed.