Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rds): Correct ARN in IAM policy for IAM database access #25141

Merged
merged 3 commits into from
Apr 20, 2023
Merged

fix(rds): Correct ARN in IAM policy for IAM database access #25141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73d879a
fix(rds): Correct ARN in IAM policy for IAM database access
akash1810 Apr 15, 2023
ce630ec
refactor(rds): Make `instanceResourceId` prop optional
akash1810 Apr 20, 2023
fc0aaa2
Merge branch 'main' into fix-grantConnect
mergify[bot] Apr 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 67 additions & 5 deletions packages/aws-cdk-lib/aws-rds/lib/instance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ export interface IDatabaseInstance extends IResource, ec2.IConnectable, secretsm
*/
readonly dbInstanceEndpointPort: string;

/**
* The AWS Region-unique, immutable identifier for the DB instance.
* This identifier is found in AWS CloudTrail log entries whenever the AWS KMS key for the DB instance is accessed.
*
* @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-rds-dbinstance.html#aws-resource-rds-dbinstance-return-values
*/
readonly instanceResourceId?: string;

/**
* The instance endpoint.
*/
Expand All @@ -66,10 +74,11 @@ export interface IDatabaseInstance extends IResource, ec2.IConnectable, secretsm

/**
* Grant the given identity connection access to the database.
* **Note**: this method does not currently work, see https://github.com/aws/aws-cdk/issues/11851 for details.
* @see https://github.com/aws/aws-cdk/issues/11851
*
* @param grantee the Principal to grant the permissions to
* @param dbUser the name of the database user to allow connecting as to the db instance
*/
grantConnect(grantee: iam.IGrantable): iam.Grant;
grantConnect(grantee: iam.IGrantable, dbUser?: string): iam.Grant;

/**
* Defines a CloudWatch event rule which triggers for instance events. Use
Expand Down Expand Up @@ -97,6 +106,14 @@ export interface DatabaseInstanceAttributes {
*/
readonly port: number;

/**
* The AWS Region-unique, immutable identifier for the DB instance.
* This identifier is found in AWS CloudTrail log entries whenever the AWS KMS key for the DB instance is accessed.
*
* @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-rds-dbinstance.html#aws-resource-rds-dbinstance-return-values
*/
readonly instanceResourceId?: string;

/**
* The security groups of the instance.
*/
Expand Down Expand Up @@ -130,6 +147,7 @@ export abstract class DatabaseInstanceBase extends Resource implements IDatabase
public readonly instanceEndpoint = new Endpoint(attrs.instanceEndpointAddress, attrs.port);
public readonly engine = attrs.engine;
protected enableIamAuthentication = true;
public readonly instanceResourceId = attrs.instanceResourceId;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new, required property. Is it a breaking change for fromDatabaseInstanceAttributes?

Copy link
Contributor Author

@akash1810 akash1810 Apr 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is a new, required property. Is it a breaking change for fromDatabaseInstanceAttributes?

Ahh the AWS CodeBuild report suggests it is breaking:

err  - IFACE aws-cdk-lib.aws_rds.DatabaseInstanceAttributes: newly required property 'instanceResourceId' added: input to aws-cdk-lib.aws_rds.DatabaseInstanceBase.fromDatabaseInstanceAttributes [strengthened:aws-cdk-lib.aws_rds.DatabaseInstanceAttributes]
Some packages seem to have undergone breaking API changes. Please avoid.

Advice on how to proceed welcomed 🙏🏽.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can change this to an optional property in IDatabaseInstance and DatabaseInstanceBase and fromDatabaseInstanceAttributes.

Copy link
Contributor Author

@akash1810 akash1810 Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think you can change this to an optional property in IDatabaseInstance and DatabaseInstanceBase and fromDatabaseInstanceAttributes.

Done in ce630ec.

I'm not a huge fan of this as it means one has to ensure the property is not undefined when accessing it, which adds bloat IMO:

const db = new DatabaseInstance();
const resourceId: string | undefined = db.instanceResourceId;

if (!resourceId) {
  // unhappy path
}
else {
  // happy path
}

Alternatively, one can use non-null assertion, however I've worked in projects where this would produce a lint warning.

const db = new DatabaseInstance();
const resourceId: string = db.instanceResourceId!;

Additionally, this being optional makes it the only optional property1. Not a problem in general, however might be an indication of not being desired?

Footnotes

  1. enableIamAuthentication doesn't really count, as an optional boolean still has two values: true = true, false|undefined = false. ↩

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's not great, but it's the only way to avoid breaking changes.

}

return new Import(scope, id);
Expand All @@ -138,6 +156,7 @@ export abstract class DatabaseInstanceBase extends Resource implements IDatabase
public abstract readonly instanceIdentifier: string;
public abstract readonly dbInstanceEndpointAddress: string;
public abstract readonly dbInstanceEndpointPort: string;
public abstract readonly instanceResourceId?: string;
public abstract readonly instanceEndpoint: Endpoint;
// only required because of JSII bug: https://github.com/aws/jsii/issues/2040
public abstract readonly engine?: IInstanceEngine;
Expand All @@ -158,16 +177,33 @@ export abstract class DatabaseInstanceBase extends Resource implements IDatabase
});
}

public grantConnect(grantee: iam.IGrantable): iam.Grant {
public grantConnect(grantee: iam.IGrantable, dbUser?: string): iam.Grant {
if (this.enableIamAuthentication === false) {
throw new Error('Cannot grant connect when IAM authentication is disabled');
}

if (!this.instanceResourceId) {
throw new Error('For imported Database Instances, instanceResourceId is required to grantConnect()');
}

if (!dbUser) {
throw new Error('For imported Database Instances, the dbUser is required to grantConnect()');
}

this.enableIamAuthentication = true;
return iam.Grant.addToPrincipal({
grantee,
actions: ['rds-db:connect'],
resourceArns: [this.instanceArn],
resourceArns: [
// The ARN of an IAM policy for IAM database access is not the same as the instance ARN, so we cannot use `this.instanceArn`.
// See https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.IAMDBAuth.IAMPolicy.html
Stack.of(this).formatArn({
arnFormat: ArnFormat.COLON_RESOURCE_NAME,
service: 'rds-db',
resource: 'dbuser',
resourceName: [this.instanceResourceId, dbUser].join('/'),
}),
],
});
}

Expand Down Expand Up @@ -1029,6 +1065,26 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
target: this,
});
}

/**
* Grant the given identity connection access to the database.
*
* @param grantee the Principal to grant the permissions to
* @param dbUser the name of the database user to allow connecting as to the db instance
*
* @default the default user, obtained from the Secret
*/
public grantConnect(grantee: iam.IGrantable, dbUser?: string): iam.Grant {
if (!dbUser) {
if (!this.secret) {
throw new Error('A secret or dbUser is required to grantConnect()');
}

dbUser = this.secret.secretValueFromJson('username').toString();
}

return super.grantConnect(grantee, dbUser);
}
}

