sst / ion

❍ — a new engine for SST
https://ion.sst.dev
MIT License
1.15k stars 141 forks source link

Permissions for non-managed resources: Unexpected ability to query timestream #213

Closed dmeehan1968 closed 2 months ago

dmeehan1968 commented 3 months ago

I'm adding SST/Ion to an existing CDK based application, and want to be able to access resources from the existing deployment from a NextJs application. The intention is to implement a dashboard UI that provides access to the existing resource, such as metrics stored in Timestream, service configurations in Dynamo etc. If all goes well I will import the resources into the SST application, but for now I just want to experiment with the frontend deployment.

My first test was to be able to query a Timestream table and produce a table of results in the UI. Timestream isn't a supported component type in SST, and anyway, the database/table already exists so I can't 'import' it as an SST managed resource, and all I need to be able to do is access it by name/ARN.

I was expecting to need to add appropriate permissions to the NextJs instance in the sst.config.ts, but I have been able to issue a query without doing so. I cannot see from the created IAM role/policies (app-user-DashboardDefaultRole-region) how the access is being granted, as this appears to be just the minimum required for SST access its resources (S3, Queue, Dynamo, lambda).

I have a single set of AWS credentials in \~/.aws/credentials which are for my AWS console user, and provides the necessary access for SST to deploy.

I'm a complete novice with SST (both CDK and Ion variants) so may not be approaching this correctly. In my CDK app that creates those resources, I use a least permissive approach as per AWS guidelines, so any existing resource such as a user or function is explicitly granted the permissions required (e.g. a function only gets Timestream Query permissions if that's all it needs). I don't think therefore that there can be an existing policy that would unintentionally grant access, other than the user that SST is using for deployment. The DashboardDefaultFunction has the DashboardDefaultRole assigned and the console permissions viewer only shows the expected resources and doesn't include Timestream.

This is my sst.config.ts file:

///

export default $config({
  app(input) {
    return {
      name: "replicated",
      removal: input?.stage === "production" ? "retain" : "remove",
      home: "aws",
    };
  },
  async run() {
    new sst.aws.Nextjs('Dashboard', {
      path: 'packages/dashboard',
    })
  },
});
dmeehan1968 commented 3 months ago

One thing I realised is that when working locally, I was using sst dev instead of sst dev next dev and was running next in dev mode separately. What I reported above was experienced both in sst dev mode, and after sst deploy when accessing the CF endpoint.

When I switch to sst dev next dev, I now get

"Error: The operation to discover endpoint failed. Please retry, or provide a custom endpoint and disable endpoint discovery to proceed."

which is what I had expected to encounter in the first place (without permissions).

However, if I add the permissions for timestream:DescribeEndpoints and timestream:Select to my config, it still fails with the same message, and inspecting the DashboardDefaultRole does not show the timestream permissions.

Here is the update Nextjs instance:

    new sst.aws.Nextjs('Dashboard', {
      path: 'packages/dashboard',
      permissions: [
        { actions: ['timestream:DescribeEndpoints'], resources: ['*'] },
        { actions: ['timestream:Select'], resources: ['arn:aws:timestream:eu-west-1:<REDACTED>:database/<REDACTED>/table/<REDACTED>'] },
      ]
    })

I've tried a fresh deploy, but the IAM role doesn't seem to get updated with the permissions.

dmeehan1968 commented 3 months ago

On further investigation, it appears that Nextjs constructor does not use the permissions key from the args - so the passed permissions are never used. I managed to work around that, somewhat messily, by adding a transform to Function that adds the desired permissions:

    $transform(sst.aws.Function, (fn) => {
      if (fn.description === 'Dashboard server') {
        if (Array.isArray(fn.permissions)) {
          fn.permissions = [
              ...fn.permissions,
            {actions: ['timestream:DescribeEndpoints'], resources: ['*']},
            {
              actions: ['timestream:Select'],
              resources: ['arn:aws:timestream:eu-west-1:<REDACTED>:database/<REDACTED>/table/<REDACTED>']
            },
          ]
        }
      }
    })

This does cause the DashboardDefaultRole to be updated, but it still failed to load until I stopped the dev mode and restarted it. I was able to see that the dev mode was doing additional deploy steps as I added the code to sst.config.ts, and I was careful to wait for the deploy to complete before attempting to refresh the UI (and I did multiple refreshes even after the deploy was complete) so this points to something being overlooked in the change detection.

I tried to recreate this last part - removing the permissions, waiting for a deploy to complete, adding the permissions, waiting for deploy, then refreshing, but it worked on subsequent attempts. Note sure why this would have been.

