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

ADR: Download Strategy #821

Merged
merged 11 commits into from
Apr 21, 2021
Merged

Conversation

carltonsmith
Copy link

Summary of Changes

Documentation for our Download Strategy decision

@carltonsmith carltonsmith added ADR documentation raft review This issue is ready for raft review labels Apr 6, 2021
@carltonsmith carltonsmith requested a review from kniz-raft April 6, 2021 15:58
Copy link

@kniz-raft kniz-raft left a comment

Choose a reason for hiding this comment

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

LGTM

@kniz-raft kniz-raft linked an issue Apr 7, 2021 that may be closed by this pull request
2 tasks
@jtwillis92 jtwillis92 added QASP Review and removed raft review This issue is ready for raft review labels Apr 7, 2021
@jtwillis92 jtwillis92 requested a review from ADPennington April 7, 2021 17:50
@jtwillis92 jtwillis92 mentioned this pull request Apr 7, 2021
11 tasks

We believe the use of time/IP address limited signed URLs is a reasonably secure approach to downloading files from S3. However, we also believe that it may cause issues with our ATO approval as the data is highly sensitive. Furthermore, 18F published a recommendation today, [recommending to not use pre-signed URLs](https://engineering.18f.gov/security/cloud-services/) for FISMA High projects.

In our investigation we discovered a way that we can securely download the files from the backend while streaming the files directly from S3 to the client, taking any pressure off of resources needed for parsing files on the backend.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@carltonsmith sounds promising! can we include reference links here to what was found?

Copy link
Author

Choose a reason for hiding this comment

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

The feature isn't documented in the docs, but can be found in the codebase, which is where we found it. I can include a link but it takes a bit of analysis to understand what is happening.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@carltonsmith, yes it'd be great to have the link but also, i think it'd be really helpful to have a walk-through of part of the codebase where this is happening. let us know when you think it might be best to do this (if not today); or happy to have your suggestions on other ways we can go about learning about this alternative as part of qasp review 😃

Choose a reason for hiding this comment

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

Updated this PR with technical documentation to explain how the downloads will work under this strategy.

Pasting the associated flowchart here for reference:
tdp-data-file-download-api

Copy link
Collaborator

@ADPennington ADPennington left a comment

Choose a reason for hiding this comment

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

thanks for the additional documentation @jtwillis92! helpful to have these breadcrumbs on how the dev team envisions this strategy being implemented.

cc: @lfrohlich

@carltonsmith carltonsmith merged commit 14e483b into raft-tdp-main Apr 21, 2021
@carltonsmith carltonsmith deleted the documentation/adr-download-strategy branch April 21, 2021 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SPIKE] Investigate alternative to Signed URLs for download
5 participants