Skip to content

Direct upload to Azure/Local#1188

Merged
MGibson1 merged 17 commits intomasterfrom
enable-azure-direct-upload
Mar 22, 2021
Merged

Direct upload to Azure/Local#1188
MGibson1 merged 17 commits intomasterfrom
enable-azure-direct-upload

Conversation

@MGibson1
Copy link
Member

@MGibson1 MGibson1 commented Mar 5, 2021

Overview

This PR is the second half of #1174. It creates endpoints to allow direct upload to Azure blob storage and local storage. It also provides renewed upload URLs if permissions expire, validates file upload sizes, and locks down Send writes once size is validated.

Files Changed

  • Api.csproj: Add event grid nuget dependency. This package allows for easier integration with Azure Event Grid, which is used to verify Send file sizes.
  • SendsController.cs: New endpoints:
    • PostFile (v2): Creates a file type send and get a URL/SendResponse to upload the file to. FileUploadType is used to specify Azure vs Direct upload. This request makes and validates a send based on a promised file size. This is checked upon actual file upload.
    • RenewFileUploadUrl: Verifies a Send exists, belongs to the user, and has not yet been written (validated == false), then provides a new upload URL to allow for extended file operations.
    • PostFileForExistingSend: The API endpoint for localStorage deployments. This maintains the 100MB upload limit, uploads the file to local storage. SendService also validates the length of the received file stream is within a grace size.
    • AzureValidateFile: Azure event webhook endpoint. Accepts Azure Event Grid Event POSTs and handles blobCreate events, validating the final blob size. This method blocks the client's return from blob create writes.
  • ApiHelpers.cs: Azure requires a response verification code to prove we own the webhook endpoint we specify. This helper automatically handles Verification events and accepts a Dictionary of event handlers to call for other Azure events.
  • MultipartFormDataHelper.cs: Add overload to GetSendFileAsync where formData contains only file data now since the URL has the send Id and file ID that have already been created. The old overload still exists to support old clients.
  • FileUploadType.cs: Specifies the method of uploading a file. Direct is direct to Bitwarden. Azure is an Azure Upload. This is used by the client to upload in the appropriate manner.
  • SendRequestModel.cs: Optionally provide a hint of the File Size that will be uploaded for this Send. The FileLength is required for File Sends and ignored for Text sends.
  • SendFileUploadDataResponseModel: Specifies the Send URL, UploadType, and Generated Send. This is the response to a request to make a new File Type Send.
  • SendFileData: Add Validated bool to data model. This defaults to true so existing sends cannot be updated by requesting an upload URL for them. Send Data Creations must, unfortunately, specify this value as false upon instantiating new SendFileData.
  • ISendService/SendService.cs:
    • FileUploadType: specify the file upload method to the client. Current options are Direct and Azure.
    • SaveFileSendAsync: Creates a File Send without file data and responds with a string to upload the file data to.
    • UploadFileToExistingSend: The local storage endpoint. Should not be used for Azure deployments. Validates existence of Send, stores the stream, and validates its size prior to returning. Throws in the event of a size mismatch since the client will correctly interpret that throw as a toast error.
    • ValidateSendFile.cs: Simply validates the specified file and deletes the send if it does not mesh within a reasonable leeway (set to 1MB). Updates Send's data with size and sets validation to true.
  • AzureSendFileStorageService.cs:
    • GetSendUploadUrl: Uses SAS string to authorize a 1 min create permission to the specified blob. This requires a few Azure settings
      1. CORS for blob storage needs to be set to allow any origin PUT
      2. The sendfiles container should have permissions set to off
    • ValidateFile: Called from the Azure event webhook upon blob creation (which occurs after the file is finished uploading). Pulls the blob's size and sets some metadata/property attributes that could not be set prior to creation. We could offload this to the clients, but it makes sense to keep this file organization stuff in the server. If the size is not roughly equal to expected, returns false to indicate the Send should be removed due to un-validated file size. Always returns size of the blob (null if not found). This allows SendService to do some additional work updating db
  • LocalSendFileStorageService: Validates that file on disk equals expected file size. We need to write to disk first since HttpStreams have no known length until they're read and we don't want to risk storing a huge file in memory to figure out the size.
  • NoopSendFileStorageService: Noop implementations.

