Repository navigation
fix(logger): do not log the virtual media image URL (it carries the image service token) - #38
itayherzberg wants to merge 2 commits into
Conversation
The image URL handed to InsertMedia carries the image service token in its query string, and it was written to the log at INFO on every insert, plus the DEBUG lines of the handler, the task manager and the worker. The parse error of an unparsable URL repeated the input as well. RedactURL keeps scheme, host and path and drops userinfo, query and fragment; an input that does not parse is replaced entirely. Every log line that carried the URL goes through it, and the parse error keeps only the reason. Signed-off-by: itayherzberg <itayherzberg@gmail.com>
kubevirt-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @itayherzberg. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| if err != nil || u.Scheme == "" && u.Host == "" { | ||
| return "<unparsable url>" | ||
| } | ||
| return (&url.URL{Scheme: u.Scheme, Host: u.Host, Path: u.Path}).String() |
There was a problem hiding this comment.
I think I would prefer if the redaction was obvious from the log. If there is a query string, the log should show it. Even "?" would be enough to show the URL is not complete and the developer debugging an issue should not try it as it.
There was a problem hiding this comment.
The same applies to username, password and the other removed pieces.
There was a problem hiding this comment.
Agreed, a silently shortened URL looks complete and invites someone to try it as is. Changed in 9f98767: RedactURL now leaves a marker in place of every removed piece, and only when that piece was present:
| Input | Logged |
|---|---|
https://host/full.iso?api_key=…&arch=x86_64 |
https://host/full.iso?<redacted> |
https://user:pass@host/boot.iso |
https://<redacted>@host/boot.iso |
https://user@host/boot.iso |
https://<redacted>@host/boot.iso |
http://host/boot.iso#frag |
http://host/boot.iso#<redacted> |
http://host:8080/alpine.iso |
http://host:8080/alpine.iso (unchanged) |
The username is redacted together with the password as a single <redacted>@, and the whole query string becomes one ?<redacted>, without keeping the parameter names. An input that does not parse stays <unparsable url>, which already cannot be mistaken for a usable URL.
The unit tests and the PR description are updated to match. The cluster run in the description was made with the first revision; I did not rerun it for this change.
A URL that was silently shortened looks complete, and someone debugging from the log may try it as is. RedactURL now leaves a marker in place of each removed piece, only when that piece was present: https://<redacted>@host/path?<redacted>#<redacted> The username is redacted together with the password, and the query string as a whole, without keeping the parameter names. A URL without those pieces is logged unchanged; an input that does not parse is still replaced entirely. Signed-off-by: itayherzberg <itayherzberg@gmail.com>
Problem
The image URL Ironic passes to
InsertMediais written to the bridge log on every insert, atINFO:For an assisted-installer discovery ISO the query string is a bearer token for the image service, and the same URL appears on three
DEBUGlines (handler, task manager, worker). A URL that does not parse is logged in full as well, as part of the*url.Error. The project's rule is that no credential reaches the console or the log files; a token in a URL is one.Cause
Plain
%sofimageURLinlogger.Info/Debug/Errorcalls inpkg/kubevirt/client.go(InsertVirtualMedia,insertVirtualMediaAsync,copyISOToPVC,downloadISO) andpkg/server/task_manager.go(CreateTask,processVirtualMediaInsert), andfmt.Errorf("failed to parse URL %s: %w", imageURL, parseErr)whereparseErrrepeats the input once more.Fix
logger.RedactURL(pkg/logger/redact.go): keeps scheme, host (with port) and path; replaces userinfo, query string and fragment each with a<redacted>marker, only when the piece was present, so the log shows the URL is incomplete (https://<redacted>@host/path?<redacted>#<redacted>). The username is redacted together with the password, and the query string as a whole, without keeping the parameter names. A URL without those pieces is logged unchanged. An input that does not parse is replaced by<unparsable url>entirely, because the reason it does not parse may be the secret.missing protocol scheme) and the redacted URL; the error text also ends up in the task'sMessages.Not changed, on purpose: the full URL is still stored in the
virtual-media.redfish/<device>annotation (the same-image check on a repeatedInsertMedianeeds it), in the helper pod'scurlcommand (oc describe pod) and in theTaskreturned to the requester. Those are RBAC-protected cluster objects and the authenticated caller's own response, not log files.downloadISOis only called from tests; its line is redacted like the others but nothing in the bridge reaches it.Behaviour changes: log lines show
http://host/path?<redacted>instead of the full URL; the parse error message of an invalid image URL no longer contains the URL.Verification on the cluster
OpenShift 4.22, the bridge built from fork
mainwith #36 and this branch.InsertMediawithImage: http://slow-iso.redfish-prtest.svc:8080/alpine.iso?api_key=SECRET-TOKEN(the nginx ignores the query string), thenOn; the import completed and the VM booted the ISO. In the bridge log of the whole run (three VMs, inserts, power actions, one failed and three successful imports):grep -c SECRET-TOKEN→ 0, and the insert lines readThis run was made with the first revision of the branch, which dropped the query string without a marker. With the current revision the same lines end in
alpine.iso?<redacted>; that form is covered by the unit tests and was not rerun on the cluster.Tests
pkg/logger/redact_test.go: query string, userinfo (with and without a password) and fragment each replaced by<redacted>, all three together, port kept, plain URL unchanged, unparsable/empty/bare input replaced.pkg/kubevirt/log_redaction_test.gocaptures the log atDEBUGand runs a realInsertVirtualMedia(mock clients) with?api_key=SECRET-TOKEN: the token is absent and the redacted URL (...full.iso?<redacted>) present; the same with an unparsable URL.pkg/server/log_redaction_test.go:CreateTaskdoes not log the token. On currentmainall three fail (the image token was logged: ... imageURL=http://example.com/full.iso?api_key=SECRET-TOKEN). The capture test is the one that catches a future log line reintroducing the URL.go build ./...,go vet ./...,go test ./...,gofmt,go mod tidy(no change) pass.Related