Skip to content

fix: redact all query parameters in CRI error logs #12491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
AkihiroSuda merged 1 commit into from
Nov 19, 2025
Merged

fix: redact all query parameters in CRI error logs #12491

AkihiroSuda merged 1 commit into from
Nov 19, 2025
+229 −0

Conversation

@andrey-noskov
Copy link
Contributor

@andrey-noskov andrey-noskov commented Nov 6, 2025

Trying to fix #5453

@github-project-automation github-project-automation bot moved this to Needs Triage in Pull Request Review Nov 6, 2025
@dosubot dosubot bot added area/cri Container Runtime Interface (CRI) kind/bug labels Nov 6, 2025
cpuguy83
cpuguy83 requested changes Nov 6, 2025
View reviewed changes
internal/cri/instrument/instrumented_service.go Outdated
log.G(ctx).WithError(err).Errorf("PullImage %q failed", r.GetImage().GetImage())
// Sanitize error to remove sensitive information
sanitizedErr := ctrdutil.SanitizeError(err)
log.G(ctx).WithError(sanitizedErr).Errorf("PullImage %q failed", r.GetImage().GetImage())
Copy link
Member

@cpuguy83 cpuguy83 Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It loos like this is fixing the logs but not the returned error.
Also needs to fix spans.
I think it should set err = ctrdutil.SanitizeError(err) instead of creating a new var.

Copy link
Contributor Author

@andrey-noskov andrey-noskov Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

internal/cri/util/sanitize.go Outdated

// sensitiveParams defines query parameters that should be redacted.
var sensitiveParams = map[string]bool{
"sig": true, // Azure SAS signature
Copy link
Member

@cpuguy83 cpuguy83 Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need more things for sure.
Maybe we can just redact all query params?

Copy link
Contributor Author

@andrey-noskov andrey-noskov Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@github-project-automation github-project-automation bot moved this from Needs Triage to Needs Update in Pull Request Review Nov 6, 2025
@andrey-noskov andrey-noskov changed the title fix: sanitize sensitive information in CRI error logs fix: redact all query parameters in CRI error logs Nov 7, 2025
@fuweid fuweid added the area/distribution Image Distribution label Nov 7, 2025
cpuguy83
cpuguy83 approved these changes Nov 18, 2025
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mikebrow
mikebrow approved these changes Nov 18, 2025
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-project-automation github-project-automation bot moved this from Needs Update to Review In Progress in Pull Request Review Nov 18, 2025
AkihiroSuda
AkihiroSuda requested changes Nov 19, 2025
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good, but please squash the commits

@github-project-automation github-project-automation bot moved this from Review In Progress to Needs Update in Pull Request Review Nov 19, 2025
@andrey-noskov
fix: redact all query parameters in CRI error logs
3e2cee2 
Signed-off-by: Andrey Noskov <andreyn@microsoft.com>
@github-project-automation github-project-automation bot moved this from Needs Update to Review In Progress in Pull Request Review Nov 19, 2025
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Nov 19, 2025

/cherry-pick release/2.2
/cherry-pick release/2.1
/cherry-pick release/1.7

@k8s-infra-cherrypick-robot
Copy link

k8s-infra-cherrypick-robot commented Nov 19, 2025

@AkihiroSuda: once the present PR merges, I will cherry-pick it on top of release/1.7, release/2.1, release/2.2 in new PRs and assign them to you.

Details

In response to this:

/cherry-pick release/2.2
/cherry-pick release/2.1
/cherry-pick release/1.7

Instructions 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.

@AkihiroSuda AkihiroSuda added cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch cherry-pick/2.1.x Change to be cherry picked to release/2.1 branch cherry-pick/2.2.x Change to be cherry picked to release/2.2 branch labels Nov 19, 2025
@AkihiroSuda AkihiroSuda added this pull request to the merge queue Nov 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 19, 2025
@AkihiroSuda AkihiroSuda added this pull request to the merge queue Nov 19, 2025
Merged via the queue into containerd:main with commit 8fa9f4a Nov 19, 2025
90 of 92 checks passed
@github-project-automation github-project-automation bot moved this from Review In Progress to Done in Pull Request Review Nov 19, 2025
@k8s-infra-cherrypick-robot
Copy link

k8s-infra-cherrypick-robot commented Nov 19, 2025

@AkihiroSuda: new pull request created: #12546

Details

In response to this:

/cherry-pick release/2.2
/cherry-pick release/2.1
/cherry-pick release/1.7

Instructions 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.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Nov 19, 2025
Merged
@k8s-infra-cherrypick-robot
Copy link

k8s-infra-cherrypick-robot commented Nov 19, 2025

@AkihiroSuda: new pull request created: #12547

Details

In response to this:

/cherry-pick release/2.2
/cherry-pick release/2.1
/cherry-pick release/1.7

Instructions 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.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Nov 19, 2025
Merged
@k8s-infra-cherrypick-robot
Copy link

k8s-infra-cherrypick-robot commented Nov 19, 2025

@AkihiroSuda: #12491 failed to apply on top of branch "release/1.7":

Applying: fix: redact all query parameters in CRI error logs
Using index info to reconstruct a base tree...
A	internal/cri/instrument/instrumented_service.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): internal/cri/instrument/instrumented_service.go deleted in HEAD and modified in fix: redact all query parameters in CRI error logs. Version fix: redact all query parameters in CRI error logs of internal/cri/instrument/instrumented_service.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 fix: redact all query parameters in CRI error logs
Details

In response to this:

/cherry-pick release/2.2
/cherry-pick release/2.1
/cherry-pick release/1.7

Instructions 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.

@AkihiroSuda AkihiroSuda added cherry-picked/2.1.x PR commits are cherry picked into the release/2.1 branch cherry-picked/2.2.x PR commits are cherry-picked into release/2.2 branch and removed cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch cherry-pick/2.1.x Change to be cherry picked to release/2.1 branch labels Nov 21, 2025
@AkihiroSuda AkihiroSuda mentioned this pull request Nov 21, 2025
Merged
@AkihiroSuda AkihiroSuda added cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch and removed cherry-pick/2.2.x Change to be cherry picked to release/2.2 branch labels Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@mikebrow mikebrow mikebrow approved these changes

Assignees

No one assigned

Labels

area/cri Container Runtime Interface (CRI) area/distribution Image Distribution cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch cherry-picked/2.1.x PR commits are cherry picked into the release/2.1 branch cherry-picked/2.2.x PR commits are cherry-picked into release/2.2 branch kind/bug size/L

Projects

Pull Request Review
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error messages returned from registries should hide sensitive information

7 participants

@andrey-noskov @AkihiroSuda @k8s-infra-cherrypick-robot @cpuguy83 @mikebrow @fuweid @k8s-ci-robot