Feat/dpop by Avantol13 · Pull Request #314 · uc-cdis/gen3sdk-python

Avantol13 · 2026-08-20T21:10:08Z

Depends on:

Feat/dpop authutils#113
Feat/dpop fence#1372
⚠️ Merge those first and then update authutils here!

New Features

DPoP support (CLI & SDK)
Nextflow run support utilizing DPoP (CLI & SDK)

Breaking Changes

3.13 only due to required updates from upstream dependencies, including authutils and other Gen3 libraries (this matches the Python version used and required by other Gen3 services)

Bug Fixes

Improvements

Unit tests parallelized, down to ~7s from ~40s locally (w/ the new tests included) - looks like existing runs would take ~30s before these new tests and on this branch they take ~17s, so still almost half the time with many new tests added
Rip out coupling to indexd by actually mocking in unit tests

Dependency updates

Deployment changes

…elize tests

github-actions · 2026-08-20T21:11:20Z

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

github-actions · 2026-08-20T21:37:25Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_dbgap.py	4	0	1	5
tests/test_ras_passport.py	0	0	2	2
tests/test_data_upload.py	8	0	1	9
tests/test_graph_submit_and_query.py	12	1	1	14
tests/test_presigned_url.py	7	0	0	7
tests/test_centralized_auth.py	5	0	0	5
tests/test_google_data_access.py	1	0	0	1
tests/test_audit_service.py	1	0	0	1
tests/test_drs_endpoint.py	2	0	0	2
tests/test_gen3_sdk.py	1	0	0	1
TOTAL	41	1	5	47

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

paulineribeyre · 2026-09-10T20:34:16Z

