Skip to content
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

Ssrf work #97

Merged
merged 5 commits into from
May 11, 2022
Merged

Ssrf work #97

merged 5 commits into from
May 11, 2022

Conversation

BryceStevenWilley
Copy link
Collaborator

A bit of a large refactor that involved 3 important fixes:

  • the biggest change: not persisting Database connection objects for the whole life of the app, and instead using a pool of database connections. Persistent single connections is not really scalable, and given we haven't done much load testing, would have failed (or corrupted data / not been thread safe) under higher loads.
  • Partially addressed server-side request forgery vulnerabilities that could be happening when accepting arbitrary URLs from docassemble servers. An allow list would be nice, but can't think of a way to do that without hardcoding a list of servers (or env variable of list of servers), which will make things more difficult to interoperate / handle more traffic
  • The API keys used to access the server were being stored in plaintext. They weren't being retrieved or saved in memory at all, but were still in the database directly. Changed these keys to be hashed, and to hash the incoming keys to make sure they match. In order to make this change, I did have to setup a way to upgrade the database schema in place, which will be very helpful for future feature updates (being able to add columns to databases running in production automatically on startup, etc.)

Details and links to more resources I used to write these are in each individual commit.

Realized that having each DB have a single connection was probably
not good for long term connections or for parallelism. This change
(which has very wide sweeping effects) makes a Pooled Connection DataSource
using the Appache Commons dbcp2 library, and makes each of the Database class
act as a light wrapper with static insert / select queries around those
transient connection objects that are grabbed at the beginning of calls /
programs and not held onto after.

Some useful links:

* https://stackoverflow.com/a/18962931/11416267
* https://docs.oracle.com/javase/tutorial/jdbc/basics/sqldatasources.html
* https://stackoverflow.com/a/17746379/11416267
* Added hooks to actually run at App start
* Added tests: manually making a v0 at_rest_keys table and updating it
* Ran integration tests separately and fixed small bugs
Advice from this thread: https://stackoverflow.com/a/4507618/11416267
The javadocs say that ResultSets should also be closed when a statement
is closed, though it could vary by driver. Haven't run into issues for now,
so won't worry about it too much.
@BryceStevenWilley
Copy link
Collaborator Author

The final change that I made was trying to properly try-with-resources all of the SQL statements / prepared statements. Just some more boilerplate.

@BryceStevenWilley BryceStevenWilley merged commit aaf1521 into main May 11, 2022
@BryceStevenWilley BryceStevenWilley deleted the ssrf_work branch May 16, 2022 21:36
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