restrict secret sharing #2261
restrict secret sharing #2261
Conversation
go/enclave/enclave_admin_service.go
Outdated
| bsr := &common.BlockSubmissionResponse{RollupMetadata: rollupMetadata} | ||
|
|
||
| // in phase 1, only if the enclave is a sequencer, it can respond to shared secret requests | ||
| if e.isBackupSequencer(ctx) || e.isActiveSequencer(ctx) || e.sharedSecretService.IsGenesis() { |
There was a problem hiding this comment.
I guess in HA scenarios this might lead to the host responding to the secret request twice (for each enclave) if we're not careful. I can make sure that's covered in a followup.
There was a problem hiding this comment.
this PR is just a security check to make sure someone who has a validator can freely let stealth nodes in. Which we don't want in phase 1
@StefanIliev545 that's supposed to be covered already, the enclave won't let itself go into sequencer state until it's seen its enclave ID promoted on the L1 (and if it sees it revoked it should undo that). We should make sure that's robust because a lot depends on it, including this. |
| err := authenticateFrom(builder.VK, builder.From) | ||
| if err != nil { | ||
| builder.Err = err | ||
| return nil //nolint:nilerr | ||
| } | ||
|
|
||
| latest := gethrpc.LatestBlockNumber | ||
| s, err := rpc.registry.GetBatchState(builder.ctx, gethrpc.BlockNumberOrHash{BlockNumber: &latest}) | ||
| s, err := rpc.registry.GetBatchState(builder.ctx, *builder.Param) |
There was a problem hiding this comment.
there was a bug here. It ignored the passed in blockNumber
| if err != nil { | ||
| return fmt.Errorf("failed to get host batch number (%s): %w", rpcAddress, err) | ||
| } | ||
| if height <= common.L2SysContractGenesisSeqNo+1 { |
There was a problem hiding this comment.
this stuff is to avoid rpc calls before the node has received some batches
We should still have the management contract enforce whatever possible; Our security shouldn't boil down to one spot. |
Sorry, I misunderstood your first comment, thought you were saying the enclave state couldn't be trusted for that and should go back to L1 data somehow at that point. I agree be good to have check in the contract. |
There was a problem hiding this comment.
LGTM but I think maybe worth addressing Stefan's point in this PR?
I.e. that we should probably also add the check in RespondNetworkSecret in ManagementContract.sol, like:
// ensure attester is a permissioned sequencer enclave
bool isEnclSequencer = sequencerEnclave[attesterID];
require(isEnclSequencer, "responding attester is not a sequencer");
```
or something?
There was a problem hiding this comment.
lgtm, @StefanIliev545 to do the contract-side verification separately
…_share # Conflicts: # go.mod
Why this change is needed
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks