Add DNS and CG endpoints to test server - #1074
Conversation
PaulJouvanceau
commented
Jul 31, 2026
PaulJouvanceau
commented
- Extend util/sgcpserverfortest/main.go with handlers for the new SGCP DNS CNAME records API (list, create, get, update/patch, delete) and a basic CG endpoint.
- Add util/sgcpserverfortest/users.yaml for test credentials.
- Add util/sgcpserverfortest/qa.sh to test the server endpoints, covering auth token, filesystem, CG, and DNS alias lifecycle.
8947ae2 to
ae645d5
Compare
PaulJouvanceau
changed the title
- Extend util/sgcpserverfortest/main.go with handlers for the new SGCP DNS CNAME records API (list, create, get, update/patch, delete) and a basic CG endpoint. - Add util/sgcpserverfortest/users.yaml for test credentials.
ae645d5 to
aaaaf5b
Compare
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| ListDnsAliases = "GET /dns/zones/{zoneID}/cname-records" | ||
| CreateDnsAlias = "POST /dns/zones/{zoneID}/cname-records" | ||
| GetDnsAlias = "GET /dns/zones/{zoneID}/cname-records/{id}" | ||
| UpdateDnsAlias = "PATCH /dns/zones/{zoneID}/cname-records/{id}" | ||
| DeleteDnsAlias = "DELETE /dns/zones/{zoneID}/cname-records/{id}" |
There was a problem hiding this comment.
🟡 Medium - DNS test server no longer serves the configured client paths
The server replaces the existing /dns/zone/{zoneID}/cname-entry handler with /dns/zones/{zoneID}/cname-records, but the SGCP client still constructs the former path from the repository's test configuration. Running the DNS client against this test server therefore receives a 404 for list/create/update/delete operations instead of exercising the new implementation, breaking the test-server integration.
Show fix
Either retain the existing /dns/zone/{zoneID}/cname-entry route (and add the new API routes), or update the client/configuration and all consumers in the same change so the configured DNS paths match the server routes. Add an end-to-end test that starts this server and verifies the URL generated by util/sgcp/dns.go reaches the handler.
More info - Reply on this comment to give feedback or ignore the issue.
| func dnsCreateAlias(w http.ResponseWriter, r *http.Request) { | ||
| zoneID := r.PathValue("zoneID") | ||
| slog.Info(CreateDnsAlias, "zoneID", zoneID) | ||
| if !assertAuth(CreateDnsAlias, w, r, "account1:sgcp:dns:read", "account1:sgcp:dns:write") { |
There was a problem hiding this comment.
🟡 Medium - DNS write operations reject the scopes issued by the client
The new create, update, and delete handlers require both the DNS read and DNS write scope, while util/sgcp/dns.go requests only dns_write for each of those operations. With the repository test configuration mapping that request to only dns:write, valid write tokens receive 403 and none of the mutating DNS paths can be exercised.
Show fix
Require only the write scope for create, update, and delete handlers, matching the client calls, or change the client contract to request both scopes and update the token configuration consistently. Add integration coverage for each operation using the exact scopes emitted by DNSAPI.
More info - Reply on this comment to give feedback or ignore the issue.
| return | ||
| } | ||
| // Use search by id | ||
| a, ok := dnsApi.DB.Search(zoneID, "", id) |
There was a problem hiding this comment.
🟡 Medium - DNS alias IDs bypass the requested zone boundary
The new GET, PATCH, and DELETE handlers pass an empty name when looking up an alias by path ID, and the helper falls back to its global byId index whenever name is empty, ignoring the supplied zone ID. A caller who knows an alias UUID can therefore read it through another zone path or update/delete it while presenting that other zone, violating the zone/resource binding expected by these routes.
Show fix
Make ID lookups require both the UUID and zone ID, for example by validating the returned alias's ZoneID against the path before returning or mutating it, and make the helper's UUID index zone-aware. Add tests that create or seed an alias in zone A and assert that GET, PATCH, and DELETE through zone B return not found without changing the zone-A record.
More info - Reply on this comment to give feedback or ignore the issue.
| func dnsCreateAlias(w http.ResponseWriter, r *http.Request) { | ||
| zoneID := r.PathValue("zoneID") | ||
| slog.Info(CreateDnsAlias, "zoneID", zoneID) | ||
| if !assertAuth(CreateDnsAlias, w, r, "account1:sgcp:dns:read", "account1:sgcp:dns:write") { |
There was a problem hiding this comment.
🟡 Medium - New DNS and CG routes accept forged Authorization headers
The new handlers rely on assertAuth, which only checks whether the raw Authorization header contains each scope string and never verifies a Bearer scheme, token issuance, signature, or client identity. Consequently, any caller who can reach this listener can invoke the new DNS mutations or CG endpoint by sending arbitrary text containing the required scope names, without obtaining credentials; this extends the pre-existing weak check to newly added privileged operations.
Show fix
Validate the authorization scheme and token against the token issued by postAuthToken (or use a real signed/opaque-token verifier), derive scopes from the validated principal rather than substring matching, and return 401/403 for invalid tokens. Add tests proving that arbitrary headers and tokens with similar-but-unissued text cannot access each new route.
More info - Reply on this comment to give feedback or ignore the issue.
| if !assertAuth(ListDnsAliases, w, r, "account1:sgcp:dns:read") { | ||
| return | ||
| } | ||
| _, _, code, data, err := dnsApi.GetAliases(r.Context(), zoneID, name, id) |
There was a problem hiding this comment.
🟡 Medium - Listing DNS aliases without filters always returns 404
The new list handler forwards empty name and id when the caller requests all aliases, but the helper's search function has no empty-filter case and reports no match. As a result, the endpoint returns 404 for a valid list-all request instead of a successful cnameRecords response, so clients cannot enumerate aliases when neither filter is supplied.
Show fix
Implement an explicit list-all path in the helper that returns all aliases in the requested zone, with a 200 response and an empty cnameRecords array when there are no matches. Preserve filtered lookup semantics separately and add tests for list-all on populated and empty zones.
More info - Reply on this comment to give feedback or ignore the issue.
1 participant
