Implement --ice-servers feature flag

[NWoodsman]

Implements #9 Bring your own TURN

Implemented the --ice-servers flag and the associated app logic to handle passing custom ICE server configuration to the bitbang signalling server.

In a slightly roundabout way, we take the user-supplied .json and validate it first by ensuring it parses to a []webrtc.ICEServer. Not long after, we covert it back to it's json representation for sending over the wire.

Changed files

serve.go

• Added cli arg parsing: handles the ice-servers flag
• Uses the existing icehelpers.ParseICEServers() function to generate the aforementioned strong type instance
• Modifies the cfg struct with extra fields.

client.go

• Added an additional function signature to handle the custom ICE servers.
• In register() we call the added helper function AppendIceServerToMessage() to prep our Message.

Tests

• For discussion. Haven't thought hard enough about what tests would be helpful.

[richlegrand]

Not reviewing the draft, just saving you a debugging round: ice_servers needs to go on
the wire as an array, not a JSON-encoded string. The server's wire.Register has
ICEServers []ICEServer, so the string form fails to decode with cannot unmarshal string into Go struct field Register.ice_servers of type []wire.ICEServer -- and because that
error is server-side, all you see is the socket closing.

msg["ice_servers"] = *iceServers and dropping the manual json.Marshal should do it.

[NWoodsman]

Oh my bad, I will fix that. That simplifies the PR.

[NWoodsman]

I cleaned up the wrong assumptions about the ICE server config type. The commit fixes the type and removes the unnecessary added helper methods.

Note that in client.go the function I added called NewClient_MaybeICE is necessary unless we want to refactor all call sites to pass nil in for the 3rd parameter which is the "maybe ICE server" argument.

