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.

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(aws-rds): correct Policy resource for Proxy::grantConnect() #12416

Merged
merged 8 commits into from
Feb 16, 2021

Conversation

jdvornek
Copy link
Contributor

@jdvornek jdvornek commented Jan 8, 2021

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

fixes aws#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.
@gitpod-io
Copy link

gitpod-io bot commented Jan 8, 2021

@jdvornek jdvornek force-pushed the fix/dbproxy-grantconnect-policy branch from a7b0843 to c86fd64 Compare January 8, 2021 07:07
@skinny85
Copy link
Contributor

skinny85 commented Jan 8, 2021

I have one general question. What's the user passed to grantConnect()? is that the same user that's the master user of the Database that the proxy is in front of? Or a different one?

@jdvornek
Copy link
Contributor Author

jdvornek commented Jan 8, 2021

The DatabaseProxy can be associated with many SecretsManager Secrets. Each of those holds the credentials for a user account in the underlying database. When you call to request an access token for IAM auth, you pass in the database user for the connection you're borrowing. RDS proxy finds an appropriate existing connection, or finds the secret associated with that user and starts a new one. In that way IAM auth is able to be as granular as the user accounts on the underlying database. At least, that's my understanding as a product user.

@skinny85
Copy link
Contributor

skinny85 commented Jan 8, 2021

OK. So the user has to be one of the users passed through the secrets property when creating the DatabaseProxy?

@jdvornek
Copy link
Contributor Author

jdvornek commented Jan 8, 2021

The documentation could be more clear, but that's my understanding and how it has worked practically for me in use.

I didn't know if databaseUser was the best term since "database" is used all over the library (I considered sqlUser as well), but "database user" is the term from the docs.

@skinny85
Copy link
Contributor

skinny85 commented Jan 8, 2021

Thanks! One last question 🙂. Does the iamAuth property need to be set on the Proxy for this to work? Or is that a separate thing?

@jdvornek
Copy link
Contributor Author

jdvornek commented Jan 8, 2021

If iamAuth is false the proxy uses native authentication and, I believe, just passes the credentials through to the database, ignoring any secrets. Calling grantConnect with iamAuth disabled wouldn't break anything, but it also wouldn't make sense.

@skinny85
Copy link
Contributor

skinny85 commented Jan 8, 2021

Thanks @jdvornek . Now that I have a clearer picture of what this entails, here's my idea.

Let's make the dbUser parameter in grantConnect() optional. This will make this change backwards-compatible. In the case when it's not passed, then we'll do the following check:

  1. If we're not dealing with an imported Proxy, and there was a single Secret passed in the secrets property, let's just use the username from that Secret. It seems like that will be the most common case for this functionality anyway...?
  2. In all other cases, error out saying providing dbUser is required.

Of course, if dbuser was passed, let's just use that.

Does this make sense?

@jdvornek
Copy link
Contributor Author

jdvornek commented Jan 8, 2021

I had considered making it optional via simply adding a wildcard at the end of the policy if a databaseUser were not passed; I've tested that in the Policy Simulator, but not in the wild. I don't believe the required properties are persisted beyond construction of the DatabaseProxy, to look up the users from the secrets would be a more invasive fix.

@skinny85
Copy link
Contributor

skinny85 commented Jan 8, 2021

Would you mind me pushing a few PRs commits to your branch? I think I can get it working 🙂.

@jdvornek
Copy link
Contributor Author

jdvornek commented Jan 8, 2021

No worries, go for it! I just figured I'd tread lightly, but if there's a better fix to be had I'm on board.

@skinny85
Copy link
Contributor

@jdvornek I've pushed the code that I'd like to see for this feature. Let me know what you think!

@jdvornek
Copy link
Contributor Author

jdvornek commented Jan 16, 2021

@skinny85

On proxy.ts:347 it looks like you reference DatabaseProxy.dbProxyName instead of the resource from the parsed ARN of the proxy. In my testing this did not yield the same value. Parsing the ARN gave prx-012345, whereas dbProxyName gives the ID you pass during construction of the proxy in CDK.

On another note I just wanted to confirm that the automatic reference of a single-secret-proxy's username would work with a proxy constructed with an imported secret, since the current implementation relies only on the secret's secretArn and the ability to grantRead to it.

@skinny85
Copy link
Contributor

skinny85 commented Jan 20, 2021

