feat(sip): add band sip realm/credential provisioning and vcp origination routes - #20
feat(sip): add band sip realm/credential provisioning and vcp origination routes#20kshahbw wants to merge 25 commits into
Conversation
…tives Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Adds TestConstructors_RefuseCrossOriginRedirect, which drives a real cross-origin redirect through NewClient/NewXMLClient/NewBasicAuthClient/ NewClientNoAuth's actual httpClient.Do() call rather than only exercising sameOriginRedirect directly. Closes the gap where a future edit dropping CheckRedirect from one constructor would go undetected.
… bodies Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
… precondition - Update hashElementRe to match <Hash1 attr="...">value</Hash1> patterns - Add test coverage for attributed hash elements - Document that RedactSecrets only redacts within maps/slices, not typed structs Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…ase edge cases Addresses code review findings on the SIP service layer, backed by live probes against the Dashboard API: - *APIFault now implements Unwrap() so cmdutil.ExitCodeForError maps documented SIP failures (ErrorCode-bearing) onto the real exit-code taxonomy instead of always falling back to exit 1. - do() now probes 2xx bodies for an error envelope too (a partially successful bulk operation can return 200/201 with Errors/ResponseStatus), with an explicit empty-body guard so a 202 delete with no body is not misread as unparseable. - parseFault returns nil (not a bogus empty-Code APIFault) on unparseable bodies, falling through to the existing scrubbed api.APIError path. - CreateCredential distinguishes "credential never created" from "created under a different-case username, so its digest hashes are unusable." - Rewrote the partial-success test to actually hit the 2xx+Errors branch, and added coverage for hash scrubbing, shortName's FQDN parsing, and the 2xx empty-body/error-envelope split. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…tials Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Adds the band sip command group with realm CRUD: async create with --wait/--if-not-exists, list (always emits an array), get, update (promote to default), and delete (async, treats 404-on-poll as success). Adds Service.SetRealmDefault (read-modify-write PUT so Description survives) and registers the group in cmd/root.go. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…2/33002 The four non-33004 branches wrapped the original *APIFault in a bare fmt.Errorf (no %w), detaching it from errors.As. cmdutil.ExitCodeForError determines exit codes purely by unwrapping, so these documented conflicts (should exit 4) fell through to exit 1 - contradicting the spec's own error-mapping table. Wrap with %w in all four branches and add TestFaultExit_PreservesExitCode to guard the mapping. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Adds the SIP digest credential commands: create (--password-stdin, --password-file, or --generate-password; --if-not-exists for idempotent retries), rotate (password recovery without changing the credential ID), list, get, and delete. Passwords are never accepted via argv or echoed back once known to the caller; a generated password is shown exactly once. --if-not-exists on a duplicate credential with a generated password returns SecretUnavailableError (exit 8) instead of silently succeeding, since the CLI can no longer prove the caller knows the password. Adds Service.FindCredentialByUsername and Service.CredentialHashesMatch to internal/sip to support the --if-not-exists reconciliation path.
Addresses 7 reliability defects found in review of the credential commands (secrecy invariants were already verified sound): - CredentialHashesMatch no longer folds "no hashes in response" into "false (different password)" -- pins the expected root element via XMLName and errors explicitly when both hashes are absent, rather than telling an --if-not-exists caller to rotate a credential that may be correct. - FindCredentialByUsername now matches usernames case-insensitively (mirrors CreateCredential's existing fold), matching the API's case-insensitive duplicate-detection semantics. - A generated password lost to a post-write failure (e.g. decode error after a successful POST/PUT) now exits 8 (SecretUnavailableError) instead of a generic, retryable-looking exit 1, in both create and rotate. - The app-binding gate in --if-not-exists re-reads the credential via GetCredential rather than trusting ListCredentials' shape for HttpVoiceV2AppId, and reports an empty existing binding distinctly from a different one. - faultExit gained a 23026 (duplicate credential) branch with actionable remediation, wrapped with %w so exit-code mapping isn't silently dropped. - --password-file now reads bounded (os.Open + io.LimitReader) instead of slurping the whole file before checking size, and the size cap is enforced after trimming the trailing newline so a legitimate max-length password isn't rejected. Adds test coverage for FindCredentialByUsername and CredentialHashesMatch in internal/sip, plus cmd/sip tests for the --if-not-exists + --generate-password exit-8 path, the app-binding mismatch message, emitCredential's password inclusion/omission, and the --password-stdin pipe path.
Re-review found that the exit-8 test added for Finding 3 exercised reuseCredential's pre-existing "if generated" block, not either of the two branches Finding 3 actually introduced: CreateCredential failing with generated==true (credential_create.go) and RotateCredential failing with generated==true (credential_rotate.go). Both were previously untested. Adds a stub-server test pair for each command: a plain 500 on the write call (not the documented 23026 duplicate, so --if-not-exists cannot mask which branch is under test) asserts *cmdutil. SecretUnavailableError / exit 8 when the password was generated, and the identical failure with a caller-supplied (--password-stdin) password asserts the opposite -- proving the branch keys on `generated`, not the failure alone. Verified by mutation: manually deleting each "if generated" block and re-running the corresponding new test reproduces a failure; both files were restored afterward with a clean `git diff`.
Adds --route-endpoint/--route-endpoint-type/--route-name/--route-plan-json to `band vcp create` and `band vcp update`, plus --replace-routes on update to guard against silently overwriting an existing origination route plan. `vcp create --if-not-exists` is now state-aware: it compares the requested name, description, app ID, and route plan against any existing match and errors on a mismatch (or on an ambiguous duplicate name) instead of returning the first name match unconditionally.
canonicalPlanJSON compared only route name and per-endpoint
{endpoint, type}, so --route-plan-json could change route priority,
route type (WEIGHTED vs ANI), or endpoint weight and the
--replace-routes guard would treat the plans as identical. Now
compares those fields too, normalizing priority/weight to float64 so
JSON-decoded and hand-built numeric values compare equal.
Also: --route-name alone (or combined with --route-plan-json) was a
silent no-op; it now participates in resolveRoutePlan's flag/JSON
mutual exclusivity like the other route flags. Removed a dead
DisallowUnknownFields() decoder in ParseRoutePlanJSON that had no
effect. Extracted the --if-not-exists conflict check and the
--replace-routes guard condition into pure functions (vcpConflict,
requiresRouteReplaceConfirmation) and added unit coverage for the
canonicalization gap, the route-name fix, duplicate-name detection,
and the guard's transitions.
requiresRouteReplaceConfirmation previously only captured the "would this destroy data" half of the guard; the "&& !updateReplaceRoutes" override lived as a bare boolean expression in runUpdate, untested and outside the extracted function. Move the override inside the function so the complete decision — including whether --replace-routes was honored — is one unit-tested call, and add the fourth guard transition (non-empty existing plan, different requested plan, --replace-routes set -> write proceeds) that was previously uncovered.
auth status stays offline and can only ever know whether the SIP
Credentials role is present, not whether the account itself is enabled
for SIP -- that requires a live probe. Report SIP as a separate
{"status","reason"} object (sip) alongside the existing boolean
capabilities map rather than overloading it, and add `band sip status`
as the explicit, non-persisted probe that resolves the offline
`unknown`.
…Role
band sip status is the probe result itself, not just a mutation that
either succeeds or fails -- so 429/5xx must still surface
{"status":"unknown","reason":"probe_failed"} on stdout even though the
command exits non-zero, or the reason value documented in AGENTS.md is
unreachable. 401/403 stay on the plain error path since those are auth
failures, not a fact about SIP availability.
Also add direct coverage for hasRole against the exact "SIP Credentials"
JWT role string and a realistic mixed-role slice, mirroring
TestCapabilities' fixture style.
…ands Adds cmd/sip/golden_test.go with command-level golden tests driving the real cobra commands through the service seam: realm get field presence and real JSON types (default as bool, credentialCount as number, not XML-stringified), realm list rendering zero realms as [] not null, credential list stripping Hash1/Hash1b while carrying id/realmId/ username/hostname/appId, and realm delete's accepted-vs-completed shape without --wait. Coverage that already existed (status shapes, emitCredential password inclusion/omission, faultExit exit-code preservation) is intentionally not duplicated. The delete test caught a real defect: realm_delete.go set `"deleted": !realmDeleteWait`, so a plain `band sip realm delete` (no --wait) reported deleted:true immediately after the API's 202 Accepted, contradicting the command's own documented async-delete contract. Fixed to start deleted:false unconditionally; --wait still promotes it to true once polling confirms the realm is actually gone.
…ry paths AGENTS.md was missing the SIP prerequisite chain, write-once credential behavioral notes, and timeout-recovery rows for realm create/delete. Exit code 8 (ExitSecretUnavailable) was undocumented in both AGENTS.md and README.md, and README's SIP realm table rows omitted --wait, --if-not-exists, --timeout, and --description.
…ry guidance Review caught that AGENTS.md attributed sip credential create's non-ACTIVE-realm failure to API error 23022, but that path is actually a client-side check that exits 1 with a different message; 23022 only fires in a narrow API race. Also extended the exit-8 recovery row to cover the case where the credential ID is not yet known (list before rotating), and added --app-id/--if-not-exists to README's sip credential create row and a realm create --if-not-exists mismatch caveat to AGENTS.md.
Final whole-branch review fix wave. The core finding: exit codes — the
primary contract for the agents this CLI is built for — were derived from
the HTTP status a Bandwidth endpoint happened to use, so every state
conflict this feature produces exited 1 or 8 instead of the documented 4.
Exit codes
- Add cmdutil.ConflictError (exit 4) with Unwrap, mirroring
SecretUnavailableError. Convert every client-side state conflict in
`sip realm create`, `sip credential create`, `vcp create`, `vcp update`,
and the service's duplicate-username check. Messages and remediation
commands are preserved verbatim; only the type changed.
- faultExit's 33002/33006/12666/23022/23026 branches now return
ConflictError instead of leaning on APIFault.Unwrap's status-derived
mapping. Live statuses are 409, 400, and even 201-with-an-Errors-
envelope, so status could never produce a consistent 4. 33004 stays on
FeatureLimitError. The old test built 23022 as a 409 — a status that
endpoint never returns — so it proved nothing; fixtures are now live.
- --generate-password no longer converts a definitive rejection into exit
8. An *APIFault means the server parsed and refused the request, so
nothing was written; a 429 must report exit 7 (retry), not "a secret you
can't recover may exist". Exit 8 is now reserved for genuine ambiguity
(decode failure, mid-flight transport error), which 500-based tests still
cover.
Correctness
- `realm create --if-not-exists --wait` polls a reused CREATE_PENDING realm
to ACTIVE instead of returning exit 0 with an unusable realm. AGENTS.md
tells agents this combination is safe after a --wait timeout.
- `realm create --if-not-exists` matches realm names with EqualFold, per
the spec's case-insensitive rule. The API normalizes the DNS label, so
--name VAPI never matched an existing vapi.
- Add `sip realm update --description`, the remediation the CLI's own
--if-not-exists error names but no command could perform. Extend
SetRealmDefault into UpdateRealm, still read-modify-write so the API's
full-replace PUT cannot drop an unspecified field.
Security
- parseFault scrubs fault Descriptions: the live 23026 response echoes the
submitted digest hashes back inside the error text, which is printed to
stderr and captured in agent transcripts. Add a bare 32-hex redactor to
ScrubHashes for hashes echoed as prose, scoped to raw bodies only —
RedactSecrets keys off field names and must not eat legitimate hex IDs.
- Discard malformed error bodies entirely rather than partially scrubbing
them: a body truncated mid-hash has no closing tag, so the element regex
provably missed it. A parseable body with no ErrorCode (the live 404
shape) is still kept — it is useful and provably hash-free.
Output
- emit normalizes payloads to generic JSON values, which fixes `--format
table` printing Go struct dumps (&{1103 vapi ... ACTIVE 0}) and makes
RedactSecrets actually run instead of being structurally inert on typed
structs. --plain is unchanged modulo key order. A reflection test now
enforces that the domain structs carry no hash fields.
- `credential list` warns on stderr when it returns a full 500-item page.
Auto-pagination stays deferred (the next-link shape is unverified), but a
silent cap that reads as a complete list does not.
Docs
- AGENTS.md: 23026 and 33004 error rows, a note that the whole table exits
4, the realm update section, and three agent-contract bullets.
- README: realm update and credential list rows.
- Fix sipCapability's comment (five reasons, not four) and emit's comment,
which overstated what redaction caught.
cmdutil's exit-code test moves to the external cmdutil_test package:
internal/sip now imports cmdutil for ConflictError, which makes an
in-package test importing internal/sip a test-only import cycle. Every
assertion is retained; the file only gained package qualifiers.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Six fixes that share one theme: an operation that cannot be undone must not
be reachable by accident, and a comparison that guards one must not be
decided on coerced data.
sip credential create/rotate — exit 8 on a stdout-write failure
The API write commits before the password is printed. If stdout is full,
closed, or short-writes at that moment, the credential exists and nobody
will ever know its password: the same unrecoverable state a lost response
leaves behind, so it now reports the same exit code (8) instead of the
generic 1 a write error maps to. Rotate is the worse case of the two — the
peer's hashes are already replaced — so its message names the exact rotate
command to re-run. A caller-supplied password loses nothing, so its write
errors are returned unchanged.
sip credential create/rotate — the generated password is written first
passwordFirstPayload.MarshalJSON emits "password" ahead of every other key
so a truncated write still delivers the one copy of the secret. The JSON
paths therefore bypass emit, which normalizes to a map and inherits Go's
alphabetical key order. emitCredential performs emit's two meaningful
steps itself in the order that matters: normalize, redact, THEN wrap.
Wrapping last is required, not stylistic — output.RedactSecrets only walks
unnamed map[string]interface{}, so a wrapped payload would slip past
redaction entirely. The table path still uses emit.
sip credential create — --app-id is validated as a UUID up front
sipsvc.ValidateAppID runs before the service is built, before the realm
lookup, and before any password is read or generated. An invalid value now
costs zero HTTP requests and, more to the point, never generates a
write-once secret the API was going to reject anyway.
sip realm delete — returns the realm's canonical ID
The ref is resolved with GetRealm before the delete, so --plain "id" is
always the numeric realm ID rather than whichever name or FQDN the caller
typed. Polling uses the canonical ID too: the name a deleted realm
answered to is not guaranteed to keep resolving. Costs one extra GET. The
202 semantics are unchanged — deleted starts false and only --wait
promotes it.
sip credential password reading — trimOneLineEnding
TrimSuffix(TrimSuffix(s, "\n"), "\r") also stripped a BARE trailing "\r".
\r is a legal password byte and the hashes are built from whatever
survives the trim, so dropping it silently produced a credential whose
peer can never authenticate, with no error to explain why. Now exactly one
line ending — "\r\n" or a lone "\n" — is removed.
vcp --route-plan-json — validate nested structure instead of coercing it
The parser checked only that the sole top-level key was "routes".
Everything below it was unchecked, and canonicalPlanJSON then coerced
wrong types and absent numerics into empty slices and zeros — so
{"routes":"not-an-array"}, {"routes":[null]}, a route with "endponts", and
an endpoint with "weigth" all parsed, then canonicalized into a plan the
caller never wrote. That canonical form is what --if-not-exists compares
and what the --replace-routes destructive-write guard is decided from, so a
silent coercion there can let a real plan change read as a no-op.
Input now decodes into typed structs via json.Decoder with
DisallowUnknownFields, and dec.More() rejects trailing content (Decode
stops at the first complete value, so a second concatenated plan was
silently discarded). Structural checks reject an absent, null, or empty
"routes", null route/endpoint entries, an absent/null/empty "endpoints",
and an endpoint missing its "endpoint" or "type". Decode errors are
rewritten to name the offending field rather than leak Go type names.
Numerics are *float64 rather than json.Number for two reasons: float64 is
the exact type encoding/json produces for a JSON number decoded into an
interface{}, so the request map marshals byte-identically to the old raw
map; and a json.Number field reports a wrong type without naming the
field, which defeats the point. Every field is a pointer so a field the
caller omitted stays omitted on the wire — --route-plan-json is a
pass-through, not a defaults provider.
Tests cover each malformed shape with an assertion that the message names
the offending field, trailing content, and byte-identical wire output for
valid input — the last compared against a reference implementation of the
old marshaling, not only a literal, so the invariant cannot be quietly
re-baselined. Every new rejection was mutation-verified against the
pre-fix parser.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…rite path Three behavior changes shipped without documentation. cmd/doccontract_test.go validates that every documented command and flag resolves to a real one, but it cannot catch a description that is missing or wrong, so all three were silent gaps. --app-id must be a UUID. The check runs client-side, ahead of the realm lookup and ahead of reading or generating the password, so an invalid value fails deterministically with zero HTTP requests and no password generated. That last part is the agent-relevant guarantee: a typo cannot burn a write-once secret. sip realm delete returns the canonical realm ID. The argument still accepts an ID, a short name, or an FQDN, but the --plain "id" field is always the resolved numeric realm ID — `band sip realm delete vapi --plain` returns the ID, not "vapi" — which costs one extra GET. The existing accepted/deleted documentation is unchanged: 202 means accepted, and only --wait promotes deleted to true. Exit 8 now also covers stdout-write failures. The exit-code tables described exit 8 only for the existing-credential and lost-response cases. Both now include the third producer: the API write succeeded but the generated password could not be written to stdout (full pipe, closed pipe, or short write), leaving a credential whose password nobody holds. Recovery is unchanged — list to find the ID when it isn't known, then rotate — and the README gains a short recovery paragraph under the table. Both documents now state that the generated password is emitted FIRST in JSON output specifically so a truncated write still delivers it. AGENTS.md also records the consistency exception behind that ordering: every band sip command routes output through the shared emit helper except credential create/rotate's JSON paths, which use their own path to guarantee password-first ordering, with redaction still running there explicitly. It is written up as deliberate so a future reader doesn't "fix" it back to emit. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
⛔ Snyk checks have failed. 1 issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| // server presents the FQDN in its digest challenge. | ||
| func ComputeHashes(username, realmFQDN, password string) (hash1 string, hash1b string) { | ||
| sum := func(s string) string { | ||
| h := md5.Sum([]byte(s)) |
There was a problem hiding this comment.
Use of a Broken or Risky Cryptographic Algorithm
Content of password is encrypted using crypto.md5.Sum. Consider using AES or another strong encryption algorithm instead of MD5.
Line 33 | CWE-327 | Priority score 812 | Learn more about this vulnerability
Snyk flags ComputeHashes as CWE-327. The finding is understood but not remediable: RFC 2617 digest auth defines HA1 as MD5(user:realm:pass), and the Bandwidth API accepts only MD5 for Hash1/Hash1b. A stronger hash would produce credentials no SIP peer could authenticate with. Records that the real risk here is the output being password-equivalent, which is already handled by hash-free domain types, output redaction, and error-body scrubbing. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Thanks — flagging this one as understood but not remediable, and I've pushed a comment at the call site (875dc27) so the reasoning lives with the code rather than only in this thread. Why the algorithm can't change here: this isn't a hash we chose, it's a wire format the protocol dictates. SIP/HTTP digest authentication (RFC 2617) defines HA1 as Substituting a stronger hash wouldn't harden anything — it would create a credential that no SIP peer could ever authenticate with, and the failure would surface at call time as a silent auth rejection rather than as a CLI error. Worth noting the suggested remediation ("consider using AES or another strong encryption algorithm") doesn't apply: AES is encryption, not hashing, and can't produce an HA1. The risk that is real here, and how it's handled. The genuine exposure isn't MD5's collision resistance — it's that the output is password-equivalent: anyone holding
An independent review pass specifically checked for hash/password disclosure across every path and found none, and confirmed the HA1/HA1b construction matches Bandwidth's published recipe. Ask: since remediation isn't possible without breaking interop, this needs an ignore with justification. There's no |
What this adds
band sip— SIP trunk authentication provisioning — plus origination route-plan flags on the existingband vcp. Together these let an operator or an agent stand up a SIP interconnect to a third-party voice-AI platform (Vapi, ElevenLabs, OpenAI SIP) end to end from the terminal. Today none of it is reachable from the CLI:vcpcan only link an app, and there is no realm command at all, so every integration guide says "click through the Bandwidth App."band sip realmcreate(async,--wait) ·list·get·update·deleteband sip credentialcreate·rotate·list·get·deleteband sip statusband auth statussipobject (the booleancapabilitiesmap is unchanged)band vcp create/update--route-endpoint,--route-endpoint-type,--route-name,--route-plan-json,--replace-routesSupporting layers:
internal/sip(MD5 digest hashing, typedencoding/xmlmodels, service), a response-aware HTTP primitive with a same-originCheckRedirectpolicy ininternal/api, hash redaction ininternal/output, and two new exit codes.Design notes worth knowing
Every API behavior here was live-verified against a real SIP-enabled account, and several contradict the published docs:
name-hex.auth.bandwidth.com), not the dot the docs show — and the suffix is environment-dependent. The hostname is always echoed from the response, never constructed.Hash1/Hash1b. The CLI generates the password, computes the digests from the realm FQDN, and prints the password exactly once.rotaterequiresRealmIdin the body despite it being in the path, and must omitUserName— sending it fails with23030even when unchanged. The rotate request type therefore has noUserNamefield at all, by construction.DELETEreturns 200 (synchronous); realmDELETEreturns 202 (accepted).realm deletereportsaccepted: true, deleted: falseand only promotesdeletedafter polling confirms a 404.Security. SIP digest hashes are password-equivalent and the API echoes them on reads and inside error bodies. Containment is layered: domain types carry no hash fields, all output routes through a redaction helper, and error bodies are scrubbed before they are ever stored on an error. An independent review confirmed no hash or password reaches stdout, stderr, logs, or argv on any inspected path, that the HA1/Hash1b construction matches the Bandwidth recipe, and that password generation is unbiased at ~131 bits.
Agent ergonomics. Secrets never enter argv — exactly one of
--password-stdin/--password-file/--generate-passwordis required, with no--passwordflag by design. A generated password is unrecoverable, so any path that could lose one exits8(ExitSecretUnavailable) rather than a generic failure, including a stdout write that fails after the API write committed; the password is emitted first in JSON so a truncated write still delivers it. Client-side state conflicts exit4via a newConflictError.Testing
--plaincontract: field presence, real JSON types (XML-derived scalars asbool/number, not strings), empty lists as[]notnull, and no hash leakage.Known gaps — please read before approving
realm create --wait→credential create→rotate→ teardown) has not happened because the account credentials rotated. This is the one verification I would want before merge.Hash1brecipe is API-accepted but not registration-proven. The API accepts the hashes; only a real SIP registration proves they satisfy a digest challenge. Worth confirming with a softphone before announcing the feature.--route-priority/--route-weight(meaningless with v1's single route — use--route-plan-json), full credential-list pagination (a stderr truncation warning ships instead), and realmPUTreplace-vs-partial semantics.--route-plan-json '{"routes":[]}'is rejected, so the escape hatch cannot clear a route plan. Clearing was never a supported operation; the rejection is explicit rather than a silent coercion.ExitCodeForErrorreturns 1 instead of 2 for auth failures on everybandcommand. Agents lose the "re-authenticate vs. unknown failure" distinction. Worth its own small PR.Review history
Built with a spec-first workflow: two adversarial spec reviews, then 11 implementation tasks each with its own review and fix loop, a whole-branch review, and a final independent review by a different model. That last pass caught four defects the earlier reviews missed, including a write-once-secret bug where a stdout failure after a committed write returned exit 1 instead of 8.
🤖 Generated with Claude Code