Skip to content

Commit

Permalink
feat(ecr): validate repository arn in fromRepositoryArn (#25302)
Browse files Browse the repository at this point in the history 
This PR is adding a regex to check the `arn` structure of an ECR repository. 

Closes #16223

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@vinayak-kukreja
vinayak-kukreja committed May 31, 2023
1 parent fe5ed10 commit 383cccb
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 18 deletions.
10 changes: 10 additions & 0 deletions packages/aws-cdk-lib/aws-ecr/lib/repository.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,8 @@ export class Repository extends RepositoryBase {
throw new Error('"repositoryArn" is a late-bound value, and therefore "repositoryName" is required. Use `fromRepositoryAttributes` instead');
}

validateRepositoryArn();

const repositoryName = repositoryArn.split('/').slice(1).join('/');

class Import extends RepositoryBase {
Expand All @@ -575,6 +577,14 @@ export class Repository extends RepositoryBase {
return new Import(scope, id, {
environmentFromArn: repositoryArn,
});

function validateRepositoryArn() {
const splitArn = repositoryArn.split(':');

if (!splitArn[splitArn.length - 1].startsWith('repository/')) {
throw new Error(`Repository arn should be in the format 'arn:<PARTITION>:ecr:<REGION>:<ACCOUNT>:repository/<NAME>', got ${repositoryArn}.`);
}
}
}

public static fromRepositoryName(scope: Construct, id: string, repositoryName: string): IRepository {
Expand Down
13 changes: 13 additions & 0 deletions packages/aws-cdk-lib/aws-ecr/test/repository.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,19 @@ describe('repository', () => {
expect(stack.resolve(repo2.repositoryName)).toBe('foo/bar/foo/fooo');
});

test('import with arn without /repository', () => {
// GIVEN
const stack = new cdk.Stack();

// WHEN
const invalidArn = 'arn:aws:ecr:us-east-1:123456789012:foo-ecr-repo-name';

// THEN
expect(() => {
ecr.Repository.fromRepositoryArn(stack, 'repo', invalidArn);
}).toThrowError(`Repository arn should be in the format 'arn:<PARTITION>:ecr:<REGION>:<ACCOUNT>:repository/<NAME>', got ${invalidArn}.`);
});

test('fails if importing with token arn and no name', () => {
// GIVEN
const stack = new cdk.Stack();
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-lambda/test/lambda-insights.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ describe('lambda-insights', () => {
const stack = new cdk.Stack();
new lambda.DockerImageFunction(stack, 'MyFunction', {
code: lambda.DockerImageCode.fromEcr(ecr.Repository.fromRepositoryArn(stack, 'MyRepo',
'arn:aws:ecr:us-east-1:0123456789:repository/MyRepo')),
'arn:aws:ecr:us-east-1:123456789012:repository/MyRepo')),
insightsVersion: lambda.LambdaInsightsVersion.VERSION_1_0_98_0,
});

Expand Down
34 changes: 17 additions & 17 deletions packages/aws-cdk-lib/pipelines/test/docker-credentials.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ let stack: cdk.Stack;
beforeEach(() => {
app = new TestApp();
stack = new cdk.Stack(app, 'Stack', {
env: { account: '0123456789012', region: 'eu-west-1' },
env: { account: '123456789012', region: 'eu-west-1' },
});
});

Expand Down Expand Up @@ -44,7 +44,7 @@ describe('ExternalDockerCredential', () => {
});

test('maximum example includes all expected properties', () => {
const roleArn = 'arn:aws:iam::0123456789012:role/MyRole';
const roleArn = 'arn:aws:iam::123456789012:role/MyRole';
const creds = cdkp.DockerCredential.customRegistry('example.com', secret, {
secretUsernameField: 'login',
secretPasswordField: 'pass',
Expand Down Expand Up @@ -86,7 +86,7 @@ describe('ExternalDockerCredential', () => {
});

test('grants read access to the secret to the assumeRole if provided', () => {
const assumeRole = iam.Role.fromRoleArn(stack, 'Role', 'arn:aws:iam::0123456789012:role/MyRole');
const assumeRole = iam.Role.fromRoleArn(stack, 'Role', 'arn:aws:iam::123456789012:role/MyRole');
const creds = cdkp.DockerCredential.customRegistry('example.com', secret, { assumeRole });

const user = new iam.User(stack, 'User');
Expand All @@ -108,7 +108,7 @@ describe('ExternalDockerCredential', () => {
Statement: [{
Action: 'sts:AssumeRole',
Effect: 'Allow',
Resource: 'arn:aws:iam::0123456789012:role/MyRole',
Resource: 'arn:aws:iam::123456789012:role/MyRole',
}],
Version: '2012-10-17',
},
Expand All @@ -117,7 +117,7 @@ describe('ExternalDockerCredential', () => {
});

test('does not grant any access if the usage does not match', () => {
const assumeRole = iam.Role.fromRoleArn(stack, 'Role', 'arn:aws:iam::0123456789012:role/MyRole');
const assumeRole = iam.Role.fromRoleArn(stack, 'Role', 'arn:aws:iam::123456789012:role/MyRole');
const creds = cdkp.DockerCredential.customRegistry('example.com', secret, {
assumeRole,
usages: [cdkp.DockerCredentialUsage.ASSET_PUBLISHING],
Expand All @@ -138,7 +138,7 @@ describe('EcrDockerCredential', () => {
let repo: ecr.IRepository;

beforeEach(() => {
repo = ecr.Repository.fromRepositoryArn(stack, 'Repo', 'arn:aws:ecr:eu-west-1:0123456789012:repository/Repo');
repo = ecr.Repository.fromRepositoryArn(stack, 'Repo', 'arn:aws:ecr:eu-west-1:123456789012:repository/Repo');
});

test('minimal example only renders ecrRepository', () => {
Expand All @@ -150,7 +150,7 @@ describe('EcrDockerCredential', () => {
'Fn::Select': [
0, {
'Fn::Split': ['/', {
'Fn::Join': ['', ['0123456789012.dkr.ecr.eu-west-1.', { Ref: 'AWS::URLSuffix' }, '/Repo']],
'Fn::Join': ['', ['123456789012.dkr.ecr.eu-west-1.', { Ref: 'AWS::URLSuffix' }, '/Repo']],
}],
},
],
Expand All @@ -161,7 +161,7 @@ describe('EcrDockerCredential', () => {
});

test('maximum example renders all fields', () => {
const roleArn = 'arn:aws:iam::0123456789012:role/MyRole';
const roleArn = 'arn:aws:iam::123456789012:role/MyRole';
const creds = cdkp.DockerCredential.ecr([repo], {
assumeRole: iam.Role.fromRoleArn(stack, 'Role', roleArn),
usages: [cdkp.DockerCredentialUsage.SYNTH],
Expand All @@ -173,7 +173,7 @@ describe('EcrDockerCredential', () => {
'Fn::Select': [
0, {
'Fn::Split': ['/', {
'Fn::Join': ['', ['0123456789012.dkr.ecr.eu-west-1.', { Ref: 'AWS::URLSuffix' }, '/Repo']],
'Fn::Join': ['', ['123456789012.dkr.ecr.eu-west-1.', { Ref: 'AWS::URLSuffix' }, '/Repo']],
}],
},
],
Expand Down Expand Up @@ -201,7 +201,7 @@ describe('EcrDockerCredential', () => {
'ecr:BatchGetImage',
],
Effect: 'Allow',
Resource: 'arn:aws:ecr:eu-west-1:0123456789012:repository/Repo',
Resource: 'arn:aws:ecr:eu-west-1:123456789012:repository/Repo',
},
{
Action: 'ecr:GetAuthorizationToken',
Expand All @@ -215,7 +215,7 @@ describe('EcrDockerCredential', () => {
});

test('grants pull access to the assumed role', () => {
const assumeRole = iam.Role.fromRoleArn(stack, 'Role', 'arn:aws:iam::0123456789012:role/MyRole');
const assumeRole = iam.Role.fromRoleArn(stack, 'Role', 'arn:aws:iam::123456789012:role/MyRole');
const creds = cdkp.DockerCredential.ecr([repo], { assumeRole });

const user = new iam.User(stack, 'User');
Expand All @@ -230,7 +230,7 @@ describe('EcrDockerCredential', () => {
'ecr:BatchGetImage',
],
Effect: 'Allow',
Resource: 'arn:aws:ecr:eu-west-1:0123456789012:repository/Repo',
Resource: 'arn:aws:ecr:eu-west-1:123456789012:repository/Repo',
},
{
Action: 'ecr:GetAuthorizationToken',
Expand All @@ -246,7 +246,7 @@ describe('EcrDockerCredential', () => {
Statement: [{
Action: 'sts:AssumeRole',
Effect: 'Allow',
Resource: 'arn:aws:iam::0123456789012:role/MyRole',
Resource: 'arn:aws:iam::123456789012:role/MyRole',
}],
Version: '2012-10-17',
},
Expand All @@ -255,7 +255,7 @@ describe('EcrDockerCredential', () => {
});

test('grants pull access to multiple repos if provided', () => {
const repo2 = ecr.Repository.fromRepositoryArn(stack, 'Repo2', 'arn:aws:ecr:eu-west-1:0123456789012:repository/Repo2');
const repo2 = ecr.Repository.fromRepositoryArn(stack, 'Repo2', 'arn:aws:ecr:eu-west-1:123456789012:repository/Repo2');
const creds = cdkp.DockerCredential.ecr([repo, repo2]);

const user = new iam.User(stack, 'User');
Expand All @@ -270,7 +270,7 @@ describe('EcrDockerCredential', () => {
'ecr:BatchGetImage',
],
Effect: 'Allow',
Resource: 'arn:aws:ecr:eu-west-1:0123456789012:repository/Repo',
Resource: 'arn:aws:ecr:eu-west-1:123456789012:repository/Repo',
},
{
Action: 'ecr:GetAuthorizationToken',
Expand All @@ -284,7 +284,7 @@ describe('EcrDockerCredential', () => {
'ecr:BatchGetImage',
],
Effect: 'Allow',
Resource: 'arn:aws:ecr:eu-west-1:0123456789012:repository/Repo2',
Resource: 'arn:aws:ecr:eu-west-1:123456789012:repository/Repo2',
}]),
Version: '2012-10-17',
},
Expand Down Expand Up @@ -324,7 +324,7 @@ describe('EcrDockerCredential', () => {
});

describe('dockerCredentialsInstallCommands', () => {
const secretArn = 'arn:aws:secretsmanager:eu-west-1:0123456789012:secret:mySecret-012345';
const secretArn = 'arn:aws:secretsmanager:eu-west-1:123456789012:secret:mySecret-012345';
let secret: secretsmanager.ISecret;

beforeEach(() => {
Expand Down

0 comments on commit 383cccb

Please sign in to comment.