fix: feat: disinto edge command + SSH-forced-command control plane in tools/edge-control/ (#621) #640
No reviewers
Labels
No labels
action
backlog
blocked
bug-report
cannot-reproduce
in-progress
in-triage
needs-triage
prediction/actioned
prediction/dismissed
prediction/unreviewed
priority
rejected
reproduced
tech-debt
underspecified
vision
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: disinto-admin/disinto#640
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/issue-621"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes #621
Changes
a4fe845b9dto637ea66a5aAI Review
Bugs
critical
bin/disinto:1720-1722:registerappends to.envon every call without checking for existing entries. Sinceallocate_portis idempotent server-side (returns the same port), re-runningdisinto edge register myprojectsilently accumulates duplicateEDGE_TUNNEL_*lines. Replace the threeecho >>lines with sed-in-place replacements (or a dedup-and-replace pattern matching the deregister cleanup logic).critical
bin/disinto:1767-1771:deregister.env cleanup is logically broken. The threegrep -vpasses each read from the original$env_fileand append to$tmp_env. After all three passes,$tmp_envcontains: (all lines except HOST) + (all lines except PORT, including HOST) + (all lines except FQDN, including HOST and PORT). None of the three EDGE_TUNNEL vars are reliably removed. Fix: singlegrep -Ev "^EDGE_TUNNEL_(HOST|PORT|FQDN)="pass into$tmp_env.critical
tools/edge-control/lib/caddy.sh:64:add_routecallsPOST /load, which replaces the entire Caddy config. Registering a second project overwrites the first project's route. Should usePOST /config/apps/http/servers/edge/routes/to append, or maintain a full routes list in registry and reconstruct on each change.critical
tools/edge-control/lib/caddy.sh:89:remove_routealways deletesroutes/0(hardcoded index), regardless of which project is being deregistered. After two registrations, deregistering the second project would delete the first project's route instead. Route deletion needs to find and delete by host match, not by array index.critical
tools/edge-control/lib/authorized_keys.sh:16:SCRIPT_DIRis set to the parent oflib/(i.e.,/opt/disinto-edge), but thensource "${SCRIPT_DIR}/ports.sh"looks for/opt/disinto-edge/ports.shwhich does not exist —ports.shlives inlib/. This causes a runtime error wheneverrebuild_authorized_keysis called (i.e., on every register or deregister). Fix:source "${SCRIPT_DIR}/lib/ports.sh".high
tools/edge-control/install.sh:168: Caddy download URL is wrong on two counts: (1) the filename has no.tar.gzextension —curlwill receive a 404 or HTML error page, not a binary; (2) even with the correct URL, this downloads stock Caddy without thecaddy-dns/gandiplugin required by the Caddyfiledns gandiblock. The wildcard cert DNS-01 challenge will fail. Usexcaddyor the Caddy download API (https://caddyserver.com/api/download?os=linux&arch=amd64&p=github.com/caddy-dns/gandi) to get a plugin-enabled binary.high
tools/edge-control/install.sh:336: TheDOMAIN_SUFFIXsed substitution is a no-op —DOMAIN_SUFFIXis never defined ininstall.sh(there is no--domain-suffixargument), so the sed just replacesdisinto.aiwithdisinto.ai. Domain customization is silently broken.high
tools/edge-control/install.sh:254-257:cp -n(no-clobber) silently skips copying scripts that already exist. On reinstall or upgrade, control plane scripts will not be updated. This is the opposite of the desired idempotent-install behavior.Security
bin/disinto:1696,1759,1802: All three SSH calls useStrictHostKeyChecking=no, disabling host key verification. This makes register/deregister/status vulnerable to MITM — an attacker could intercept the registration and inject a different public key, gaining tunnel access. The edge host key should be pinned. At minimum, useStrictHostKeyChecking=accept-newand document that the first connection pins the key, or add a--edge-host-keyoption.Verdict
REQUEST_CHANGES — Multiple bugs that would prevent the feature from working: broken .env write/cleanup, Caddy route management overwrites all routes, authorized_keys.sh sources wrong path, and Caddy download URL is malformed/missing Gandi plugin
Reviewed at
637ea66| AGENTS.mdAI Review: REQUEST_CHANGES — Multiple bugs that would prevent the feature from working: broken .env write/cleanup, Caddy route management overwrites all routes, authorized_keys.sh sources wrong path, and Caddy download URL is malformed/missing Gandi plugin
AI Re-review (round 2)
Previous Findings
grep -vreads from the original$env_file, not$tmp_env, so$tmp_envends up as (all-except-HOST) + (all-except-PORT) + (all-except-FQDN). Aftercat "$tmp_env" > "$env_file", all three EDGE_TUNNEL vars are present twice. Compare the correct fix inderegister(singlegrep -Evpass) — apply the same pattern here:grep -Ev "^EDGE_TUNNEL_(HOST|PORT|FQDN)=" "$env_file" > "$tmp_env"+mv.grep -Evpass + atomicmv./config/apps/http/servers/edge/routes.SCRIPT_DIRnow points tolib/.cpused.registerupgraded toaccept-new.deregisterandstatusstill useStrictHostKeyChecking=no(see new issue #4 below).New Issues
critical
tools/edge-control/register.sh:40,49:rebuild_authorized_keysis called by runningauthorized_keys.shas a subprocess with the function name as an argument — butauthorized_keys.shhas no argument dispatch. It sourcesports.sh, defines functions, and exits.rebuild_authorized_keysis never invoked on eitherregisterorderegister. Tunnelauthorized_keysis never updated, so newly registered projects can never open reverse tunnels. Fix: source the library inregister.sh(source "${SCRIPT_DIR}/lib/authorized_keys.sh") and call the function directly, or add a dispatch block at the bottom ofauthorized_keys.sh.critical
bin/disinto:1718-1720: same triple-pass append bug as the originalderegistercode (described above in finding #1). Fix:grep -Ev "^EDGE_TUNNEL_(HOST|PORT|FQDN)=" "$env_file" > "$tmp_env" 2>/dev/null || truethenmv "$tmp_env" "$env_file".high
tools/edge-control/lib/caddy.sh:75-80:remove_routeindex logic is wrong. The loop iterates over allhost[]entries across all routes via.[].match[].host[]. The counteridxincrements for each host string, not each route array element. If a route has two hosts, the next route maps toidx=2, notidx=1— deleting the wrong route. Fix: use jq to find the route index directly, e.g.jq -r 'to_entries[] | select(.value.match[]?.host[]? == "\(fqdn)") | .key'.medium
tools/edge-control/lib/authorized_keys.sh:50: operator precedence bug —[ -z "$port" ] || [ -z "$pubkey" ] && continueis parsed as[ -z "$port" ] || ([ -z "$pubkey" ] && continue). When port is empty but pubkey is non-empty thecontinuenever fires; a malformed entry is processed instead of skipped. Fix:{ [ -z "$port" ] || [ -z "$pubkey" ]; } && continue.medium
tools/edge-control/install.sh:92-97: Caddyfile block:2019 { ... respond ... }creates an ordinary HTTP server on port 2019, not the Caddy admin API. The admin API is configured via global options ({ admin localhost:2019 }). This Caddyfile will either conflict with the admin API or silently shadow it, breakingadd_route/remove_routecalls. Fix: add{ admin localhost:2019 }as Caddyfile global options and remove the spurious:2019site block.Verdict
REQUEST_CHANGES — register .env dedup still broken (three-pass append), authorized_keys rebuild never executes, remove_route index logic incorrect
Reviewed at
cf3c63b| Previous:637ea66| AGENTS.mdAI Re-review (round 2): REQUEST_CHANGES — register .env dedup still broken (three-pass append), authorized_keys rebuild never executes, remove_route index logic incorrect
- Fix .env write in edge register to use single grep -Ev + mv pattern (not three-pass append) - Fix register.sh to source authorized_keys.sh and call rebuild_authorized_keys directly - Fix caddy.sh remove_route to use jq to find route index by host match - Fix authorized_keys.sh operator precedence: { [ -z ] || [ -z ]; } && continue - Fix install.sh Caddyfile to use { admin localhost:2019 } global options - Fix deregister and status SSH to use StrictHostKeyChecking=accept-newAI Re-review (round 3)
Previous Findings
grep -Ev+ atomicmvrebuild_authorized_keysdirectly in bothdo_registeranddo_deregisterto_entries[]approach finds route by fqdn value, not by iterating hosts{ [ -z "$port" ] || [ -z "$pubkey" ]; } && continue{ admin localhost:2019 }global options blockaccept-newin both subcommandsNew Issues
high
tools/edge-control/lib/caddy.sh:72,107,121,lib/authorized_keys.sh:88:add_route,remove_route,reload_caddy, andrebuild_authorized_keysall print status messages to stdout. Sinceregister.shruns as an SSH forced command, all stdout is captured as the response bybin/disinto. The SSH response then contains mixed text (Added route: ...,Rebuilt authorized_keys...,Caddy reloaded) followed by the JSON object.jq -r '.port // empty' 2>/dev/nullfails on the non-JSON leading lines,portandfqdnremain empty, anddisinto edge registerexits withError: invalid response from edge host. Fix: redirect all informational echoes in these four functions to>&2.medium
tools/edge-control/install.sh:314:"${INSTALL_DIR}/lib/authorized_keys.sh" rebuild_authorized_keysis the same subprocess-dispatch pattern that was fixed inregister.sh— passingrebuild_authorized_keysas an argument does nothing sinceauthorized_keys.shhas no argument dispatch. The initial authorized_keys file fordisinto-tunnelis never created during installation. Fix: source the library and call the function directly (same pattern as the register.sh fix), or accept that a firstdisinto edge registerwill create it.Verdict
REQUEST_CHANGES — stdout pollution in helper functions corrupts JSON response; install.sh subprocess dispatch still broken
Reviewed at
cd115a5| Previous:cf3c63b| AGENTS.mdAI Re-review (round 3): REQUEST_CHANGES — stdout pollution in helper functions corrupts JSON response; install.sh subprocess dispatch still broken
AI Re-review (round 4)
Previous Findings
caddy.sh:72,107,121,authorized_keys.sh:88) → FIXED: all four informational echoes now redirect to>&2; JSON response on stdout is cleaninstall.sh:314) → FIXED: now sourceslib/ports.shandlib/authorized_keys.shdirectly and callsrebuild_authorized_keysinline; initial authorized_keys is correctly created during installationPre-existing Tech Debt (not introduced by fix commits)
tools/edge-control/register.sh:57: pubkey comment field corrupts key —awk '{$1=""; print $0}' | tr -d ' 'strips the separator between base64 key data and the comment, producingAAAA...==edge-tunnel@projectas the key. OpenSSH rejects the malformed entry so reverse tunnels can never connect. Fix: useawk '{print $2}'to extract only the base64 field. Tracked in #649.Verdict
APPROVE — Both previous blocking issues fixed; no new issues from fix commits
Reviewed at
5a6cffe| Previous:cd115a5| AGENTS.mdAI Re-review (round 4): APPROVE — Both previous blocking issues fixed; no new issues from fix commits