api/image: escape images[].name before interpolating into regexp - #6208
api/image: escape images[].name before interpolating into regexp#6208zanarellidev wants to merge 2 commits into
Conversation
IsImageMatched interpolates images[].name unescaped into a regexp, so a literal "." in a registry hostname (gcr.io, quay.io, docker.io, etc.) acts as a wildcard instead of matching only itself, contradicting the documented "identical" match semantics. These tests fail on current HEAD; the following commit fixes it.
images[].name is documented as an identical/literal image name match,
but IsImageMatched spliced it unescaped into a regexp pattern. Since
"." is a regexp wildcard and appears in virtually every real registry
hostname, a rule like images: [{name: gcr.io/org/app, newTag: v2}]
could also incorrectly match an unrelated image such as
gcrXio/org/app if one happened to appear in the same manifest set.
Wrap the interpolated name in regexp.QuoteMeta so it is matched
literally, matching the documented behavior.
|
This PR has multiple commits, and the default merge method is: merge. 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. |
|
Welcome @zanarellidev! |
|
Hi @zanarellidev. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the 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. |
saitejabandaru-in
left a comment
There was a problem hiding this comment.
Excellent catch! Escaping the input with regexp.QuoteMeta is the correct approach to prevent regex injection/wildcard matching for image names. The tests clearly demonstrate the issue and verify the fix.
saitejabandaru-in
left a comment
There was a problem hiding this comment.
LGTM! Excellent catch. Using regexp.QuoteMeta is exactly the right approach here to prevent image registries containing dots from accidentally matching wildcard characters. The added tests look very comprehensive too.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: saitejabandaru-in, zanarellidev 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 |
Summary
images[].nameis documented as an identical/literal image name match:// Name is a tag-less image name.but
IsImageMatchedsplices it unescaped into a regexp pattern:Since
.is a regexp metacharacter and appears in virtually every real registry hostname (gcr.io,docker.io,quay.io,ghcr.io,*.azurecr.io), a rule such as:would also match an unrelated image like
gcrXio/my-project/my-appif one happened to be present in the same manifest set, contradicting the documented "identical" semantics.This line has had two recent fixes for adjacent issues (#6184 panic-on-invalid-regexp, #6167 digest-algorithm matching) that didn't touch this particular gap.
Fix
Wrap the interpolated name in
regexp.QuoteMetaso it's matched literally, per commit 2.Test plan
image_test.go) and an end-to-endkrustytest (transformersimage_test.go), following this repo's own documented reviewer-guide shape (failing-test commit, then fix commit).git checkout <test-commit> -- image.go(pre-fix) that both new tests fail on unmodified HEAD.api/internal/image/...andapi/krusty/....