Resolve API hostnames before dialing - #553
Conversation
|
Heads-up on merging this one, and it is my doing rather than yours. This branch predates #560, which stopped tracking compiled bytecode. It has a Git leaves the file sitting in the working tree, so resolving with git rm <path>.disOr just rebase onto current master, where the file no longer exists and the Nothing else in the PR is affected — for d in appl appl/mpeg appl/veltro tests; do (cd $d && mk install); done
Sorry for the friction — five open PRs are in this position because of the |
pdfinn
left a comment
There was a problem hiding this comment.
Diagnosis confirmed. I dialled both forms from a throwaway module on master
rather than trusting the test's skip message:
tcp!1.1.1.1!80 ok=0
tcp!one.one.one.one!80 ok=-1 invalid IP address (connect one.one.one.one!80 )
So it is exactly as you describe — dial refuses the name, and the machine is
online. TcpDialHostname skipping was hiding a real defect behind an
"offline?" message.
The approach is right: optional Srv load with a literal fallback means native
builds behave as before, and dialaddr degrading to the old string when
iph2a returns nothing is the correct failure mode. Preferring IPv4 because
the dial string is !-separated is a good reason, well stated.
One thing to confirm before this lands
llmclient.b does not consult publicnet. appl/lib/webclient.b and
appl/veltro/tools/webfetch.b do — that is where the SSRF blocklist lives.
Today that gap is narrow, because dial only accepts a literal address, so the
reachable set is whatever an operator typed as an IP. This change widens it to
anything DNS resolves, which is the point of the PR but also a real change in
reach for a module that has no blocklist in front of it.
The question is whether baseurl is ever agent-influenced. It is a parameter
defaulting to http://localhost:11434/v1, and nsconstruct.b:311 is explicit
that /mnt/llm is granted by capability rather than by existence — so on the
face of it this is operator configuration and there is no issue. But I could
not rule out a write path through llmsrv's ctl surface, and if one exists (or
is added later), a prompt-injected agent pointing the client at an internal
address is precisely what publicnet was written to stop.
If it is operator-only, say so in the PR and I am happy. If it is not, this
should route through the same check webfetch uses.
Smaller notes
dialaddr resolves unconditionally, including when host is already a literal
IP — iph2a("127.0.0.1") presumably returns it unchanged, but that is a DNS
round trip on the common Ollama-at-localhost path. A cheap "does it contain
only digits and dots, or a colon" check up front would skip it. Not
correctness, just latency on the hot path.
The IPv4 scan sets isv4 = 1 then clears it on a colon, which reads slightly
backwards — contains(a, ':') would say it more directly if agentlib is
already imported here. Cosmetic.
Also needs the dis/lib/llmclient.dis conflict resolved by deletion rather
than by keeping the branch's copy — see my other comment.
pdfinn
left a comment
There was a problem hiding this comment.
Closing my own question — I could determine this from the tree and should not
have put it to you.
apiurl has exactly two assignments in appl/cmd/llmsrv.b:
245: apiurl = "";
257: 'u' => apiurl = arg->earg();
Init, and the -u command-line flag. There is no runtime write path. The
Qctl write handler accepts reset, close and autocompact <n> and
rejects everything else with "unknown command"; Qmodel sets sess.model
through resolvemodel(), not a URL.
So the endpoint is operator configuration fixed at process start, and this
change does not widen anything an agent can reach. It widens what an
operator's own -u can express, which is the entire point. publicnet does
not belong in this path — it guards agent-supplied URLs in webclient and
webfetch, which is a different trust context.
The engineering justification stands on its own: hostname dial is broken, I
reproduced it, and every hosted LLM API is addressed by name.
Approving. The two smaller notes — skipping resolution when host is already
a literal, and the inverted isv4 scan — are both optional.
Still needs the dis/lib/llmclient.dis conflict resolved by deleting the file
rather than keeping the branch's copy.
4811968 to
6129379
Compare
|
Rebased onto current master; I had this reviewed here before pushing, including re-deriving your own The reviewer also confirmed the change does what it claims, with a probe rather One finding worth having, since it is a gap rather than a defect: no test in the Your two smaller notes I would leave. |
|
The force-push dismissed your approval, sorry. The delta from the commit you approved ( Nothing but the |
6129379 to
c476515
Compare
|
Rebased onto current master (df34b02). Force-pushed 6129379 -> c476515.
Re-verified on macOS after the rebase.
This branch adds no test of its own: #578 fixed the ClusterFuzzLite link break, so |
What this changes
llmclientbuilt every dial string as"tcp!" + host + "!" + portand handedit to
sys->dial.deviponly accepts a literal address, and this build shipsno connection server, so any endpoint configured by name fails before a
connection is attempted. Upstream's own
tcp_testshows it:That is not an offline machine —
TcpDialIppasses in the same run. It isdialrefusing a name.The six dial sites now go through
dialaddr(), which resolves the name withSrv->iph2afirst.$Srvis a hosted builtin and absent on native builds, sothe module is loaded optionally and
dialaddrfalls back to the literal hostwhen it is nil — a native build behaves exactly as before. IPv4 is preferred
because the dial string is
!-separated and an IPv6 literal is ambiguous inthat form. Same approach
tlsperfalready takes.Evidence
sys->dialrejects every name, not just an unreachable one. Dialing fouraddresses from inside the emulator on this machine:
127.0.0.1gets a real TCP answer, so the stack is up.localhostfailsidentically to the others, so this is not DNS and not reachability —
dialwill not take a name.
The resolver path works, and shows why IPv4 is preferred:
iph2areturns the IPv6 address first forlocalhost, and an IPv6 literal isambiguous inside a
!-separated dial string.Test
No new test. Resolution needs a live resolver, so a hermetic one would have to
stub
Srv, anddialaddris internal. What is checked:bytecode —
llmclient_reqshape4,llmclient_think_gating5,llmsrv62Worth a separate look
TcpDialHostnamereports this failure as a skip, under "network unavailableor DNS failed". A build that cannot dial a name at all is indistinguishable
from an offline test machine, so the limitation stays invisible. Splitting
those two cases would make it visible, but that is a change to his test rather
than to the client, so it is not in this PR.