Note that it was necessary to qualify whether the permissions is an array, and I'm suspecting that in some cases it won't be because the alternative seems to be to handle cases where the resource references are pending.

dmeehan1968 commented 3 months ago

There is a slightly cleaner way of intercepting the server function to add the permissions:

    new sst.aws.Nextjs('Dashboard', {
      path: 'packages/dashboard',
      transform: {
        server: (fn) => {
          if (Array.isArray(fn.permissions)) {
            fn.permissions = [
              ...fn.permissions,
              { actions: ['timestream:DescribeEndpoints'], resources: ['*'] },
              { actions: ['timestream:Select'], resources: ['arn:aws:timestream:eu-west-1:<REDACTED>:database/<REDACTED>/table/<REDACTED>']
              },
            ]
          }
        }
      }

It seems that the transform function was designed without taking account of Pulumi handling values that are T | Promise<T> | OutputInstance<T>, which makes alterations tricky. Pulumi contains a function output that is designed to unwrap any value and return an OutputInstance, whilst allowing changes to be made using the .apply() method.

The transform function only allows T | undefined, which doesn't allow for unwrapping. It would be better if the transform also allowed an OutputInstance so that you could then do:

{
  transform: {
    server: (args: FunctionArgs) => {
      return output(args).apply(args => {
          fn.permissions = [
            ...fn.permissions,
            { actions: ['timestream:DescribeEndpoints'], resources: ['*'] },
            { actions: ['timestream:Select'], resources: ['arn:aws:timestream:eu-west-1:<REDACTED>:database/<REDACTED>/table/<REDACTED>']
            },
          ]
      }
   }
}
dmeehan1968 commented 3 months ago

A useful helper function to disambiguate whether the function property is an Output or a Promise to apply the changes more succinctly:

function apply<T>(input: Input<T> | undefined, fn: (input: T) => void) {
  if (Output.isInstance(input)) {
    input.apply(fn)
  } else if (input instanceof Promise) {
    input.then(input => {
      fn(input)
      return input
    })
  } else if (input !== undefined) {
    fn(input)
  }
}

Use like:

      transform: {
        server: (fn) => {
          const databaseName = '<REDACTED>'
          const tableName = '<REDACTED>'
          apply(fn.permissions, perms => {
            perms.push({ actions: ['timestream:DescribeEndpoints'], resources: ['*'] })
            perms.push({ actions: ['timestream:Select'], resources: [`arn:aws:timestream:eu-west-1:<REDACTED>:database/${databaseName}/table/${tableName}`] })
          })
          apply(fn.environment, env => {
            env.METRICS_DATABASE_NAME = databaseName
            env.METRICS_TABLE_NAME = tableName
          })
        }
      }
JuniYadi commented 3 months ago

There is a slightly cleaner way of intercepting the server function to add the permissions:

    new sst.aws.Nextjs('Dashboard', {
      path: 'packages/dashboard',
      transform: {
        server: (fn) => {
          if (Array.isArray(fn.permissions)) {
            fn.permissions = [
              ...fn.permissions,
              { actions: ['timestream:DescribeEndpoints'], resources: ['*'] },
              { actions: ['timestream:Select'], resources: ['arn:aws:timestream:eu-west-1:<REDACTED>:database/<REDACTED>/table/<REDACTED>']
              },
            ]
          }
        }
      }

It seems that the transform function was designed without taking account of Pulumi handling values that are T | Promise<T> | OutputInstance<T>, which makes alterations tricky. Pulumi contains a function output that is designed to unwrap any value and return an OutputInstance, whilst allowing changes to be made using the .apply() method.

The transform function only allows T | undefined, which doesn't allow for unwrapping. It would be better if the transform also allowed an OutputInstance so that you could then do:

{
  transform: {
    server: (args: FunctionArgs) => {
      return output(args).apply(args => {
          fn.permissions = [
            ...fn.permissions,
            { actions: ['timestream:DescribeEndpoints'], resources: ['*'] },
            { actions: ['timestream:Select'], resources: ['arn:aws:timestream:eu-west-1:<REDACTED>:database/<REDACTED>/table/<REDACTED>']
            },
          ]
      }
   }
}

we should put this on documentation

fwang commented 2 months ago

Should be fixed in v0.0.324. You can now pass in permissions to the Nextjs component. And the permissions will be granted to the server function.

Feel free to reopen if the issue persists.

ennioVisco commented 2 months ago

Should be fixed in v0.0.324. You can now pass in permissions to the Nextjs component. And the permissions will be granted to the server function.

Feel free to reopen if the issue persists.

Is this option relevant also for other resources (basically related to https://github.com/sst/ion/issues/243), or just for Next? I think it would be very nice to have it at least also for lambdas