+        """
+        is_s3 = service == _SERVICE_S3
+        not_forwarded = (
+            _HEADERS_NOT_FORWARDED_TO_S3 if is_s3 else _HEADERS_NOT_FORWARDED


This is a bit redundant since _HEADERS_NOT_FORWARDED_TO_S3 is just _HEADERS_NOT_FORWARDED except "authorization", and the "authorization" header is overwritten anyway a couple lines later if "not is_s3".

Could we just remove "authorization" from _HEADERS_NOT_FORWARDED and get rid of _HEADERS_NOT_FORWARDED_TO_S3?

yeah, good call. I cleaned it up and added a check b/c of case-sensitivity causing duplicates

paulineribeyre · 2026-09-10T20:46:41Z

+        else:
+            logging.warning(
+                f"Refusing to proxy {path}: only /ga4gh/tes and /s3 paths are "
+                "proxied. Check the endpoints your pipeline is configured with."


The paths start with either /ga4gh/tes/v1 or /workflows, see config here.

Plus, the S3 endpoint is also exposed at the app root (see here).

So a request to Gen3 S3 could start with /workflows or /workflows/s3 or /ga4gh/tes/v1 or /ga4gh/tes/v1/s3 (although the last 2 are not documented or used). But never just /s3.

I'm not sure what a reliable way to identify S3 requests while maintaining root S3 endpoint support would be. We could list all the non-S3 routes but that's not very future-proof.

For now to unblock my testing, i made this change, which assumes S3 requests are non-root.

Edit: I see the proxy is accepting /s3 requests and forwarding them to /workflows/s3, and DPOP_PROTECTED_PATHS in gen3-workflow matches that, so maybe I misunderstood the intent. We can discuss it when you're back!

the proxy itself has its own endpoints and then translates those to the real Gen3 Workflow, and I'd rather keep them separate and have the mapping in the proxy itself. We can control in the Nextflow config what local proxy endpoint to hit for S3 and TES respectively (and the auto-generated config should already be doing that). e.g. this should've worked out of the box

in your edit: /ga4gh/tes and /s3 are the proxy's own local namespace, not commons paths. the commons paths only appear in TES_ENDPOINT / S3_ENDPOINT, and the generated nextflow config points nextflow at the local ones.

round trip for S3: nextflow hits 127.0.0.1:port/s3/bucket/key, proxy strips /s3 and appends to {commons}/workflows/s3, signs htu over {commons}/workflows/s3/bucket/key.

gen3-workflow should rebuild that same string to check.

What was the config that sent /workflows/... at the proxy? e.g. why didn't what was written work out of the box?

if it was a hand-written pipeline config or the auto-generated config... we could fix that instead.

re: your changes, if we want to keep this and support commons paths matching locally - I think it needs some updates either way. It looks like this will happen:

/workflows/s3/bucket/key.txt -> ('https://cx/workflows/s3/bucket/key.txt', 's3') /workflows/bucket/key.txt -> ('https://cx/ga4gh/tes/bucket/key.txt', 'tes') /s3/bucket/key.txt -> None

the middle one is the root-mounted S3 case you raised - after /workflows/ matches, anything that isn't /s3/ falls through to the TES base, so it goes to
{commons}/ga4gh/tes/<bucket>/<key> and since the service isn't s3 we replace the
SigV4 header with Authorization: DPoP <token>. and the last one is what the
generated config sends, so generated-config runs 404.

But... I still am not convinced we need to change this. The local proxy and nextflow config can be opinionated - we can choose to only support /s3 -> /workflows/s3 instead of all 4 options the service itself supports.

My integration tests do not use the path that updates the config automatically. I see that I misconfigured aws.client.endpoint (<proxy>/workflows/s3 instead of <proxy>/s3), that's probably where the issue came from. I'll fix that and test - no need to change anything if that works 👍

Maybe a bit of documentation/docstring about this endpoint mapping would be nice though, so the next reader isn't confused like I was?

Ok that was only part of the issue - all the existing TES tests were also pointing at /workflows/s3, which i didn't change when i updated them to use the dpop proxy.

Co-authored-by: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com>

github-actions · 2026-09-14T16:12:06Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_graph_submit_and_query.py	12	1	1	14
tests/test_data_upload.py	8	0	1	9
tests/test_presigned_url.py	8	0	0	8
tests/test_centralized_auth.py	5	0	0	5
tests/test_audit_service.py	1	0	0	1
tests/test_dbgap.py	4	0	1	5
tests/test_google_data_access.py	1	0	0	1
tests/test_gen3_sdk.py	1	0	0	1
tests/test_ras_passport.py	0	0	2	2
TOTAL	40	1	5	46

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

github-actions · 2026-09-14T17:21:40Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_graph_submit_and_query.py	12	1	1	14
tests/test_data_upload.py	8	0	1	9
tests/test_presigned_url.py	8	0	0	8
tests/test_centralized_auth.py	5	0	0	5
tests/test_dbgap.py	4	0	1	5
tests/test_google_data_access.py	1	0	0	1
tests/test_audit_service.py	1	0	0	1
tests/test_gen3_sdk.py	1	0	0	1
tests/test_ras_passport.py	0	0	2	2
TOTAL	40	1	5	46

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

… match the known env vars for tokens so other processes/users cannot just hit it without also sending auth

nss10

Great work. Left some comments, questions and suggestions.

nss10 · 2026-09-27T22:17:31Z

+        if httpx2.URL(endpoint).host != commons_host:
+            raise Gen3AuthError(
+                f"{option} is {endpoint}, which is not on {commons_host} - the "
+                "commons that issued your credentials. The task token is only "
+                "valid there, so it will not be sent anywhere else."
+            )


The there and anywhere else in the error message seemed a little ambiguous while reading. Could we try something like this ⬇

Suggested change

if httpx2.URL(endpoint).host != commons_host:

raise Gen3AuthError(

f"{option} is {endpoint}, which is not on {commons_host} - the "

"commons that issued your credentials. The task token is only "

"valid there, so it will not be sent anywhere else."

)

if httpx2.URL(endpoint).host != commons_host:

raise Gen3AuthError(

f"{option} is {endpoint}, which is not on {commons_host} - the "

"commons that issued your credentials. For security, the task "

f"token is only sent to the host that issued it. Change {option} "

f"to a URL on {commons_host}."

)

nss10 · 2026-09-28T01:18:59Z

+    # A client acting on behalf of a user appends the user ID to its token.
+    return candidate.split(";userId=")[0] or None


@pauline -- Do we still need this here? Now that we are moving away from client accessing S3 bucket on users' behalf?

nss10 · 2026-09-28T01:26:55Z

+        """
+        Route a path to its upstream base, or refuse to route it at all.
+
+        Only TES and S3 traffic belongs on this proxy, so the two prefixes are


The /ga4gh/tes and /s3 prefixes are hardcoded in _resolve_upstream_url and referenced in several comments. If a third service is added later, those will need to be found and updated together. Worth considering whether the route table should be data-driven (e.g. a dict of prefix → service) so adding a service is one change in one place — but fine to defer if the two-service assumption and extensibility is out of scope for now.

nss10 · 2026-09-28T03:44:08Z

 gen3users = "*"
+joserfc = ">=1.7.3"
+
+authutils = {git = "https://github.com/uc-cdis/authutils.git", rev = "feat/dpop"}


Reminder to pin it to master, after authutils PR is merged

nss10 · 2026-09-28T04:24:08Z

+        self.records: dict[str, dict] = {}
+        self.bundles: dict[str, dict] = {}


