Implement proxy v2 architecture, in Rust by legoktm · Pull Request #1718 · freedomofpress/securedrop-client

legoktm · 2023-12-13T21:23:02Z

This was migrated from freedomofpress/securedrop-proxy#127

Status

Ready for review

Description

Implement the proxy v2 architecture; see individual commits messages for more detail.

Test Plan

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

I have tested these changes in the appropriate Qubes environment
I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
These changes should not need testing in Qubes

If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:

I have updated the AppArmor profile
No update to the AppArmor profile is required for these changes
I don't know and would appreciate guidance
AppArmor will be done as a follow-up: Confine securedrop-proxy with apparmor #1889

If these changes modify the database schema, you should include a database migration. Please check as applicable:

I have written a migration and upgraded a test database based on main and confirmed that the migration is self-contained and applies cleanly
I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
I need help writing a database migration
No database schema changes are needed

legoktm · 2023-12-13T21:23:44Z

I accidentally squashed the README commits when migrating and then dropped the CircleCI config since we need to redo it for GHA anyways.

legoktm · 2024-01-12T00:01:35Z

I just pushed "WIP: Have the sdk always shell out to the proxy" which is somewhat working, I messed up in negating a boolean somewhere so file downloading is a bit messed up, but the login and other API requests are happening via shelling out to a locally built proxy. Once that's fixed, the next step will be mocking HTTP traffic in tests, maybe with some vcr-lite system.

proxy/src/main.rs

cfm · 2024-01-18T20:49:59Z

I'll pick this up from @legoktm after today, starting with the to-do list I've added in #1718 (comment). (Feel free to edit and add to it!)

legoktm · 2024-01-18T21:02:37Z

A bit of an update and what's left:

@cfm and I discussed that we're good with outputting headers on stderr in streaming mode, except that it should be all headers, and the proxy should not hardcode specific headers.
I fixed whatever subprocess issue was causing problems and now the client is able to use the proxy in development mode when started with the ./run.sh script.
Streaming is actually streaming now! Had to switch to async reqwest but the code flow should be relatively straightforward.
Tests need fixing, they're currently using vcr. We either need to reimplement vcr for our mock format or mock the proxying to use requests and then keep using vcr.
Also the proxy code needs a test that actually verifies streaming. Should be doable with the e.g. /drip?duration=5&numbytes=20&code=200&delay=0 endpoint
Packaging needs to run cargo build --release and install it; also need to bootstrap the Rust compiler into the container.
It would be good to clean up the git history with a rebase, we could try to split this into other PRs, e.g. I think we could maybe merge some of the cargo-vet infrastructure ahead of time?

legoktm · 2024-01-19T01:21:02Z

Oh also, it might be worth looking at the SDK error handling a bit closer, there's a few raised errors that I left with "FIXME", but also I saw at least one higher-level method that was directly catching a JSON error, which shouldn't be possible since _send_json_request() should already catch it.

@legoktm

Per @legoktm in <#1718 (comment)>.

cfm · 2024-01-23T19:18:02Z

#1718 (comment) is done as suggested. Stack of work is now:

cfm · 2024-01-24T02:57:36Z

#1718 (comment):

Tests need fixing, they're currently using vcr. We either need to reimplement vcr for our mock format or mock the proxying to use requests and then keep using vcr.

Tracking this in #1778.

cfm · 2024-02-02T03:14:33Z

#1778 is substantively done in f3a5c5a...b82720b and just needs some documenting and Git-polishing.

cfm · 2024-02-06T01:50:28Z

#1778 is done and documented in f3a5c5a...b002a5f. @legoktm, let's sync up about this and the remaining checklist here tomorrow.

@legoktm

Per @legoktm in <#1718 (comment)>.

@legoktm

Per @legoktm in <#1718 (comment)>.

Rewrite the existing tests to be integration tests against a compiled Rust binary. We use the httpbin library to start up a Python webserver and instruct the proxy to connect to it. This allows to test connection properties that aren't recordable in the VCR format, like timeouts or streamed responses. The tests are reorganized to be split into proxy handling and error handling.

The SDK will now unconditionally use proxy v2: in development mode, it'll shell out directly, while production mode will invoke it over qrexec. The dev `./run.sh` script will compile the proxy so it's ready for use before the client starts up. New typed dataclasses represent the two types of responses that can be returned. No user-facting changes are happening at this stage, but this will enable future client features. The union return of send_json_request() means that we need instanceof assertions to make mypy happy. One minor logic bug in API.get_submission() was fixed in the case that no request is made (an undefined exception would've been raised previously).

@vickyliin

We can't use VCR.py's "custom_patches" parameter because our API._send_json_request() is RPC- rather than connection-oriented. But we can just instrument _send_json_request() directly, which is what we do here. We subclass vcr.cassette.Cassette to handle identical requests with different responses, which was suggestd by @vickyliin as a workaround for kevin1024/vcrpy#753. Now that the SDK‒proxy connection is itself instrumented, there's only one path to test, with no special error-handling logic required, so merge TestAPIProxy into TestAPI. I considered merging TestAPI and TestShared as well, now that (without TestAPIProxy) TestAPI is the only subclass of TestShared. But reorganizing the alphabetized helpers in TestShared versus the strictly-sequenced TestAPI methods can wait. The tests are also now more patient with slow deletion operations. I'd want to DRY up this logic if this pattern shows up in more places, but it would require adding another level of indirection. A @Retry decorator isn't appropriate at the level of the test method, and a context manager can't loop over its closure. And re-apply the hack from 880635d by renaming test_logout to start with a "z" so it runs last.

Help the developer through regenerating SDK cassettes against a development server, as well as in CI.

* Switch to `Arch: any` because the package contains compiled code and this enables debhelper's automatic shlibs dependency system. * Rename the binary to `securedrop-proxy` because that's the Rust name.

Audits were done by Kunal over the course of a few months.

Same as the SDK, use our custom VCRAPI wrapper instead of pytest-vcr. Because each of the functional tests log in and out, document the server hack needed to run them one after another in the README.

Now replaced by the Rust version.

Fixes #1761.

We are setting the proxy's origin in QubesDB (via dom0 salt), and as discussed/agreed upon in <freedomofpress/securedrop-workstation#1013>, we're reading it directly in the proxy. We link directly to libqubesdb, generating the Rust wrapper with bindgen so it stays in sync (at build time at least). As a result, some unsafe is needed, but it is clearly annotated with relevant SAFETY notes. If we're not building with QubesDB (for development purposes), we'll continue to read from the environment and skip the bindgen and linking steps entirely. Fixes <freedomofpress/securedrop-workstation#853>. Co-authored-by: Kunal Mehta <[email protected]>

libclang is needed by bindgen. The Qubes signing key was fetched from <https://github.com/QubesOS/qubes-builder-debian/blob/64f530a712539829f8f2c1fbb0907d99b562ccdd/keys/qubes-debian-r4.2.asc>.

… environments

This does nothing because Rust code is linted through the root Makefile's `rust-lint` target, and Python code linted from the root's ruff targets as well.

Historically production systems used ~/QubesIncoming/sd-proxy as a temporary directory because files were sent over qvm-move. This switches to ~/Downloads as a stop-gap. A better interface would be for the caller to provision a temporary directory, and pass it as the path.

Avoid IndexError in the case that `response.stdout` is empty.

Co-authored-by: Kunal Mehta <[email protected]>

The relevant httpbin issue has been resolved and is compatible with newer werkzeug.

legoktm

I can't approve this since I opened the PR, but this is ready to go and has sign-offs from @cfm and @micahflee on their respective parts.

addressed as per ~/Downloads discussion.

zenmonkeykstop

Stamping given @cfm and @micahflee's reviews, CI passing, and (admittedly) cursory review.

Now that proxy v2 has landed (see <freedomofpress/securedrop-client#1718>), we can remove the qubes.Filecopy RPC rule from sd-proxy to sd-client (it now goes over the securedrop.Proxy RPC) and the sd-proxy.yaml configuration (now read from QubesDB). Fixes #1026.

legoktm mentioned this pull request Dec 13, 2023

Implement proxy v2 architecture, in Rust freedomofpress/securedrop-proxy#127

Closed

legoktm force-pushed the proxy-rusting branch from d470ee7 to 07e97ec Compare January 5, 2024 21:14

legoktm requested a review from a team as a code owner January 5, 2024 21:14

legoktm force-pushed the proxy-rusting branch 3 times, most recently from 658fb3d to 1a025c9 Compare January 11, 2024 23:57

cfm reviewed Jan 18, 2024

View reviewed changes

proxy/src/main.rs Outdated Show resolved Hide resolved

cfm reviewed Jan 18, 2024

View reviewed changes

proxy/src/main.rs Outdated Show resolved Hide resolved

cfm assigned legoktm and cfm Jan 18, 2024

legoktm force-pushed the proxy-rusting branch from 8633321 to 80e7718 Compare January 18, 2024 21:03

cfm added a commit that referenced this pull request Jan 23, 2024

refactor(authenticate): let _send_json_request() catch JSONDecodeErrors

984f19d

Per @legoktm in <#1718 (comment)>.

cfm mentioned this pull request Jan 24, 2024

teach vcrpy our proxy v2 request/response semantics #1778

Closed

cfm force-pushed the proxy-rusting branch from 33cdf0b to b82720b Compare February 2, 2024 02:54

cfm force-pushed the proxy-rusting branch 3 times, most recently from eac46df to b002a5f Compare February 6, 2024 01:46

legoktm pushed a commit that referenced this pull request Feb 12, 2024

refactor(authenticate): let _send_json_request() catch JSONDecodeErrors

9a9d9e3

Per @legoktm in <#1718 (comment)>.

legoktm force-pushed the proxy-rusting branch from b002a5f to bc30f84 Compare February 12, 2024 20:04

legoktm pushed a commit that referenced this pull request Feb 12, 2024

refactor(authenticate): let _send_json_request() catch JSONDecodeErrors

af23ae9

Per @legoktm in <#1718 (comment)>.

legoktm force-pushed the proxy-rusting branch from bc30f84 to 2b95298 Compare February 12, 2024 20:48

legoktm and others added 19 commits May 17, 2024 15:14

Install Rust in proxy test runs and build

c237931

Add make regenerate-sdk-cassettes and use in SDK CI

ecdd356

Help the developer through regenerating SDK cassettes against a development server, as well as in CI.

Build Rust code in package

3711367

* Switch to `Arch: any` because the package contains compiled code and this enables debhelper's automatic shlibs dependency system. * Rename the binary to `securedrop-proxy` because that's the Rust name.

Add cargo-vet config

1bb415f

Audits were done by Kunal over the course of a few months.

Switch functional tests to use custom VCR setup

5cd8f7d

Same as the SDK, use our custom VCRAPI wrapper instead of pytest-vcr. Because each of the functional tests log in and out, document the server hack needed to run them one after another in the README.

Remove Python implementation of proxy

0ca17ab

Now replaced by the Rust version.

Remove requests dependency

a6db478

Fixes #1761.

Install qubesdb dependencies during packaging and CI

c6e7ba6

libclang is needed by bindgen. The Qubes signing key was fetched from <https://github.com/QubesOS/qubes-builder-debian/blob/64f530a712539829f8f2c1fbb0907d99b562ccdd/keys/qubes-debian-r4.2.asc>.

docs(readme): visually compare the development and staging/production…

52bd1fd

… environments

Documentation improvements

665bd5f

Remove proxy's no-op make lint

c91d76c

This does nothing because Rust code is linted through the root Makefile's `rust-lint` target, and Python code linted from the root's ruff targets as well.

Add bounds check in _send_json_request

1755185

Avoid IndexError in the case that `response.stdout` is empty.

Apply suggestions from code review

82a71a0

Co-authored-by: Kunal Mehta <[email protected]>

chore(Poetry): remove pinned werkzeug, then "poetry lock --no-update"

caabec2

The relevant httpbin issue has been resolved and is compatible with newer werkzeug.

legoktm force-pushed the proxy-rusting branch from 03bb417 to caabec2 Compare May 17, 2024 19:15

legoktm requested a review from cfm May 17, 2024 19:23

legoktm commented May 17, 2024

View reviewed changes

zenmonkeykstop approved these changes May 17, 2024

View reviewed changes

zenmonkeykstop merged commit 9e56263 into main May 17, 2024

zenmonkeykstop deleted the proxy-rusting branch May 17, 2024 20:03

legoktm mentioned this pull request May 17, 2024

Update for proxy v2 freedomofpress/securedrop-workstation#1035

Merged

4 tasks

eloquence mentioned this pull request May 20, 2025

Implement v3 proxy architecture #2453

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement proxy v2 architecture, in Rust#1718

Implement proxy v2 architecture, in Rust#1718
zenmonkeykstop merged 21 commits intomainfrom
proxy-rusting

legoktm commented Dec 13, 2023 •

edited by micahflee

Loading

Uh oh!

legoktm commented Dec 13, 2023

Uh oh!

legoktm commented Jan 12, 2024

Uh oh!

Uh oh!

Uh oh!

cfm commented Jan 18, 2024

Uh oh!

legoktm commented Jan 18, 2024 •

edited by cfm

Loading

Uh oh!

legoktm commented Jan 19, 2024

Uh oh!

cfm commented Jan 23, 2024

Uh oh!

cfm commented Jan 24, 2024

Uh oh!

cfm commented Feb 2, 2024

Uh oh!

cfm commented Feb 6, 2024

Uh oh!

legoktm left a comment

Uh oh!

zenmonkeykstop left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

legoktm commented Dec 13, 2023 • edited by micahflee Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status

Description

Test Plan

Checklist

Uh oh!

legoktm commented Dec 13, 2023

Uh oh!

legoktm commented Jan 12, 2024

Uh oh!

Uh oh!

Uh oh!

cfm commented Jan 18, 2024

Uh oh!

legoktm commented Jan 18, 2024 • edited by cfm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legoktm commented Jan 19, 2024

Uh oh!

cfm commented Jan 23, 2024

Uh oh!

cfm commented Jan 24, 2024

Uh oh!

cfm commented Feb 2, 2024

Uh oh!

cfm commented Feb 6, 2024

Uh oh!

legoktm left a comment

Choose a reason for hiding this comment

Uh oh!

zenmonkeykstop left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

legoktm commented Dec 13, 2023 •

edited by micahflee

Loading

legoktm commented Jan 18, 2024 •

edited by cfm

Loading