@skinny85

On proxy.ts:347 it looks like you reference DatabaseProxy.dbProxyName instead of the resource from the parsed ARN of the proxy. In my testing this did not yield the same value. Parsing the ARN gave prx-012345, whereas dbProxyName gives the ID you pass during construction of the proxy in CDK.

So, dbProxyName is defined here as a CloudFormation Ref. The documentation for that resource states:

When you pass the logical ID of this resource to the intrinsic Ref function, Ref returns the name of the DB proxy.

Does this address your concern @jdvornek?

On another note I just wanted to confirm that the automatic reference of a single-secret-proxy's username would work with a proxy constructed with an imported secret, since the current implementation relies only on the secret's secretArn and the ability to grantRead to it.

What's the worry here? Why do you think there's a difference in the current version between imported and new Secrets?

@jdvornek
Copy link
Contributor Author

@skinny85

Trying to construct a minimal example:

const cdk = require('@aws-cdk/core');
const ec2 = require('@aws-cdk/aws-ec2');
const rds = require('@aws-cdk/aws-rds');
const iam = require('@aws-cdk/aws-iam');

class CdkTestStack extends cdk.Stack {
  constructor(scope, id, props) {
    super(scope, id, props);
    const vpc = new ec2.Vpc(this, 'VPC');
    const cluster = new rds.DatabaseCluster(this, 'Database', {
      engine: rds.DatabaseClusterEngine.AURORA,
      instanceProps: { vpc },
    });
    const proxy = new rds.DatabaseProxy(this, 'ThisIsMyProxy', {
      proxyTarget: rds.ProxyTarget.fromCluster(cluster),
      secrets: [cluster.secret],
      vpc,
    });
    const role = new iam.Role(this, 'Role', {
      assumedBy: new iam.ServicePrincipal('ecs-tasks.amazonaws.com')
    });
    const userArn = cdk.Stack.of(this).formatArn({
      service: 'rds-db',
      resource: 'dbuser',
      resourceName: `${proxy.dbProxyName}/test`,
      sep: ':',
    });
    iam.Grant.addToPrincipal({
      grantee: role,
      actions: ['rds-db:connect'],
      resourceArns: [userArn],
    });
  }
}

module.exports = { CdkTestStack }

$ cdk synth yields sections

ThisIsMyProxy168A5452:
    Type: AWS::RDS::DBProxy
    Properties:
      Auth:
        - AuthScheme: SECRETS
          IAMAuth: DISABLED
          SecretArn:
            Ref: DatabaseSecretAttachmentE5D1B020
      DBProxyName: ThisIsMyProxy ##### <----- Proxy name
      EngineFamily: MYSQL
      RoleArn:
        Fn::GetAtt:
          - ThisIsMyProxyIAMRole0CCAB74A
          - Arn
      VpcSubnetIds:
        - Ref: VPCPrivateSubnet1Subnet8BCA10E0
        - Ref: VPCPrivateSubnet2SubnetCFCDAA7A
      RequireTLS: true
    Metadata:
      aws:cdk:path: CdkTestStack/ThisIsMyProxy/Resource

and

RoleDefaultPolicy5FFB7DAB:
    Type: AWS::IAM::Policy
    Properties:
      PolicyDocument:
        Statement:
          - Action: rds-db:connect
            Effect: Allow
            Resource:
              Fn::Join:
                - ""
                - - "arn:"
                  - Ref: AWS::Partition
                  - ":rds-db:"
                  - Ref: AWS::Region
                  - ":"
                  - Ref: AWS::AccountId
                  - ":dbuser:"
                  - Ref: ThisIsMyProxy168A5452
                  - /test

When I had previously tested that, it built an ARN like arn:aws:rds-db:us-west-2:012345:dbuser:ThisIsMyProxy/test when we need to construct one such as arn:aws:rds-db:us-west-2:012345:dbuser:prx-012345/test. That ID (formatted prx-012345) is not available as a return value on its own, but is available as a component of the DBProxyArn.

As for an imported Secret, I had no specific concerns, I was simply asking you if it was assumed that it would work.

@skinny85
Copy link
Contributor

