-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: read proxy origin from QubesDB when available; otherwise environment #1895
Conversation
We identified multiple features that the current implementation of the proxy is unable to support (e.g. progress reporting, resuming downloads), necessitating a new protocol dubbed "v2". We are taking the opportunity to rewrite this component in Rust. Normal operation is mostly the same, input is received as a JSON blob (parsed and validated by serde) and output in our custom JSON response format. The `url` crate (from servo) is used to assemble and validate the target URL, and `reqwest` fires off the HTTP request. Downloads are now streamed back to the client over stdout, and metadata passed over stderr. Tests and further integration will happen in follow-up commits. Refs #1678. Co-authored-by: Kunal Mehta <[email protected]>
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).
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.
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.
Co-authored-by: Kunal Mehta <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like where this is going! It worked correctly when I set the qubesdb value, but when I first tried to run it with nothing set, it SIGSEV'd (instead of gracefully erroring):
$ cargo run --features qubesdb
Finished dev [unoptimized + debuginfo] target(s) in 0.05s
Running `/home/user/.cargo/target/debug/securedrop-proxy`
fish: Job 1, 'cargo run --features qubesdb' terminated by signal SIGSEGV (Address boundary error)
We may also want to move all the config-related code into a separate file to keep the qubesdb/env/etc. related code separate from the HTTP-over-JSON stuff.
Since qdb_read() returns a pointer, not a byte-slice, we can't rely on the nul-byte handling of the CStr::from_bytes*() functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that CI is failing because the build.rs script/bindgen is unconditionally trying to link against qubesdb, regardless of whether we enabled the feature. And then I pointed out two other things inline.
Overall I think this is ready to rebase into the main proxy-rusting branch. We do need to figure out a solution for integration testing though, which I'm guessing requires integrating into the new Qubes CI stuff...
a8fb9cf
to
ca3f0c5
Compare
f4dac15
to
b132932
Compare
Okay, I think we've got it! Thanks for iterating on this with me, @legoktm. Pushing to |
Status
Work in progress
Description
On top of #1718, adds the
qubesdb
feature to the v2 proxy to read the QubesDB key/vm-config/SD_PROXY_ORIGIN
.Test Plan
TK, but roughly:
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:
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:
main
and confirmed that the migration is self-contained and applies cleanlymain
and would like the reviewer to do so