/**
Expand Down Expand Up @@ -1074,6 +1130,7 @@ export class DatabaseInstance extends DatabaseInstanceSource implements IDatabas
public readonly instanceIdentifier: string;
public readonly dbInstanceEndpointAddress: string;
public readonly dbInstanceEndpointPort: string;
public readonly instanceResourceId?: string;
public readonly instanceEndpoint: Endpoint;
public readonly secret?: secretsmanager.ISecret;

Expand All @@ -1095,6 +1152,7 @@ export class DatabaseInstance extends DatabaseInstanceSource implements IDatabas
this.instanceIdentifier = this.getResourceNameAttribute(instance.ref);
this.dbInstanceEndpointAddress = instance.attrEndpointAddress;
this.dbInstanceEndpointPort = instance.attrEndpointPort;
this.instanceResourceId = instance.attrDbiResourceId;

// create a number token that represents the port of the instance
const portAttribute = Token.asNumber(instance.attrEndpointPort);
Expand Down Expand Up @@ -1141,6 +1199,7 @@ export class DatabaseInstanceFromSnapshot extends DatabaseInstanceSource impleme
public readonly instanceIdentifier: string;
public readonly dbInstanceEndpointAddress: string;
public readonly dbInstanceEndpointPort: string;
public readonly instanceResourceId?: string;
public readonly instanceEndpoint: Endpoint;
public readonly secret?: secretsmanager.ISecret;

Expand Down Expand Up @@ -1172,6 +1231,7 @@ export class DatabaseInstanceFromSnapshot extends DatabaseInstanceSource impleme
this.instanceIdentifier = instance.ref;
this.dbInstanceEndpointAddress = instance.attrEndpointAddress;
this.dbInstanceEndpointPort = instance.attrEndpointPort;
this.instanceResourceId = instance.attrDbiResourceId;

// create a number token that represents the port of the instance
const portAttribute = Token.asNumber(instance.attrEndpointPort);
Expand Down Expand Up @@ -1229,6 +1289,7 @@ export class DatabaseInstanceReadReplica extends DatabaseInstanceNew implements
public readonly instanceIdentifier: string;
public readonly dbInstanceEndpointAddress: string;
public readonly dbInstanceEndpointPort: string;
public readonly instanceResourceId?: string;
public readonly instanceEndpoint: Endpoint;
public readonly engine?: IInstanceEngine = undefined;
protected readonly instanceType: ec2.InstanceType;
Expand Down Expand Up @@ -1260,6 +1321,7 @@ export class DatabaseInstanceReadReplica extends DatabaseInstanceNew implements
this.instanceIdentifier = instance.ref;
this.dbInstanceEndpointAddress = instance.attrEndpointAddress;
this.dbInstanceEndpointPort = instance.attrEndpointPort;
this.instanceResourceId = instance.attrDbInstanceArn;

// create a number token that represents the port of the instance
const portAttribute = Token.asNumber(instance.attrEndpointPort);
Expand Down
81 changes: 80 additions & 1 deletion packages/aws-cdk-lib/aws-rds/test/instance.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1112,7 +1112,86 @@ describe('instance', () => {
Effect: 'Allow',
Action: 'rds-db:connect',
Resource: {
'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':rds:', { Ref: 'AWS::Region' }, ':', { Ref: 'AWS::AccountId' }, ':db:', { Ref: 'InstanceC1063A87' }]],
'Fn::Join': [
'',
[
'arn:',
{
Ref: 'AWS::Partition',
},
':rds-db:',
{
Ref: 'AWS::Region',
},
':',
{
Ref: 'AWS::AccountId',
},
':dbuser:',
{
'Fn::GetAtt': [
'InstanceC1063A87',
'DbiResourceId',
],
},
'/{{resolve:secretsmanager:',
{
Ref: 'InstanceSecretAttachment83BEE581',
},
':SecretString:username::}}',
],
],
},
}],
Version: '2012-10-17',
},
});
});

test('createGrant - creates IAM policy and enables IAM auth for a specific user', () => {
const instance = new rds.DatabaseInstance(stack, 'Instance', {
engine: rds.DatabaseInstanceEngine.mysql({ version: rds.MysqlEngineVersion.VER_8_0_19 }),
vpc,
});
const role = new Role(stack, 'DBRole', {
assumedBy: new AccountPrincipal(stack.account),
});
instance.grantConnect(role, 'my-user');

Template.fromStack(stack).hasResourceProperties('AWS::RDS::DBInstance', {
EnableIAMDatabaseAuthentication: true,
});
Template.fromStack(stack).hasResourceProperties('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:',
{
'Fn::GetAtt': [
'InstanceC1063A87',
'DbiResourceId',
],
},
'/my-user',
],
],
},
}],
Version: '2012-10-17',
Expand Down