Conversation
There was a problem hiding this comment.
Pull request overview
Extends the rpki stage with optional ASPA (AS_PATH) validation alongside existing ROV, and hardens/adjusts the HTTP API and websocket stage behavior (auth, origin checks, limits, timeouts), plus a few correctness fixes and dependency bumps.
Changes:
- Add ASPA path validation (VALID/UNKNOWN/INVALID) and migrate RPKI data handling from ROAs to VRPs + ASPAs (RTR + file loader + metrics + docs).
- Make HTTP API auth fail-closed by default, add flexible credential sources, and adjust pprof mounting/serving behavior.
- Harden websocket connections (same-origin default, read limits, constant-time auth compare) and fix a withdrawal counting bug in
limit.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
stages/websocket.go |
Adds constant-time auth compare, same-origin checks, read limits, and server ReadHeaderTimeout. |
stages/speaker.go |
Exposes remote ASN/hold validation flags and wires them into speaker options. |
stages/rpki/validate.go |
Refactors ROV validation against VRP caches and integrates optional ASPA validation. |
stages/rpki/validate_test.go |
Updates unit tests to VRP terminology/types and new ROV status constants. |
stages/rpki/validate_msg_test.go |
Adds message-level tests covering ROV withdrawal behavior and ASPA edge cases. |
stages/rpki/rtr.go |
Replaces stayrtr client with bgpfix/rtr client, adding ASPA callbacks and new session handling. |
stages/rpki/rpki.go |
Adds ASPA config/state/metrics, renames ROA→VRP structures, updates flags and HTTP status output. |
stages/rpki/next.go |
Implements VRP/ASPA incremental “next” cache management and validation of maxLength. |
stages/rpki/next_test.go |
Updates “next” cache tests for VRP naming/types and adds invalid maxLength coverage. |
stages/rpki/file.go |
Enhances local file loader to support VRPs+ASPAs (JSON) and improves input validation/trim. |
stages/rpki/file_test.go |
Updates file parser tests for VRP naming and expected counts. |
stages/rpki/aspa.go |
Implements ASPA authorization + valley-free AS_PATH verification and enforcement actions. |
stages/rpki/aspa_test.go |
Adds unit tests for ASPA authorization/verification and role parsing helpers. |
stages/limit.go |
Fixes withdrawal-counting bug in checkUnreach. |
pkg/util/net.go |
Adds documentation to ConnPublish. |
core/http.go |
Adds HTTP Basic auth middleware, credential sourcing, pprof split-server option, and hardening timeouts. |
core/config.go |
Introduces --http-auth, --http-open, and new string-based --pprof flag. |
core/bgpipe.go |
Tracks whether pprof is mounted on the main HTTP server for dashboard links. |
core/attach.go |
Updates HTTP stage attachment call after attachHTTPStages signature change. |
docs/stages/rpki.md |
Documents ASPA behavior, new flags, file formats, tags, and metrics. |
go.mod |
Bumps Go version/toolchain and updates dependencies (adds testify direct dep). |
go.sum |
Updates module checksums to match dependency changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/http.go
Outdated
| fh, err := os.Open(v) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| buf := make([]byte, 128) | ||
| n, err := fh.Read(buf) | ||
| fh.Close() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("file %s: %w", v, err) | ||
| } | ||
| line, _, _ := bytes.Cut(buf[:n], []byte{'\n'}) |
There was a problem hiding this comment.
Credential file reads will commonly return err==io.EOF after a successful short read, but this code treats any non-nil error as fatal. As a result, using --http-auth /path (and similar) can fail even when the file is valid. Consider using os.ReadFile / io.ReadAll, or accept io.EOF when n>0 (and ensure the file is closed via defer, handling close errors if relevant).
| fh, err := os.Open(v) | |
| if err != nil { | |
| return nil, err | |
| } | |
| buf := make([]byte, 128) | |
| n, err := fh.Read(buf) | |
| fh.Close() | |
| if err != nil { | |
| return nil, fmt.Errorf("file %s: %w", v, err) | |
| } | |
| line, _, _ := bytes.Cut(buf[:n], []byte{'\n'}) | |
| buf, err := os.ReadFile(v) | |
| if err != nil { | |
| return nil, fmt.Errorf("file %s: %w", v, err) | |
| } | |
| line, _, _ := bytes.Cut(buf, []byte{'\n'}) |
There was a problem hiding this comment.
Fixed — replaced manual open/read/close with os.ReadFile, dropping the 128-byte buffer.
stages/websocket.go
Outdated
| cred = make([]byte, 128) | ||
| n, err := fh.Read(cred) | ||
| fh.Close() | ||
| if err != nil { | ||
| return fmt.Errorf("--auth: file %s: %w", v, err) |
There was a problem hiding this comment.
Reading the credential file via fh.Read() will typically return err==io.EOF for small files; treating any non-nil error as fatal can make --auth /path fail even with valid credentials. Consider using os.ReadFile / io.ReadAll, or accept io.EOF when n>0 (and close the file via defer).
There was a problem hiding this comment.
Fixed — same as the http.go fix: replaced manual open/read/close with os.ReadFile.
| // NB: role resolved once per direction on first UPDATE. BGP guarantees OPEN | ||
| // is exchanged before any UPDATE. If --aspa-role auto and peer didn't | ||
| // send BGP Role capability, ASPA is permanently skipped for this direction. | ||
| di := m.Dir & 1 // direction index: 0=R, 1=L |
There was a problem hiding this comment.
Using di := m.Dir & 1 to index per-direction state is brittle and the comment is inconsistent with the project’s dir constants (e.g. core/http.go:274-275 indicates DIR_R=1 and DIR_L=2, so DIR_R maps to di=1 and DIR_L to di=0). Consider mapping explicitly (e.g. switch on m.Dir for DIR_R/DIR_L) to avoid confusing inversions and any future changes to dir bit values.
| di := m.Dir & 1 // direction index: 0=R, 1=L | |
| var di int | |
| switch m.Dir { | |
| case dir.DIR_L: | |
| di = 0 | |
| case dir.DIR_R: | |
| di = 1 | |
| default: | |
| return true | |
| } |
There was a problem hiding this comment.
Not applying. The reviewer has the constants backwards: DIR_L = 0b01 = 1, DIR_R = 0b10 = 2. So DIR_R & 1 = 0 and DIR_L & 1 = 1, which matches the comment // 0=R, 1=L exactly. The code is correct.
Extends the rpki stage with ASPA path validation on top of the existing ROV, and picks up several orthogonal fixes to the HTTP API, websocket stage and a few others.
rpki stage — ASPA
is skipped for RS peers per RFC 7947.
HTTP API
Websocket stage
Other stage fixes
Docs
docs/stages/rpki.md updated for all of the above: new flags, ASPA action table, RS first-hop note, JSON-vs-CSV capability matrix, new metrics.
Compatibility
ROV behaviour is unchanged by default. ASPA is opt-in (--aspa). --http-token was dropped in favour of --http-auth; callers must migrate (this is the only breaking change).