-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
RFC: AWS Serverless Support #5591
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR. I'm not sure if this is something we want to have embedded by default though, @dani-garcia what is your opinion here. Further, i do like the abstraction of the filesystem part. But it's still a lot of custom work. It might be nicer to try and use Opendal maybe? That supports a lot of different file and object storage's. |
I hadn’t seen Opendal before, but it looks like it should be a perfect fit for this use case. I’d be happy to refactor the fs work to use it. Hopefully that would address testing concerns of the fs layer as well, since it should be abstracting away most of the functionality? The SES code is quite limited in scope. It may be hard to test without an aws account, but it sort of “just works” because it takes the whole raw email from lettre and sends it. The DSQL part is most challenging. That said, most of the changes involve implementing ManageConnection in order to generate and cache pre-signed connection URLs. AWS and GCP are moving in this direction for greater security via temporary credentials. I hope we can find a solution here we all like, but I would be fine with deferring DSQL work for now given it’s still in Preview state with known issues. |
Same MinIO or Garage (in Rust) are good replacements.
At work I used the SMTP interface for quite a while, so no code changes are "hard" required. Having API support is nice indeed.
How is this different from MySQL Aurora ?
I also would like to see a first set of changes merged (all the non AWS required stuff) |
Agreed with some of the points mentioned. Ideally I'd like to have as much as this code integrated into crates that we reference instead of including the code in the project which would ensure that other developers get to use these services as well, while also removing some maintenance load from us. For example:
If we can get those points solved, we'd be in a situation where there would be barely any aws specific code in vaultwarden, and I'd feel more confident including support for it. The missing features from DSQL are a bit annoying, and it would be unfortunate if we need to maintain a separate migration path without foreign keys and dsql specific changes, so I agree that delaying that until it's out of preview seems like a good idea. The unimplemented functionality shouldn't be a problem, we could refactor background jobs to store time since last run on the db and run them on startup. About Websockets, that might be harder to do but they shouldn't be necessary for functionality, the clients will run syncs periodically regardless of if they receive updates via websockets or not. There are works in progress to move some clients to use WebPush so it would be less important in the future as well. |
Thanks for all the comments! What I'm hearing is:
I propose developing and merging the following PRs in sequence, deferring DSQL integration for now:
Please follow up to confirm agreement (or point out where I'm missing something) before I get too deep into the weeds of Opendal :). |
I also wanted to address one point @williamdes brought up in #5591 (comment) about SES having an SMTP interface that would mean no code changes to Vaultwarden would be required. Unfortunately, the SES SMTP proxy requires long-term credentials:
When it's so easy to run a service that uses temporary AWS credentials, which is a security best-practice, I have a hard time proposing a solution that requires long-term credentials :). Speaking for myself, I don't want to run a Vaultwarden instance that uses any long-term credentials. |
This draft PR contains a POC of support for deploying an instance of Vaultwarden into an AWS account using entirely "serverless" services (likely falling within the free-tier usage limits as well). I'm looking for feedback and agreement by Vaultwarden maintainers on whether these contributions could be merged into vaultwarden (with further refinement).
Architecture
All of this is implemented in the PR behind feature flags:
dsql
,s3
, andses
. All three can be enabled together via theaws
feature flag.Unimplemented Functionality
I believe all functionality, except as listed below, is functional. But I'm new to vaultwarden and may have missed something along the way. I've not found any significant issues with my own usage due to this missing functionality, however.
Open Questions / Concerns
POST
instead of S3'sPUT
to upload, and although S3 also supportsPOST
with signed URLs the data must be form-encoded. This means existing Bitwarden clients cannot upload directly to S3, and must instead upload through the API Lambda Function. Unfortunately, the AWS Lambda service has a 6 MB size limit for request payloads. Note: This only applies to uploads, downloads are streamed from S3 directly via signedGET
URLs which do not have a size limit.Deployment Instructions
See the aws/README.md file in this PR.
Proposed Plan of Attack
This PR is too large to attempt to review and merge with sanity. With agreement from Vaultwarden maintainers in comments below, I propose developing and merging the following as separate PRs in sequence:
persistent_fs
module, migrating existing filesystem functionality into a "local" backend implementation. Seesrc/persistent_fs/mod.rs
andsrc/persistent_fs/local.rs
. This would not change any functionality; it would simply rearchitect file access in preparation for the addition of an S3 backend.dsql-beta
)