Required Azure Changes

  1. CORS for blob storage needs to be set to allow any orgin for GET and PUT methods (upload is PUT only, but GET is needed for the download portion).
  2. The sendfiles container should have permissions set to off
  3. Specify webhook event for blob creation with filters defined by @joseph-flinn. Endpoint is <<apiUrl>>/sends/file/validate/azure

Testing requirements

Need to test upload and download to both local and Azure hosted environments.

Need to test using both old and new client, to make sure original Send upload implementation is not broken.

I've tested lying about file size, but a second set of eyes on that is a good idea.

The above require access to bitwarden/web#875 for the new client implementation

MGibson1 and others added 15 commits March 4, 2021 10:16
To validate file sizes in the event of a rogue client, Azure event webhooks
will be hooked up to AzureValidateFile.
Sends outside of a grace size will be deleted as non-compliant.

TODO: LocalSendFileStorageService direct upload method/endpoint.
These shouldn't happen, but might if some errant get requests occur
It turns out that multipartHttpStreams do not have a length
until read. this causes all long files to be "invalid". We need to
write the entire stream, then validate length, just like Azure.

the difference is, We can return an exception to local storage
admonishing the client for lying
Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>
Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>
@MGibson1 MGibson1 requested a review from a team March 5, 2021 16:42
@MGibson1 MGibson1 marked this pull request as ready for review March 17, 2021 17:50
Copy link
Contributor

@cscharf cscharf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just a few comments

@MGibson1 MGibson1 requested a review from cscharf March 19, 2021 19:11
@MGibson1 MGibson1 merged commit 989d4df into master Mar 22, 2021
@MGibson1 MGibson1 deleted the enable-azure-direct-upload branch March 22, 2021 04:01
addisonbeck pushed a commit that referenced this pull request Mar 22, 2021
* Direct upload to azure

To validate file sizes in the event of a rogue client, Azure event webhooks
will be hooked up to AzureValidateFile.
Sends outside of a grace size will be deleted as non-compliant.

TODO: LocalSendFileStorageService direct upload method/endpoint.

* Quick respond to no-body event calls

These shouldn't happen, but might if some errant get requests occur

* Event Grid only POSTS to webhook

* Enable local storage direct file upload

* Increase file size difference leeway

* Upload through service

* Fix LocalFileSendStorage

It turns out that multipartHttpStreams do not have a length
until read. this causes all long files to be "invalid". We need to
write the entire stream, then validate length, just like Azure.

the difference is, We can return an exception to local storage
admonishing the client for lying

* Update src/Api/Utilities/ApiHelpers.cs

Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>

* Do not delete directory if it has files

* Allow large uploads for self hosted instances

* Fix formatting

* Re-verfiy access and increment access count on download of Send File

* Update src/Core/Services/Implementations/SendService.cs

Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>

* Add back in original Send upload

* Update size and mark as validated upon Send file validation

* Log azure file validation errors

* Lint fix

Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>
@MGibson1 MGibson1 mentioned this pull request Mar 22, 2021
addisonbeck pushed a commit that referenced this pull request Apr 8, 2021
* Direct upload to azure

To validate file sizes in the event of a rogue client, Azure event webhooks
will be hooked up to AzureValidateFile.
Sends outside of a grace size will be deleted as non-compliant.

TODO: LocalSendFileStorageService direct upload method/endpoint.

* Quick respond to no-body event calls

These shouldn't happen, but might if some errant get requests occur

* Event Grid only POSTS to webhook

* Enable local storage direct file upload

* Increase file size difference leeway

* Upload through service

* Fix LocalFileSendStorage

It turns out that multipartHttpStreams do not have a length
until read. this causes all long files to be "invalid". We need to
write the entire stream, then validate length, just like Azure.

the difference is, We can return an exception to local storage
admonishing the client for lying

* Update src/Api/Utilities/ApiHelpers.cs

Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>

* Do not delete directory if it has files

* Allow large uploads for self hosted instances

* Fix formatting

* Re-verfiy access and increment access count on download of Send File

* Update src/Core/Services/Implementations/SendService.cs

Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>

* Add back in original Send upload

* Update size and mark as validated upon Send file validation

* Log azure file validation errors

* Lint fix

Co-authored-by: Chad Scharf <3904944+cscharf@users.noreply.github.com>
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.

2 participants