Should reads and writes to these dicts be thread safe? Since mutliple threads could "technically" write in parallel. I don't think it is an issue with the current use case though.

nss10 · 2026-09-28T22:26:07Z

+    def test_no_requested_lifetime_skips_the_check(self, ec_key, requests_mock):
+        """Without an explicit lifetime the server picks one, so nothing is checked."""
+        token, _ = _exchange(ec_key, api_key=_api_key_expiring_in(-60))
+
+        assert token == TASK_TOKEN
+        assert requests_mock.called


This seems a little off to me — maybe I'm missing something. There is no restriction on fetching a TASK_TOKEN with an already-expired API key as long as no explicit task_token_expiration is provided?

At dpop.py#L920-921 we simply skip the check when task_token_expiration is None. Is this a missed edge case, or are we intentionally deferring to the server to reject the expired API key?

nss10 · 2026-09-28T22:54:00Z

+            method, upstream_url, scope.get("headers", []), service
+        )
+
+        with tempfile.SpooledTemporaryFile(max_size=_MAX_BODY_SIZE_IN_MEMORY) as body:


I know buffering the body is necessary to support nonce retries, but I'm concerned about the disk space implications for large uploads. If a user uploads a 5 GB file, the proxy needs 5 GB of free space in /tmp for the duration of the transfer. With parallel uploads, that multiplies — a user could unknowingly need tens of gigabytes of temporary storage just to run the proxy.

This seems worth documenting. maybe in docs/howto/nextflow.md ?

nss10 · 2026-09-28T22:56:24Z

+
+        with proxy.get("/s3/bucket/big", stream=True, timeout=120) as response:
+            content_length = response.headers["content-length"]
+            received = sum(len(chunk) for chunk in response.iter_content(65536))


Probably being nitpicky, but can we write 64*1024 instead of 65536

Suggested change

received = sum(len(chunk) for chunk in response.iter_content(65536))

received = sum(len(chunk) for chunk in response.iter_content(64 * 1024))

nss10 · 2026-09-28T23:16:18Z

+
+        _refusal(ec_key)
+
+        assert requests_mock.call_count == 3


Probably 3 is implicitly understood, but can we have _MAX_NONCE_RETRIES + 1 to be cleaner?

nss10 · 2026-09-28T23:16:49Z

+
+        assert response.status_code == 401
+        assert response.json() == {"error": "use_dpop_nonce"}
+        assert proxy.upstream.nonce_challenges_sent == 3


Same as above --

Suggested change

assert proxy.upstream.nonce_challenges_sent == 3

assert proxy.upstream.nonce_challenges_sent == _MAX_NONCE_RETRIES + 1

Avantol13 and others added 5 commits August 5, 2026 14:45

feat(dpop): initial dpop support, no tests yet

3523d8b

Apply automatic documentation changes

dea09cf

feat(tests): dpop tests, rip out indexd dependency by mocking, parall…

6b2dc66

…elize tests

Apply automatic documentation changes

a8c56a2

Merge branch 'master' into feat/dpop

104eaff

paulineribeyre reviewed Sep 10, 2026

View reviewed changes

Avantol13 and others added 2 commits September 14, 2026 10:32

Update docs/howto/nextflow.md

a3b1c10

Co-authored-by: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com>

Merge branch 'master' into feat/dpop

c1642b2

feat(headers): cleanup header forwarding

0064eba

fix(dpop): make the proxy itself require task token to be present and…

99bbea4

… match the known env vars for tokens so other processes/users cannot just hit it without also sending auth

paulineribeyre requested review from nss10 and paulineribeyre September 16, 2026 16:15

nss10 reviewed Sep 28, 2026

View reviewed changes

		# A client acting on behalf of a user appends the user ID to its token.
		return candidate.split(";userId=")[0] or None

		self.records: dict[str, dict] = {}
		self.bundles: dict[str, dict] = {}

	received = sum(len(chunk) for chunk in response.iter_content(65536))
	received = sum(len(chunk) for chunk in response.iter_content(64 * 1024))

	assert proxy.upstream.nonce_challenges_sent == 3
	assert proxy.upstream.nonce_challenges_sent == _MAX_NONCE_RETRIES + 1

Conversation

Avantol13 commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

github-actions Bot commented Aug 20, 2026

Integration Tests

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulineribeyre Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 14, 2026

Integration Tests

Uh oh!

github-actions Bot commented Sep 14, 2026

Integration Tests

Uh oh!

nss10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nss10 Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Avantol13 commented Aug 20, 2026 •

edited

Loading

paulineribeyre Sep 10, 2026 •

edited

Loading

nss10 Sep 28, 2026 •

edited

Loading