Add proxy auth support and status/metrics commands#81
Conversation
- Add authenticated proxy support via env vars or config - Support Basic, Digest, NTLM, and certificate-based (mTLS) auth - Add 'cvd status' command for database health checks - Add 'cvd metrics' command for Prometheus monitoring - Bump version to 1.3.0 Closes Cisco-Talos#7, Cisco-Talos#9, Cisco-Talos#30
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a84f72c1e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Updated based on Codex review feedback:
|
val-ms
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The proxy auth and monitoring additions are useful directions, but I think this needs changes before we can merge.
Blocking issues:
-
cvd config setis currently broken for normal use. The new optional--proxy-certand--proxy-cert-keyoptions useclick.Path(exists=True)withdefault="", so Click validates the empty default and exits before the command runs. I reproduced this locally withcvd config set --dbdir /tmp/db, which fails withInvalid value for '--proxy-cert': Path '' does not exist.These should default toNoneor otherwise avoid validating an absent optional path, and the CLI behavior should be covered by tests. -
The PR says it closes #30, but the update path still requires DNS before any HTTP/proxy download can happen.
db_update()still calls_query_dns_txt_entry()and returns1whendns_version_tokensis empty, so environments without direct DNS still cannot update through an HTTP proxy. Please either remove theCloses #30claim or implement and test a real no-DNS fallback. -
Proxy credentials can leak in logs/config output when credentials are embedded in
proxy_url._get_proxy_configuration()logs the full proxy URL in the unauthenticated branch, sohttp://user:pass@proxy.example.com:8080exposes the secret.config_show()only masksproxy_pass, not credentials already present inproxy_url. Please sanitize userinfo in all displayed/logged proxy URLs.
Other requested changes:
-
The PR body still claims Basic, Digest, NTLM, and certificate-based auth. The current implementation embeds credentials in the proxy URL, which Requests turns into Basic
Proxy-Authorization; it does not implement Digest or NTLM. Please narrow the claim or add real support/tests for those auth types. -
cvdupdate_last_check_timestampis documented as a Unix timestamp but emits milliseconds (time.time() * 1000). Please either emit seconds or rename/re-document it as milliseconds. -
Prometheus label values are interpolated without escaping. Custom database names containing quotes, backslashes, or newlines will produce invalid exposition output. Please escape label values per Prometheus text format rules.
-
CHANGES.mdstill contains placeholder PR links (pull/XX) and says this closes #30. Please update those before merge.
Compatibility note:
This PR also conflicts with PR #88. I checked the current heads with git merge-tree; #81 merges cleanly with #87, but conflicts with #88 in cvdupdate/__main__.py and cvdupdate/cvdupdate.py. The conflict is semantic as well as textual: both PRs introduce or redefine status, both change config set, and #88 restructures config/state handling that #81 builds on. If #88 moves forward, this PR will need to be rebased and reconciled around the CLI design, especially the meaning of status.
Local verification: the PR test suite passes for me (37 passed in 3.05s), and the CLI/config regressions above were reproduced in an isolated temporary venv.
5ce44a4 to
00e2b51
Compare
Switch from requests.auth objects (which set the wrong Authorization header) to embedding credentials in the proxy URL, which is the correct way to trigger Proxy-Authorization in the requests library. Remove the now-unnecessary --proxy-auth-type option and NTLM dependency. Also fix an UnboundLocalError when using cvd status --json --check by moving the summary assignment above the output format branch. Update tests and documentation to match.
Remove the proxy client-certificate options. The requests cert parameter is presented to the destination server during the TLS handshake, not to the proxy during CONNECT, so --proxy-cert and --proxy-cert-key never provided proxy mTLS. Removing them also fixes the config set failure where an empty default path was validated by click.Path(exists=True). Mask proxy credentials wherever they are logged or displayed. The proxy log line and config show now redact userinfo, while the proxy dict returned for use still carries the real credentials. Warn when a proxy URL has no scheme. Report the status last-check time in UTC. Distinguish an unverified version from an outdated one when DNS is unavailable, and stop treating file age alone as critical so a current mirror holding an infrequently changing database is not flagged. Cache status between metrics scrapes so the server does not run a DNS query on every request. Emit cvdupdate_last_check_timestamp in seconds and escape Prometheus label values per the text exposition format. Add Click-level tests for config set, status, and metrics, plus tests for credential masking, label escaping, and the unknown-version case. Narrow CHANGES.md and the docs to Basic proxy authentication, drop the Closes Cisco-Talos#30 claim, and point the links at pull/81.
|
Thanks for the detailed review @val-ms. I have addressed each point. Blocking issues:
Other requested changes:
While addressing the above I fixed a few related issues:
On the #88 overlap: agreed. If #88 lands first I will rebase and reconcile the status command and config set design. Happy to coordinate on ordering. Test suite passes locally, 51 passed. |
Closes #7 and #9. All existing tests pass, plus new unit tests covering the proxy, status, and metrics paths, including Click-level CLI tests.