Conversation
product-security-plaid-production
Bot
requested review from
chudilka1 and
kalverra
August 12, 2026 16:41
Contributor
|
✅ No conflicts with other open PRs targeting |
Contributor
|
I see you updated files related to
|
|
|
Contributor
CORA - Pending Reviewers
Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
|
se3000
requested changes
Aug 18, 2026
Comment on lines
+334
to
+335
| # to that repo's fake-enclave runner. Check it out at the same revision the | ||
| # plugin is built from so the capability and the enclave app always match. |
Contributor
There was a problem hiding this comment.
Would it be worth making sure that the two revisions never diverge? Could read it from system-tests/tests/go.mod instead of what is listed as the public plugin.
Comment on lines
+92
to
+93
|
|
||
| t.Setenv("REQUIRE_BFT_QUORUM", "true") |
Contributor
There was a problem hiding this comment.
Suggested change
| t.Setenv("REQUIRE_BFT_QUORUM", "true") | |
| // Both env vars below configure the enclave *host servers*, not this repo: | |
| // the harness launches them with this process's environment inherited | |
| // (testhelpers.MustSetupEnclaveWithEnv appends to os.Environ()) | |
| // REQUIRE_BFT_QUORUM makes each host demand a 2f+1 BFT supermajority of node | |
| // signatures instead of f+1 (enclave/nitro/host), matching the relay's | |
| // requireBFTQuorum = true in the topology TOML. | |
| t.Setenv("REQUIRE_BFT_QUORUM", "true") |
| t.Helper() | ||
|
|
||
| root := os.Getenv("CONFIDENTIAL_COMPUTE_ROOT") | ||
| require.NotEmpty(t, root, |
Contributor
There was a problem hiding this comment.
Maybe make this a t.Skip instead so it doesn't fail locally for people?
| return fmt.Sprintf("%s://%s:%d%s", incoming.Protocol, host, incoming.ExternalPort, incoming.Path) | ||
| } | ||
|
|
||
| // injectVaultPublicKey writes the vault DON's DKG public key and threshold into |
Contributor
There was a problem hiding this comment.
Comment looks out of place. Move it to above injectVaultPublicKey.
|
|
||
| // DefaultGatewayWhitelistConfig builds the Gateway Connector's outbound allowlist: | ||
| // the caller's extra ports plus the fake service ports the config declares. Shared | ||
| // by `cre env start` and by tests that call StartCLIEnvironment directly, so both |
Contributor
There was a problem hiding this comment.
Seems no one one outside this file calls this on this branch, so we could make it private and update the comment to remove saying it is shared "and by tests"
Comment on lines
+306
to
+327
| workers, err := testEnv.Dons.MustWorkflowDON().Workers() | ||
| require.NoError(t, err, "failed to get worker nodes from topology") | ||
| require.NotEmpty(t, workers, "workflow DON has no worker nodes") | ||
|
|
||
| signers := make([][]byte, 0, len(workers)) | ||
| for _, node := range workers { | ||
| signers = append(signers, node.Keys.P2PKey.PeerID[:]) | ||
| } | ||
|
|
||
| masterPublicKey, err := hex.DecodeString(vaultPublicKey) | ||
| require.NoError(t, err, "failed to hex-decode vault public key") | ||
|
|
||
| // don.F for an N-node DON is N/3; the enclave's own F/T is 2*don.F + 1. | ||
| donF := uint32(len(workers) / 3) //nolint:gosec // G115: the worker count comes from the topology | ||
| quorum := 2*donF + 1 | ||
|
|
||
| config := cctypes.EnclaveConfig{ | ||
| Signers: signers, | ||
| MasterPublicKey: masterPublicKey, | ||
| T: quorum, | ||
| F: quorum, | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| workers, err := testEnv.Dons.MustWorkflowDON().Workers() | |
| require.NoError(t, err, "failed to get worker nodes from topology") | |
| require.NotEmpty(t, workers, "workflow DON has no worker nodes") | |
| signers := make([][]byte, 0, len(workers)) | |
| for _, node := range workers { | |
| signers = append(signers, node.Keys.P2PKey.PeerID[:]) | |
| } | |
| masterPublicKey, err := hex.DecodeString(vaultPublicKey) | |
| require.NoError(t, err, "failed to hex-decode vault public key") | |
| // don.F for an N-node DON is N/3; the enclave's own F/T is 2*don.F + 1. | |
| donF := uint32(len(workers) / 3) //nolint:gosec // G115: the worker count comes from the topology | |
| quorum := 2*donF + 1 | |
| config := cctypes.EnclaveConfig{ | |
| Signers: signers, | |
| MasterPublicKey: masterPublicKey, | |
| T: quorum, | |
| F: quorum, | |
| } | |
| don := testEnv.Dons.MustWorkflowDON() | |
| workers, err := don.Workers() | |
| require.NoError(t, err, "failed to get worker nodes from topology") | |
| require.NotEmpty(t, workers, "workflow DON has no worker nodes") | |
| signers := make([][]byte, 0, len(workers)) | |
| for _, node := range workers { | |
| signers = append(signers, node.Keys.P2PKey.PeerID[:]) | |
| } | |
| masterPublicKey, err := hex.DecodeString(vaultPublicKey) | |
| require.NoError(t, err, "failed to hex-decode vault public key") | |
| // Quorum tracks the DON's registered fault tolerance (Don.F, computed as | |
| // (workers-1)/3 in NewDON), not a re-derivation from the worker count: the | |
| // two diverge for e.g. 6-node DONs, and the enclave would then demand more | |
| // signatures than the DON can produce. | |
| quorum := 2*uint32(don.F) + 1 | |
| config := cctypes.EnclaveConfig{ | |
| Signers: signers, | |
| MasterPublicKey: masterPublicKey, | |
| T: quorum, | |
| F: quorum, | |
| } |
Dynamically determine F instead of assuming it's always 1.
| } | ||
| } | ||
|
|
||
| // UseFakeEnclaveForTest reports whether the harness selected fake enclaves. |
Contributor
There was a problem hiding this comment.
Nit: Necessity of this one line function is questionable:
UseFakeEnclaveForTest() (helpers.go:360) is a pure one-line wrapper over testhelpers.UseFakeEnclave(), and the test file already calls testhelpers.UseFakeEnclave() directly (test.go:79). Drop the wrapper, call the source in both places.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an end-to-end test for confidential workflows, using the enclave test harness from chainlink-confidential-compute.