@jdvornek I'm trying to verify the names issue, but for some reason I can't get a simple Stack with an Proxy to deploy correctly 😕. Here's my code, very similar to yours:

        const vpc = new ec2.Vpc(this, 'VPC', {
            maxAzs: 2,
            natGateways: 1,
            subnetConfiguration: [
                {
                    name: 'rds',
                    subnetType: ec2.SubnetType.PUBLIC,
                },
            ],
        });
        const cluster = new rds.DatabaseCluster(this, 'Database', {
            engine: rds.DatabaseClusterEngine.AURORA,
            instanceProps: {
                vpc,
                vpcSubnets: { subnetType: ec2.SubnetType.PUBLIC },
            },
            removalPolicy: cdk.RemovalPolicy.DESTROY,
        });
        const proxy = new rds.DatabaseProxy(this, 'ThisIsMyProxy', {
            proxyTarget: rds.ProxyTarget.fromCluster(cluster),
            secrets: [cluster.secret!],
            vpc,
        });
        const role = new iam.Role(this, 'Role', {
            assumedBy: new iam.ServicePrincipal('ecs-tasks.amazonaws.com')
        });
        const userArn = cdk.Stack.of(this).formatArn({
            service: 'rds-db',
            resource: 'dbuser',
            resourceName: `${proxy.dbProxyName}/test`,
            sep: ':',
        });
        iam.Grant.addToPrincipal({
            grantee: role,
            actions: ['rds-db:connect'],
            resourceArns: [userArn],
        });

However, cdk deploy fails after a very long time with a timeout:

11:24:58 AM | CREATE_FAILED        | AWS::RDS::DBProxyTargetGroup                | ThisIsMyProxyProxyTargetGroup1311A5BA
Resource timed out waiting for completion

Any idea what's wrong...? Is there something missing in the Proxy construct perhaps that makes this fail...?

@jdvornek
Copy link
Contributor Author

jdvornek commented Jan 21, 2021

@skinny85 do you have the fix from #12237?

@skinny85
Copy link
Contributor

Yes; from the generated template:

    "ThisIsMyProxyProxyTargetGroup1311A5BA": {
      "Type": "AWS::RDS::DBProxyTargetGroup",
      "Properties": {
        "DBProxyName": {
          "Ref": "ThisIsMyProxy168A5452"
        },
        "TargetGroupName": "default",
        "ConnectionPoolConfigurationInfo": {},
        "DBClusterIdentifiers": [
          {
            "Ref": "DatabaseB269D8BB"
          }
        ]
      },
      "DependsOn": [
        "DatabaseInstance1844F58FD",
        "DatabaseInstance2AA380DEE",
        "DatabaseB269D8BB",
        "DatabaseSecretAttachmentE5D1B020",
        "DatabaseSecret3B817195",
        "DatabaseSecurityGroup5C91FDCB",
        "DatabaseSubnets56F17B9A"
      ],
      "Metadata": {
        "aws:cdk:path": "RdsClusterWithProxyStack/ThisIsMyProxy/ProxyTargetGroup"
      }
    },

@jdvornek
Copy link
Contributor Author

jdvornek commented Feb 6, 2021

@skinny85 Any further luck here?

I'm not sure it has any bearing on the deploy hanging, but I wonder if, for proxies not passed securityGroups, a default SecurityGroup should be created and explicitly allowed to the proxyTarget?

@skinny85
Copy link
Contributor

skinny85 commented Feb 8, 2021

OK, this worked:

        const vpc = new ec2.Vpc(this, 'VPC', {
            maxAzs: 2,
            natGateways: 1,
            subnetConfiguration: [
                {
                    name: 'rds',
                    subnetType: ec2.SubnetType.PUBLIC,
                },
            ],
        });
        const cluster = new rds.DatabaseCluster(this, 'Database', {
            engine: rds.DatabaseClusterEngine.AURORA,
            instanceProps: {
                vpc,
                vpcSubnets: { subnetType: ec2.SubnetType.PUBLIC },
            },
            instances: 1,
            removalPolicy: cdk.RemovalPolicy.DESTROY,
        });
        const proxySecurityGroup = new ec2.SecurityGroup(this, 'ProxySecurityGroup', {
            vpc,
        });
        const proxy = new rds.DatabaseProxy(this, 'ThisIsMyProxy', {
            proxyTarget: rds.ProxyTarget.fromCluster(cluster),
            secrets: [cluster.secret!],
            vpc,
            securityGroups: [proxySecurityGroup],
        });
        proxy.connections.allowTo(cluster, ec2.Port.allTraffic());
        cluster.connections.allowTo(proxy, ec2.Port.allTraffic());

        const role = new iam.Role(this, 'Role', {
            assumedBy: new iam.ServicePrincipal('ecs-tasks.amazonaws.com')
        });
        const userArn = cdk.Stack.of(this).formatArn({
            service: 'rds-db',
            resource: 'dbuser',
            resourceName: `${proxy.dbProxyName}/test`,
            sep: ':',
        });
        iam.Grant.addToPrincipal({
            grantee: role,
            actions: ['rds-db:connect'],
            resourceArns: [userArn],
        });

