Support HTTP URLs in file fetcher and system-test instrumentation#22044
Support HTTP URLs in file fetcher and system-test instrumentation#22044
Conversation
Confidential workflows register HTTP URLs for the enclave. The file fetcher extracts the filename for local testing. Add relay capability constant and timing logs for system tests.
product-security-plaid-production
Bot
requested review from
chudilka1 and
prashantkumar1982
|
👋 nadahalli, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM (touches workflow artifact fetching logic + path validation in core syncer; warrants careful review)
This PR aligns local confidential-workflow testing behavior with enclave behavior by allowing the syncer’s file fetcher to accept http(s) URLs (mapping them to local filenames), and adds system-test capability/test instrumentation updates.
Changes:
- Allow file fetchers (syncer v1 + v2) to accept
http(s)request URLs by extracting the basename for local disk reads. - Add
ConfidentialRelayCapabilityand add timing instrumentation for node metadata + node key generation in system tests. - Fix mock capability type to ACTION and add a changeset entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
core/services/workflows/syncer/fetcher.go |
Accept http(s) URLs in file fetcher by converting URL path to basename. |
core/services/workflows/syncer/v2/fetcher.go |
Same as above for v2 syncer fetcher. |
system-tests/lib/cre/types.go |
Add capability constant and timing instrumentation helpers/log fields. |
system-tests/lib/cre/features/mock/mock.go |
Fix mock capability type to ACTION. |
.changeset/cw-fetcher-and-system-tests.md |
Record patch-level change summary. |
| } | ||
|
|
||
| func roundSeconds(d time.Duration) float64 { | ||
| return float64(d.Milliseconds()) / 1000.0 |
There was a problem hiding this comment.
roundSeconds doesn’t actually round, and it truncates sub-millisecond precision by using Milliseconds(). Consider either renaming to reflect what it returns (e.g., secondsFromDuration) or implementing actual rounding (or just using d.Seconds() if you don’t need rounding).
| return float64(d.Milliseconds()) / 1000.0 | |
| return d.Seconds() |
There was a problem hiding this comment.
Inlined Seconds() and removed the helper in 1072792.
| Int("evm_chains", len(input.EVMChainIDs)). | ||
| Int("solana_chains", len(input.SolanaChainIDs)). | ||
| Bool("imported", input.ImportedSecrets != ""). | ||
| Float64("duration_s", roundSeconds(time.Since(start))). |
There was a problem hiding this comment.
This log line is only reached when ImportedSecrets == "", so the Bool("imported", input.ImportedSecrets != "") field will always be false and can be misleading. Consider either removing the field, hardcoding it to false here, or adding a corresponding log (and duration) in the early-return ImportedSecrets branch so both paths are instrumented consistently.
nadahalli
force-pushed
the
tejaswi/cw-fetcher-and-system-tests
branch
from
7594339 to
396a390
Compare
| func roundSeconds(d time.Duration) float64 { | ||
| return d.Seconds() |
There was a problem hiding this comment.
Could we just call Seconds() inline instead?
There was a problem hiding this comment.
Done in 1072792. Inlined time.Since(...).Seconds() at both call sites and deleted the helper.
|
| } | ||
| // Confidential workflows register with HTTP URLs (for the enclave). | ||
| // Extract the filename so the file fetcher can find the local copy. | ||
| if u.Scheme == "http" || u.Scheme == "https" { |
There was a problem hiding this comment.
if it's "confidential" shall it only allow https ?
There was a problem hiding this comment.
Confidential means that the workflow runs in TEE's. If the user wants to do http requests from TEE's, why stop them? They know what they are doing.
There was a problem hiding this comment.
Ah, this is the fetcher URL. It can be http because we check the returned binary's hash against the hash that was sent in the request - so, the server that hosts the wasm binary cannot send back bad things.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Summary
ConfidentialRelayCapabilityconstant for E2E test support.