I refactored slightly (based on the above( to defer nil checks entirely since bitbang-server should handle nil for ICEServers when it marshals the struct to json (according to the annotations).

[NWoodsman]

I'm getting a feel for this API and maybe we want to refactor more so that the --ice-server flag can handle home directory also. Currently I wrote it to convert local paths into absolute paths so that cd into the intended bitbang share directory and calling --ice-servers foo.json will properly resolve the path of foo. But why should we be forced to carry the json around to the current directory?

With cloudflare, i might curl new credentials in home, or somewhere else. I should be able to ~/ice-credential.json I would think.

Let me know if that's better and I will add a conditional to detect the home directory.

Edit: there could probably be a helper function that just totally handles file system path -> []webrtc.ICEServers. So throw the credential anywhere on the drive and it just works.

[NWoodsman]

2 items:

• i tried the build with a credential generated from my cloudflare, and indeed iceServers from the auto-generated credentials failed. After renaming to ice_servers it successfully generated my QR code. See the new issue I opened here: Confirm ICE server config parameter is correctly understood #17

• Edit was having a panic but I fixed it.

[NWoodsman]

b347cc3 runs successfully. I was able to fix iceServers in my Cloudflare generated credentials to ice_servers and was able to use the new flag. I checked my cloudflare statistics and my TURN server showed the activity so I know the backend config was successful.

Let me know if you want any additional code or tests.

[richlegrand]

Thanks for this -- nice work, and it works. :) I traced it against the server source and the
whole loop closes: device registers the override, device_ws.go:118 stores it,
iceForClient prefers it over coturn, and it comes back to the listener on the request
message. You also turned the type-assumption fix around fast after my note.

Three things, all small, none of them the approach -- that part is right. Mostly polish
before merge.

1. The nil check never fires, so every listener sends ice_servers: null

signalingClient := signaling.NewClient_MaybeICE(cfg.server, id, &cfg.iceServers)  // serve.go
if c.OwnICEServers != nil { reg["ice_servers"] = &c.OwnICEServers }              // client.go

&cfg.iceServers is the address of a struct field, so it is never nil -- the guard is
always true. Without --ice-servers that puts {"ice_servers":null,...} on every
register, from every user. Nothign breaks today, because the server checks
len(regMsg.ICEServers) > 0, but it is a wire change for everyone resting on a check at
the far end, and the != nil reads like it prevents exactly that.

This is the dereference from my earlier note, one level further out than I described it.
The same fix retires NewClient_MaybeICE, which is NewClient's body again and will
drift from it:

// client.go
OwnICEServers []webrtc.ICEServer    // plain slice

if len(c.OwnICEServers) > 0 {
	reg["ice_servers"] = c.OwnICEServers
}

// serve.go
signalingClient := signaling.NewClient(cfg.server, id)
signalingClient.OwnICEServers = cfg.iceServers

2. Only one file shape is accepted, and it is not the one people will write

ParseICEServers takes a whole signaling message and reaches for its ice_servers key,
so the file has to be wrapped in an object. I ran the four shapes someone would try:

[ {...} ]                     Error unmarshalling ICE JSON content: cannot unmarshal
                              array into Go value of type map[string]interface {}
{"iceServers": [ {...} ]}     Malformed JSON ICE config. Cannot continue.
{"ice_servers": [ {...} ]}    accepted
{...}                         Malformed JSON ICE config. Cannot continue.

iceServers is the RTCConfiguration spelling, so it is the likely first guess, and the
bare array is the shape the flag name suggests. Neither error says what was expected, and
--ice-servers's help text doesn't either.

Worth taking the bare array as the primary form, accepting either key if the top level is
an object, and naming the shape when it fails:

--ice-servers: expected a JSON array of ICE servers, e.g.
  [{"urls":["turn:host:3478"],"username":"u","credential":"p"}]

3. gofmt

Fails on both files -- mixed tabs and spaces, blank lines carrying whitespace, and
github.com/pion/webrtc/v4 sitting in the golang.org/x import group. gofmt -w on the
two clears it. CI runs go test and govulncheck but not gofmt, so nothing would have
told you.

Smaller

• Errors print to stdout; everything else in serve.go uses fmt.Fprintf(os.Stderr, ...).
  Anything piping the URL line would swallow them.
• filepath.Clean + os.Getwd + filepath.Join is filepath.Abs.
• No test covers the new path. A table over the four shapes above is most of one.

Nothing here is a regresion -- the flag is new, and with the file shaped right it does
what it says. Happy to take another look once you've had a pass, or to pick up the
gofmt one myself if it saves you a round trip.

[NWoodsman]

Ok, a couple of improvements, and hopefully a refactor you are ok with in 4bfed7b:

• removed the redundant NewClient_MaybeICE function and stuck to Claude's guidance. We just check the length of the []webrtc.ICEServer slice instead.
• I couldn't type-fit incoming un-sanitized user ICE config JSON into icehelper.ParseICEServers(). And the signature of the arguments was causing unnecessary gymnastics to pass client-facing json in.
• Therefore I made ParseICEServers private and 1. created an intentionally named AnyToICEServers to handle the lib call sites and stress that the function is intentionally for JSON marshalling, and 2. I added a more robust client-facing function to properly validate the shape of the user-supplied config before passing it along. This added function, UnMarshalUserIceJson, should now handle the Cloudflare credentials by stripping out the "iceServer": object and passing along just the array as expected in the lib / as Claude requested.
• Finally, unrelated, I added a helper function that should resolve whatever path the user wants for their ICE config JSON: absolute, relative, or home-prefixed paths.

[richlegrand]

Thanks! splitting the wire parse from the file parse is great, and
handling all three file types with json.RawMessage is cleaner than what I would
have implemented. I built this and ran it against our test server: a listener started
with --ice-servers gets its own STUN entry back on the connect message, replacing
ours. The feature does what it says.

Four things I hit while testing:

1. bitbang share panics at startup. OwnICEServers is a *[]webrtc.ICEServer and
   only serve sets it, so internal/share/worker.go dereferences nil at register.
2. msg["ice_servers"].([]any) panics when the field is absent, which happens more
   than it looks -- the server drops it when it has no STUN to stamp, and omits it
   from the offer when TURN is unavailable. That second one is the case the connector
   is meant to survive by going direct.
3. go test ./... doesn't build: icehelper_test.go still calls the removed
   ParseICEServers.
4. --ice-servers ~ panics on path[2:].

Rather than send you back a list, I put the fixes in the attached patch
-- git am it and it lands as a commit on top of your four. It also adds tests for the three file
types, and restores the icehelper test whose first case ("missing field returns nil")
is exactly number 2.

And sorry about the CI -- your runs were sitting waiting on me to approve them, which
is annoying. go test ./... locally gives you the same answer anytime, no waiting. (but I
loosened the permissions, hopefully I got it right.)

Last thing, whenever you get to it -- the branch is 15 commits behind main. Patch
first, then merge:

git am pr16-fixes.patch     # clean
git merge main              # one conflict, two lines
git push

The conflict is in connect_pair.go, and it isn't really about ICE: main added a
verbose arg to NewPairPeer on the same line as the ICE parse, so git can't take
both halves. Take one from each side --

iceServers := icehelper.FromMessage(offer)
p, err := client.NewPairPeer(iceServers, verbose)

-- and go build plus go test ./... come back green. I ran that exact sequence to
check.

[NWoodsman]

• The patch was helpful. FromMessage is a good API. I made a slight tweak and removed AnyToICEServers (previously added by me) to intentionally keep only two entry points to the icehelper API: 1. Message parsing and 2. user JSON parsing.
• I'm leery of FromMessage returning nil. It was in the code before I started and I stuck to the convention...but are we sure all callsites now and in the future expect to handle nil? Instinct says 'maybe' since it's a library function and the ParseUserIceFile function is error-handling the higher-likelihood-to-fail flow properly.
• PR branch is now merged with main and all my tests you added in the patch pass. Let me know what else I can do.

EDIT: forgot to run gofmt. Let me know if I need to do so.

[richlegrand]

Removing AnyToICEServers was the right call -- it inlines to four lines and the
package now exports exactly two parse entry points. Better than what I sent you. CI is
green across all four jobs on 852acff, including govulncheck, which the merge fixed.

On FromMessage returning nil: you can stop worrying about that one, and it's worth
saying why rather than just "it's fine." All three call sites hand the result straight
into webrtc.Configuration{ICEServers: ...}, and pion guards it with
if len(sanitizedICEServers) > 0. A nil slice and []ICEServer{} are the same thing
to it. So nil isn't standing in for "something went wrong" -- it is the value, the
empty set of ICE servers, which is exactly what we want when the server sends none. No
caller could branch differently on absent versus empty even if the signature let them.

Your instinct is circling a real case though: present but unparseable. That's our bug
or a broken server, and right now it degrades silently to direct-only. An error return
still wouldn't help, since no caller could do anything but carry on -- but it deserves
a log line. Leave it for a follow-up; it isn't this PR's job.

Two nits and then I think this is done:

• gofmt: yes please, but only internal/icehelper/icehelper.go -- gofmt -l will also flag
  auth/pin_test.go, fileshare/fileshare.go, fileshare/safepath_test.go, and
  identity/identity.go. Those are already unformatted on main, so please leave them
  alone -- formatting them buries this diff in unrelated churn.
• Two comments still name the old function: internal/peer/connection.go:185 and
  :591 say icehelper.ParseICEServers.

Then mark it ready for review and I'll merge it. Thanks for sticking with this one --
it ended up somewhere better than where either of us started.

[NWoodsman]

Fixed the nits. I marked it ready for review; i noticed in you patch you fixed the nil dereference in OwnICEServers by switching to a slice. I forgot to address that. thanks for the patch.

[richlegrand]

Merged -- many thanks!