Definitely looks like we have something missing with the default connection setup between the Proxy and the Cluster.

@skinny85
Copy link
Contributor

skinny85 commented Feb 8, 2021

OK, I can confirm you are in fact correct @jdvornek . There are two identifiers here:

  1. The proxy name, which is a required property of the CfnDBProxy resource. The CDK defaults it for you based on the id of the resource (which is a mistake, it should have actually been uniqueId, but here we are). The console also calls this the "proxy identifier", which is... unfortunate. This is what gets returned from the Ref function for the CfnDBProxy resource.
  2. The proxy ID, or generated name, or whatever you want to call it, which is part of the ARN of the Proxy, like this: arn:aws:rds:us-west-2:123456789012:db-proxy:prx-0cc3dd62f3492172d - the prx-0cc3dd62f3492172d part. It can be obtained from the DBProxyArn attribute of the CfnDBProxy resource.

Let me try and figure out the connections problems first, and I'll get back to this PR in a sec.

@skinny85
Copy link
Contributor

skinny85 commented Feb 9, 2021

PR fixing the connectivity Proxy issue: #12953

@skinny85
Copy link
Contributor

@jdvornek I've changed the code to use the Proxy generated identifier. Do you mind taking another look?

@jdvornek
Copy link
Contributor Author

@skinny85 looks good to me, thanks.

…nnect-policy

# Conflicts:
#	packages/@aws-cdk/aws-rds/test/proxy.test.ts
@mergify
Copy link
Contributor

mergify bot commented Feb 16, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 625c5a7
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Feb 16, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit b3197db into aws:master Feb 16, 2021
NovakGu pushed a commit to NovakGu/aws-cdk that referenced this pull request Feb 18, 2021
…12416)

fixes aws#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*
eladb pushed a commit that referenced this pull request Feb 22, 2021
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*
@alex9311
Copy link
Contributor

alex9311 commented May 24, 2022

hey all, I'm trying use an RDS proxy with iam auth into my postgres rds instance. I currently have a direct connection set up between my lambdas and rds instance via iam auth using the workaround here.

Why is a secret still required when using iamAuth: true for the proxy? I thought the whole point was that no passwords were used for iam auth.

@skinny85
Copy link
Contributor

skinny85 commented May 25, 2022

@alex9311 I think that secret is for the proxy connecting to the database itself, not clients connecting to the database/proxy directly.

@kevin-mitchell
Copy link

@alex9311 sorry to piggy back off of your comment, but I've found my way here with the same question as you. @skinny85's comment makes some sense to me, but I'm still at a bit of a loss for how to setup iamAuth with RDS Proxy in CDK. My basic goal (just a PoC) at this point is to allow a Lambda function to connect through RDS Proxy to a RDS Postgres DB (Aurora Serverless v2). I'm building out a multi-tenant setup, and I don't want ot have to update the RDS Proxy each time a new user is added, hence me wanting to connect through RDS Proxy with IAM, but with different DB credentials. Any chance you managed to get this sorted?

mergify bot pushed a commit that referenced this pull request Apr 20, 2023
The [IAM policy for IAM database access](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.IAMDBAuth.IAMPolicy.html) takes the form of:

```
arn:aws:rds-db:region:account-id:dbuser:DbiResourceId/db-user-name
```

Following aws-cloudformation/cloudformation-coverage-roadmap#105, this change updates the ARN used in `grantConnect` to this format.

Additionally, update the signature of `grantConnect` to take an optional `dbUser`, which is defaulted to the master username of the database, obtained via the available `Secret`.

This signature change also matches `grantConnect` in `DatabaseProxy`. See #12416.

Closes #11851.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-rds Related to Amazon Relational Database
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-rds): grantConnect for DatabaseProxy yields incorrect policy
5 participants