Skip to content

Commit

Permalink
fix(aws-rds): correct Policy resource for Proxy::grantConnect() (#12416)
Browse files Browse the repository at this point in the history
fixes #12415

To generate the correct policy, the DatabaseProxy ARN is parsed
and the resulting components are used along with a new parameter
to grantConnect.

The unit test was updated and passes. Caveat lector, I was not
able to get a full docker build or a full local build to work on
my box.

I'm not sure if this should be considered a breaking change. While
it technically alters the functionality of a published function,
the current behavior provides no utility.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
jdvornek authored and Elad Ben-Israel committed Feb 22, 2021
1 parent ed17b9f commit 1f43a40
Show file tree
Hide file tree
Showing 3 changed files with 151 additions and 10 deletions.
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-rds/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ instance.grantConnect(role); // Grant the role connection access to the DB.
The following example shows granting connection access for RDS Proxy to an IAM role.

```ts
const cluster = new rds.DatabaseCluster(stack, 'Database'{
const cluster = new rds.DatabaseCluster(stack, 'Database', {
engine: rds.DatabaseClusterEngine.AURORA,
instanceProps: { vpc },
});
Expand All @@ -295,7 +295,7 @@ const proxy = new rds.DatabaseProxy(stack, 'Proxy', {
});

const role = new Role(stack, 'DBProxyRole', { assumedBy: new AccountPrincipal(stack.account) });
proxy.grantConnect(role); // Grant the role connection access to the DB Proxy.
proxy.grantConnect(role, 'admin'); // Grant the role connection access to the DB Proxy for database user 'admin'.
```

**Note**: In addition to the setup above, a database user will need to be created to support IAM auth.
Expand Down
37 changes: 34 additions & 3 deletions packages/@aws-cdk/aws-rds/lib/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,14 @@ export interface IDatabaseProxy extends cdk.IResource {

/**
* Grant the given identity connection access to the proxy.
*
* @param grantee the Principal to grant the permissions to
* @param dbUser the name of the database user to allow connecting as to the proxy
*
* @default - if the Proxy had been provided a single Secret value,
* the user will be taken from that Secret
*/
grantConnect(grantee: iam.IGrantable): iam.Grant;
grantConnect(grantee: iam.IGrantable, dbUser?: string): iam.Grant;
}

/**
Expand All @@ -335,11 +341,22 @@ abstract class DatabaseProxyBase extends cdk.Resource implements IDatabaseProxy
public abstract readonly dbProxyArn: string;
public abstract readonly endpoint: string;

public grantConnect(grantee: iam.IGrantable): iam.Grant {
public grantConnect(grantee: iam.IGrantable, dbUser?: string): iam.Grant {
if (!dbUser) {
throw new Error('For imported Database Proxies, the dbUser is required in grantConnect()');
}
const scopeStack = cdk.Stack.of(this);
const proxyGeneratedId = scopeStack.parseArn(this.dbProxyArn, ':').resourceName;
const userArn = scopeStack.formatArn({
service: 'rds-db',
resource: 'dbuser',
resourceName: `${proxyGeneratedId}/${dbUser}`,
sep: ':',
});
return iam.Grant.addToPrincipal({
grantee,
actions: ['rds-db:connect'],
resourceArns: [this.dbProxyArn],
resourceArns: [userArn],
});
}
}
Expand Down Expand Up @@ -393,6 +410,7 @@ export class DatabaseProxy extends DatabaseProxyBase
*/
public readonly connections: ec2.Connections;

private readonly secrets: secretsmanager.ISecret[];
private readonly resource: CfnDBProxy;

constructor(scope: Construct, id: string, props: DatabaseProxyProps) {
Expand All @@ -419,6 +437,7 @@ export class DatabaseProxy extends DatabaseProxyBase
if (props.secrets.length < 1) {
throw new Error('One or more secrets are required.');
}
this.secrets = props.secrets;

this.resource = new CfnDBProxy(this, 'Resource', {
auth: props.secrets.map(_ => {
Expand Down Expand Up @@ -477,6 +496,18 @@ export class DatabaseProxy extends DatabaseProxyBase
targetType: secretsmanager.AttachmentTargetType.RDS_DB_PROXY,
};
}

public grantConnect(grantee: iam.IGrantable, dbUser?: string): iam.Grant {
if (!dbUser) {
if (this.secrets.length > 1) {
throw new Error('When the Proxy contains multiple Secrets, you must pass a dbUser explicitly to grantConnect()');
}
// 'username' is the field RDS uses here,
// see https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/rds-proxy.html#rds-proxy-secrets-arns
dbUser = this.secrets[0].secretValueFromJson('username').toString();
}
return super.grantConnect(grantee, dbUser);
}
}

/**
Expand Down
120 changes: 115 additions & 5 deletions packages/@aws-cdk/aws-rds/test/proxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import * as rds from '../lib';
let stack: cdk.Stack;
let vpc: ec2.IVpc;

let importedDbProxy: rds.IDatabaseProxy;

nodeunitShim({
'setUp'(cb: () => void) {
stack = new cdk.Stack();
Expand Down Expand Up @@ -244,7 +246,67 @@ nodeunitShim({
test.done();
},

'grantConnect should add IAM Policy with action rds-db:connect'(test: Test) {
'imported Proxies': {
'setUp'(cb: () => void) {
importedDbProxy = rds.DatabaseProxy.fromDatabaseProxyAttributes(stack, 'Proxy', {
dbProxyName: 'my-proxy',
dbProxyArn: 'arn:aws:rds:us-east-1:123456789012:db-proxy:prx-1234abcd',
endpoint: 'my-endpoint',
securityGroups: [],
});

cb();
},

'grant rds-db:connect in grantConnect() with a dbUser explicitly passed'(test: Test) {
// WHEN
const role = new Role(stack, 'DBProxyRole', {
assumedBy: new AccountPrincipal(stack.account),
});
const databaseUser = 'test';
importedDbProxy.grantConnect(role, databaseUser);

// THEN
expect(stack).to(haveResourceLike('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [{
Effect: 'Allow',
Action: 'rds-db:connect',
Resource: {
'Fn::Join': ['', [
'arn:',
{ Ref: 'AWS::Partition' },
':rds-db:',
{ Ref: 'AWS::Region' },
':',
{ Ref: 'AWS::AccountId' },
':dbuser:prx-1234abcd/test',
]],
},
}],
Version: '2012-10-17',
},
}));

test.done();
},

'throws when grantConnect() is used without a dbUser'(test: Test) {
// WHEN
const role = new Role(stack, 'DBProxyRole', {
assumedBy: new AccountPrincipal(stack.account),
});

// THEN
test.throws(() => {
importedDbProxy.grantConnect(role);
}, /For imported Database Proxies, the dbUser is required in grantConnect/);

test.done();
},
},

'new Proxy with a single Secret can use grantConnect() without a dbUser passed'(test: Test) {
// GIVEN
const cluster = new rds.DatabaseCluster(stack, 'Database', {
engine: rds.DatabaseClusterEngine.AURORA,
Expand All @@ -270,10 +332,29 @@ nodeunitShim({
Effect: 'Allow',
Action: 'rds-db:connect',
Resource: {
'Fn::GetAtt': [
'ProxyCB0DFB71',
'DBProxyArn',
],
'Fn::Join': ['', [
'arn:',
{ Ref: 'AWS::Partition' },
':rds-db:',
{ Ref: 'AWS::Region' },
':',
{ Ref: 'AWS::AccountId' },
':dbuser:',
{
'Fn::Select': [
6,
{
'Fn::Split': [
':',
{ 'Fn::GetAtt': ['ProxyCB0DFB71', 'DBProxyArn'] },
],
},
],
},
'/{{resolve:secretsmanager:',
{ Ref: 'DatabaseSecretAttachmentE5D1B020' },
':SecretString:username::}}',
]],
},
}],
Version: '2012-10-17',
Expand All @@ -283,6 +364,35 @@ nodeunitShim({
test.done();
},

'new Proxy with multiple Secrets cannot use grantConnect() without a dbUser passed'(test: Test) {
// GIVEN
const cluster = new rds.DatabaseCluster(stack, 'Database', {
engine: rds.DatabaseClusterEngine.AURORA,
instanceProps: { vpc },
});

const proxy = new rds.DatabaseProxy(stack, 'Proxy', {
proxyTarget: rds.ProxyTarget.fromCluster(cluster),
secrets: [
cluster.secret!,
new secretsmanager.Secret(stack, 'ProxySecret'),
],
vpc,
});

// WHEN
const role = new Role(stack, 'DBProxyRole', {
assumedBy: new AccountPrincipal(stack.account),
});

// THEN
test.throws(() => {
proxy.grantConnect(role);
}, /When the Proxy contains multiple Secrets, you must pass a dbUser explicitly to grantConnect/);

test.done();
},

'DBProxyTargetGroup should have dependency on the proxy targets'(test: Test) {
// GIVEN
const cluster = new rds.DatabaseCluster(stack, 'cluster', {
Expand Down

0 comments on commit 1f43a40

Please sign in to comment.