Skip to content

[Prototype] Add mandatory meta_key check to server #937

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

Closed
wants to merge 1 commit into from

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Jun 22, 2022

Prerequisites:

  • Unless it is a documentation hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

Summary

This adds a prototype of a basic authentication system to the API server for meta cache computations. Since the meta computations run on a multi-user machine, with read and write privileges to the main DB, we need to make it secure. The idea here is to run the API server with user1 and run the meta cache computations with user2. Both users will share a secret key, which will make sure the API server only responds to requests made by user2. The basic logic is to expect a secret meta_key parameter in every request.

Many tests had to be fixed. I went with a simple patching route that added the missing fake meta_key parameter in most requests.

This will likely be deprecated by #633, but the review and roll out of that work is still a way into the future.

Creating this PR for visibility and review, though it definitely should not be merged into dev. I envision that this code will continue to live on a separate branch and the meta cache computations server will just run its API server from that branch. Any updates to the JIT computations will need to be propagated to this branch too, whenever they are pushed to dev. The diffs shouldn't be too hard to manage. We should probably automate pulling into this branch when dev gets updated. We will also want to automate updating the meta cache computations API server whenever this code gets updated.

For the sake of tests passing, the meta_key comparison only expects a meta_key if more than zero parameters were requested from the API. So for example, requests like https://api.covidcast.cmu.edu/epidata/covidcast/ and https://api.covidcast.cmu.edu/epidata/ pass. For these two, the result returns no actual data, so it's fine, but I need to double check that this won't expose information in other endpoints.

* fix all tests by adding param where needed
@dshemetov dshemetov requested a review from krivard June 22, 2022 01:58
@dshemetov dshemetov changed the title Add mandatory meta_key check to server [Prototype] Add mandatory meta_key check to server Jun 22, 2022
@dshemetov
Copy link
Contributor Author

Misunderstood the attack vector. A big concern is making sure that FLASK_SECRET isn't exposed to other users. Will ping @korlaxxalrok later for help with using local secret overrides.

@dshemetov dshemetov closed this Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant