-
Notifications
You must be signed in to change notification settings - Fork 19
fix: address MEDIUM review findings (light witness resilience, proxy CORS, threshold warning) #1382
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
base: v1.6-dev
Are you sure you want to change the base?
Changes from all commits
e892cb9
7052c97
edc50c6
f4507f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -833,22 +833,26 @@ func (c *Client) compareFirstHeaderWithWitnesses(ctx context.Context, h *types.S | |
| go c.compareNewHeaderWithWitness(compareCtx, errc, h, witness, i) | ||
| } | ||
|
|
||
| // Separate counters for the two outcomes we care about: | ||
| // - conflictingWitnesses: witnesses whose header disagrees with the primary's. | ||
| // - corroborating: witnesses whose header matches the primary's (a healthy | ||
| // primary is expected to be corroborated by at least one other party). | ||
| witnessesToRemove := make([]int, 0, len(c.witnesses)) | ||
| conflictingWitnesses := make([]int, 0, len(c.witnesses)) | ||
| corroborating := 0 | ||
|
|
||
| // handle errors from the header comparisons as they come in | ||
| for i := 0; i < cap(errc); i++ { | ||
| err := <-errc | ||
|
|
||
| switch e := err.(type) { | ||
| case nil: | ||
| corroborating++ | ||
| continue | ||
| case errConflictingHeaders: | ||
| c.logger.Error(fmt.Sprintf("witness #%d has a different header. Please check primary is correct and"+ | ||
| " remove witness. Otherwise, use the different primary", e.WitnessIndex), "witness", | ||
| c.witnesses[e.WitnessIndex]) | ||
| // The primary and a witness disagree; keep the dissenting witness, as | ||
| // the primary may be the faulty party, and fail the comparison below. | ||
| conflictingWitnesses = append(conflictingWitnesses, e.WitnessIndex) | ||
| case errBadWitness: | ||
| // If witness sent us an invalid header, then remove it | ||
|
|
@@ -881,12 +885,32 @@ func (c *Client) compareFirstHeaderWithWitnesses(ctx context.Context, h *types.S | |
| return err | ||
| } | ||
|
|
||
| // A witness presenting a header that conflicts with the primary's means the | ||
| // primary's header cannot be trusted: fail the comparison. | ||
| if len(conflictingWitnesses) > 0 { | ||
| // A single conflicting witness must not be allowed to break verification: a | ||
| // lone dissenter is far more likely to be a faulty/compromised witness than a | ||
| // sign that the primary is malicious, and failing here would let one bad | ||
| // witness denial-of-service an otherwise-healthy light client. We therefore | ||
| // treat a single conflicting witness as faulty and remove it, exactly like | ||
| // errBadWitness. We still FAIL (preserving the safety-over-availability | ||
| // guarantee) only when the conflict is corroborated: | ||
| // - two or more witnesses disagree with the primary (genuine fork / primary | ||
| // likely faulty), OR | ||
| // - at least one witness conflicts and NO witness corroborates the primary | ||
| // (we cannot establish trust in either party, so we refuse to proceed). | ||
| if len(conflictingWitnesses) > 1 || (len(conflictingWitnesses) == 1 && corroborating == 0) { | ||
| return fmt.Errorf("%w: witnesses %v", ErrConflictingWitnessHeader, conflictingWitnesses) | ||
| } | ||
|
|
||
| // Exactly one conflicting witness with at least one corroborating witness: | ||
| // drop the dissenter and continue. | ||
| if len(conflictingWitnesses) == 1 { | ||
| c.logger.Warn("single witness conflicts with the primary but others corroborate it; "+ | ||
| "removing the dissenting witness rather than failing verification", | ||
| "witness", c.witnesses[conflictingWitnesses[0]]) | ||
| if err := c.removeWitnesses(conflictingWitnesses); err != nil { | ||
|
lklimek marked this conversation as resolved.
Comment on lines
885
to
+909
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking: Sequential removals use stale witness indices and can panic
source: ['codex'] |
||
| return err | ||
| } | ||
| } | ||
|
Comment on lines
+888
to
+912
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking: A colluding primary and witness can evict the sole honest witness This changes the light client from the documented any-honest-witness model to an unenforced provider-majority model. If a malicious primary serves a validly signed non-canonical fork, one colluding witness returns the same header, and the sole honest witness returns the canonical header, the counters are one corroborating and one conflicting witness. The code removes the honest witness and then persists the primary's fork. Witness comparison exists specifically to detect an otherwise valid conflicting fork after validator trust assumptions have failed, and source: ['codex'] |
||
|
|
||
| return nil | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,14 @@ type Proxy struct { | |
| Client *lrpc.Client | ||
| Logger log.Logger | ||
| Listener net.Listener | ||
|
|
||
| // AllowedOrigins restricts which web origins may open a WebSocket to the | ||
| // proxy. It mirrors rpc.Config.CORSAllowedOrigins semantics. An empty/nil | ||
| // slice enforces the zero-trust default: only same-host origins and | ||
| // Origin-less (non-browser) clients are accepted. Operators that must permit | ||
| // specific browser origins set this explicitly; a "*" entry allows any | ||
| // origin (discouraged). | ||
| AllowedOrigins []string | ||
| } | ||
|
|
||
| // NewProxy creates the struct used to run an HTTP server for serving light | ||
|
|
@@ -39,10 +47,11 @@ func NewProxy( | |
| } | ||
|
|
||
| return &Proxy{ | ||
| Addr: listenAddr, | ||
| Config: config, | ||
| Client: lrpc.NewClient(logger, rpcClient, lightClient, opts...), | ||
| Logger: logger, | ||
| Addr: listenAddr, | ||
| Config: config, | ||
| Client: lrpc.NewClient(logger, rpcClient, lightClient, opts...), | ||
| Logger: logger, | ||
| AllowedOrigins: nil, | ||
| }, nil | ||
| } | ||
|
|
||
|
|
@@ -105,13 +114,13 @@ func (p *Proxy) listen(ctx context.Context) (net.Listener, *http.ServeMux, error | |
| }), | ||
| rpcserver.ReadLimit(p.Config.MaxBodyBytes), | ||
| ) | ||
| // Preserve the light proxy's historic permissive websocket origin policy: | ||
| // allow every origin. The proxy is configured via rpcserver.Config, which | ||
| // exposes no CORS allow-list knob, so the WebsocketManager default | ||
| // (same-host/Origin-less only) would silently reject browser clients that | ||
| // worked before. Operators that need origin restrictions are expected to | ||
| // enforce AuthN/AuthZ in front of the proxy. | ||
| wm.CheckOrigin = func(*http.Request) bool { return true } | ||
| // WebSocket origin policy is zero-trust by default: OriginChecker with no | ||
| // allow-list accepts only same-host origins and Origin-less (non-browser) | ||
| // clients, rejecting cross-origin browser requests. Operators that need to | ||
| // permit specific browser origins set Proxy.AllowedOrigins (mirrors | ||
| // rpc.Config.CORSAllowedOrigins); a "*" entry allows any origin but is | ||
| // discouraged. This replaces the previous blanket allow-all policy. | ||
| wm.CheckOrigin = rpcserver.OriginChecker(p.Logger, p.AllowedOrigins) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking: Same-host fallback permits DNS-rebinding WebSocket access
source: ['codex'] |
||
|
|
||
| mux.HandleFunc("/websocket", wm.WebsocketHandler) | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.