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

Implement proxy v2 architecture, in Rust #1718

Merged
merged 21 commits into from
May 17, 2024
Merged

Implement proxy v2 architecture, in Rust #1718

merged 21 commits into from
May 17, 2024

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Dec 13, 2023

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

  • CI passes
  • Visual review:
  • Start dev instance with cd client && ./run.sh and test syncing, sending replies and downloading attachments works fine
  • Build and install debs onto Qubes 4.2 system, test syncing, sending replies and downloading attachments works fine

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:

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
Copy link
Member Author

legoktm commented Dec 13, 2023

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

@legoktm
Copy link
Member Author

legoktm commented Jan 12, 2024

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 Outdated Show resolved Hide resolved
proxy/src/main.rs Outdated Show resolved Hide resolved
@cfm cfm assigned legoktm and cfm Jan 18, 2024
@cfm
Copy link
Member

cfm commented Jan 18, 2024

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
Copy link
Member Author

legoktm commented Jan 18, 2024

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
Copy link
Member Author

legoktm commented Jan 19, 2024

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.

@cfm
Copy link
Member

cfm commented Jan 23, 2024

@cfm
Copy link
Member

cfm commented Jan 24, 2024

#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
Copy link
Member

cfm commented Feb 2, 2024

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

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

cfm commented Feb 6, 2024

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

cfm and others added 17 commits May 17, 2024 15:14
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.
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 <legoktm@debian.org>
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 <legoktm@debian.org>
The relevant httpbin issue has been resolved and is compatible with
newer werkzeug.
Copy link
Member Author

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

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.

@zenmonkeykstop zenmonkeykstop dismissed cfm’s stale review May 17, 2024 20:02

addressed as per ~/Downloads discussion.

Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

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

@zenmonkeykstop zenmonkeykstop merged commit 9e56263 into main May 17, 2024
52 checks passed
@zenmonkeykstop zenmonkeykstop deleted the proxy-rusting branch May 17, 2024 20:03
legoktm added a commit to freedomofpress/securedrop-workstation that referenced this pull request May 17, 2024
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 added a commit to freedomofpress/securedrop-workstation that referenced this pull request May 17, 2024
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 added a commit to freedomofpress/securedrop-workstation that referenced this pull request May 17, 2024
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)
* Remove the sd-proxy.yaml configuration (now read from QubesDB)
* Fix RPC test, as the securedrop-proxy now returns a non-zero
  exit code on malformed input.

Fixes #1026.
legoktm added a commit to freedomofpress/securedrop-workstation that referenced this pull request May 17, 2024
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)
* Remove the sd-proxy.yaml configuration (now read from QubesDB)
* Fix RPC test, as the securedrop-proxy now returns a non-zero
  exit code on malformed input.

Fixes #1026.
legoktm added a commit to freedomofpress/securedrop-workstation that referenced this pull request May 17, 2024
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)
* Remove the sd-proxy.yaml configuration (now read from QubesDB)
* Fix RPC test, as the securedrop-proxy now returns a non-zero
  exit code on malformed input.

Fixes #1026.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment