search-service: stop trusting X-User-ID, gate index endpoints behind service token, keep auth on in deploys - #1561
Open
devin-ai-integration[bot] wants to merge 7 commits into
Conversation
…ken, keep auth on in deploys Fixes auth bypass (CWE-287/CWE-290/CWE-208): the middleware trusted any X-User-ID header, was disabled via REQUIRE_AUTH=false in deploy scripts, did not separate internal index/reindex routes from user routes, and compared the service token with ==.
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…esh tokens, provision SEARCH_SERVICE_TOKEN - Drop AuthConfig.require_auth: there is no longer any switch that makes the middleware trust X-User-ID or skip credentials. Unit tests authenticate with the test service token instead of turning auth off. - Accept HS512 (auth-service signs with JJWT signWith(key) on a 64-byte secret, which selects HS512) and reject tokens carrying type=refresh. - deploy-dev.sh / deploy-tenant.sh generate SEARCH_SERVICE_TOKEN (or take a stable one from the env) and inject it into search-service; docker-compose gets a local-dev placeholder. - Test secrets are derived, not literals; .gitleaksignore pins the two historical fingerprints of the earlier test-only literal.
…pods on secret change With user JWTs no longer able to write to the index, the SNS->SQS subscription Terraform already provisions for search-service is the indexing path for Kubernetes deploys. deploy-dev.sh now reads sqs_search_indexing_queue_url and enables the consumer when it is available. Tenants keep SQS_ENABLED=false (shared topic, no per-tenant queue). The search-service pod template carries a checksum of its Secret so a regenerated SEARCH_SERVICE_TOKEN or JWT_SECRET forces a rollout instead of leaving a running pod on the previous value.
The SNS->SQS search subscription filters on an eventType message attribute that no publisher sets (document-service sends event_type, file-service sets none), so turning the consumer on delivers nothing. Wiring the indexing pipeline is a separate change; this PR keeps the pre-existing SQS_ENABLED=false and only removes the user-JWT write path to the index.
Contributor
Author
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Vulnerability: authentication bypass / broken access control in the search-service auth middleware (CWE-287 Improper Authentication, CWE-290 Authentication Bypass by Spoofing, CWE-208 Observable Timing Discrepancy). Severity: medium. Trust boundary: anything that can reach the search-service pod directly (other pods in the namespace, anyone bypassing the API gateway) — the gateway is the only component that validates JWTs, and the service assumed every caller had come through it.
Before:
app/middleware/auth.pyaccepted any request that carried a nonemptyX-User-IDheader, so a direct caller could read any user's search results by setting that header.REQUIRE_AUTH=false(set byscripts/deploy-dev.shandscripts/lib/tenant-common.sh) turned the middleware off entirely, so on deployed tenants nothing was checked at all and omitting the header gave an unscoped search. Index/reindex routes shared the same rule as user routes, so anyone with a user identity could write to or wipe the index. The service token was compared with==.After: the middleware validates identity itself, has no off switch, and separates the two endpoint classes:
AuthConfig.require_auth/REQUIRE_AUTHis removed. Protected routes fail closed; a missingJWT_SECRETorSEARCH_SERVICE_TOKENis logged at startup and rejects every request on the corresponding endpoint class.search.pyhandlers scope byg.user_idinstead ofrequest.headers["X-User-ID"], so the header is ignored even when present.JwtTokenProvideruses JJWTsignWith(key)on a 64-byte HMAC key, which selects HS512.PyJWT==2.13.0added torequirements.txt(2.10.1 has two high-severity CVEs flagged by dependency scan).deploy-dev.shanddeploy-tenant.shgenerateSEARCH_SERVICE_TOKEN(or take a stable one from the env, likeJWT_SECRET) and inject it plusJWT_SECRETinto search-service;docker-compose.ymlgets local-dev placeholders. TheREQUIRE_AUTH=truesetting was dropped since the service no longer reads it.checksum/secretsannotation so a regeneratedSEARCH_SERVICE_TOKEN/JWT_SECRETrolls the pods instead of leaving them on the old value.SQS_ENABLED=falsewas already set onmain, and the SNS->SQS search subscription filters on aneventTypeattribute that neither document-service (event_type) nor file-service (no attributes) sets. The only thing that could write to the index in a deploy was a user JWT via the gateway, which is the bypass this PR closes. Wiring the eventing pipeline is a separate change.shared/openapi/search-service.yaml:X-User-IDmarked deprecated/ignored; identity comes from the bearer JWT..gitleaksignorepins two commit-scoped fingerprints for the earlier literal test JWT secret in this branch's history (now a derived value intests/conftest.py); the rule still fires on any new occurrence.Tests:
services/search-service/tests/test_auth.py(new, 21 cases) runs the app with real credentials configured and covers the attack path: forgedX-User-ID→ 401,REQUIRE_AUTH=falsein the env has no effect → 401, wrong-secret/alg=noneJWT → 401, refresh token → 401, valid HS256/HS384/HS512 JWT accepted and results scoped to the JWT subject even whenX-User-IDsays otherwise, user JWT on index/reindex/delete → 403, wrong service token → 401, correct one → allowed.tests/api/test_search_flow.py(black-box, run against a gateway) previously indexed documents with a user JWT; it now asserts that returns 403 and keeps the search/suggest/advanced and 400-validation checks.Search-service suite: 62 passed.
ruff checkclean on changed service files (remainingBLE001/I001findings are pre-existing in untouched code).gitleaks detect --log-opts origin/main..HEADclean locally.Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/80d79842d8934d5c82c6eaa504128ab1
Open in Devin Desktop: https://partner-workshops.devinenterprise.com/desktop/session/80d79842d8934d5c82c6eaa504128ab1?variant=devin
Requested by: @mbatchelor81