Skip to content

Use net.IP.IsLoopback() to match loopback addresses#5160

Merged
estesp merged 1 commit intocontainerd:masterfrom
MadhavJivrajani:master
Mar 17, 2021
Merged

Use net.IP.IsLoopback() to match loopback addresses#5160
estesp merged 1 commit intocontainerd:masterfrom
MadhavJivrajani:master

Conversation

@MadhavJivrajani
Copy link
Copy Markdown
Contributor

@MadhavJivrajani MadhavJivrajani commented Mar 11, 2021

  • changed the MatchLocalhost function in remotes/docker/registry.go
  • Added additional tests for modified code in remotes/docker/registry_test.go

Fixes: #5129

Signed-off-by: Madhav Jivrajani [email protected]

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Mar 11, 2021

Hi @MadhavJivrajani. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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/test-infra repository.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 11, 2021

Build succeeded.

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Mar 11, 2021

@MadhavJivrajani: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test?

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/test-infra repository.

@MadhavJivrajani
Copy link
Copy Markdown
Contributor Author

MadhavJivrajani commented Mar 11, 2021

/retest

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Mar 11, 2021

@MadhavJivrajani: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

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/test-infra repository.

AkihiroSuda
AkihiroSuda requested changes Mar 11, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Thanks, but please use net.SplitHostPort

Comment thread remotes/docker/registry.go Outdated
AkihiroSuda
AkihiroSuda reviewed Mar 11, 2021
View reviewed changes
Comment thread remotes/docker/registry_test.go Outdated
AkihiroSuda
AkihiroSuda reviewed Mar 11, 2021
View reviewed changes
Comment thread remotes/docker/registry_test.go Outdated
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Mar 11, 2021

Choose a reason for hiding this comment

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

@dmcgowan Was there a specific reason to return false for 127.1.1.1 ? Is there any security concern?

@MadhavJivrajani
Copy link
Copy Markdown
Contributor Author

MadhavJivrajani commented Mar 12, 2021 •
edited
Loading

Hey @AkihiroSuda, any reason why just [::1] is considered a valid loopback? If I understand correctly, the square brackets are used only when qualified with a port number, because of which the net package does not recognize [::1] as a valid IP, but ::1 and [::1]:port are valid ones.

containerd/remotes/docker/registry_test.go

Line 62 in d8208e2

{"[::1]", true},

EDIT: Sorry about that, I misunderstood.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 12, 2021

Build succeeded.

AkihiroSuda
AkihiroSuda requested changes Mar 12, 2021
View reviewed changes
Comment thread remotes/docker/registry.go Outdated
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 13, 2021

Build succeeded.

AkihiroSuda
AkihiroSuda reviewed Mar 13, 2021
View reviewed changes
Comment thread remotes/docker/registry.go Outdated
AkihiroSuda
AkihiroSuda reviewed Mar 13, 2021
View reviewed changes
Comment thread remotes/docker/registry.go Outdated
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 14, 2021

Build succeeded.

@MadhavJivrajani
Use net.IP.IsLoopback() to match loopback addresses
8f863af 
- changed the `MatchLocalhost` function in remotes/docker/registry.go
	- Make use of SplitHostPort to split host and port number
- Added additional tests for modified code in remotes/docker/registry_test.go
- Note: this does not handle mathcing of IP addresses in octal, decimal or hex format or a mix of these.

Fixes: containerd#5129

Signed-off-by: Madhav Jivrajani <[email protected]>
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 15, 2021

Build succeeded.

estesp
estesp approved these changes Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit b395f2f into containerd:master Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees

No one assigned

Labels

needs-ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MatchLocalhost should match 127.12.34.56

4 participants

@MadhavJivrajani @k8s-ci-robot @estesp @AkihiroSuda