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

feat: support custom SSL certs in SQL lambda handler #2631

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

palpatim
Copy link
Member

@palpatim palpatim commented Jun 17, 2024

Description of changes

Adds support for custom SSL certs in SQL data sources. Changes:

  • Updates CDK Construct API to add an optional sslCertConfig attribute to the SQL data source configs
  • Updates the Lambda handler deployed to the customer account to look up the SSM paths and return the custom SSL certificate if found
  • Adds beginnings of support for Beta SQL Layer in CDK test

New CDK parameters & API

cc: @atierian, @swaminator

AmplifyGraphqlApi and AmplifyData constructs add a new sslCertConfig parameter to the SQL db configs. New config API:

Type declarations

// NEW
export interface SslCertConfig {}

// NEW
export interface SslCertSsmPathConfig extends SslCertConfig {
  readonly ssmPath: string | string[];
}

// MODIFIED
export interface SqlModelDataSourceSsmDbConnectionStringConfig {
  // ...
  // V--- Added
  readonly sslCertConfig?: SslCertConfig;
}

// MODIFIED
export interface SqlModelDataSourceSecretsManagerDbConnectionConfig {
  // ...
  // V--- Added
  readonly sslCertConfig?: SslCertConfig;
}

// MODIFIED
export interface SqlModelDataSourceSsmDbConnectionConfig {
  // ...
  // V--- Added
  readonly sslCertConfig?: SslCertConfig;
}

Call site

const api = new AmplifyGraphqlApi(stack, 'MyApi', {
  definition: AmplifyGraphqlDefinition.fromString(schema, {
    name: 'myStrategy',
    dbType: 'MYSQL',
    dbConnectionConfig: {
      connectionUriSsmPath: '/ssm/path/to/connectionUri',
      // V-------- THIS IS NEWLY ADDED
      sslCertConfig: {
        ssmPath: ['/ssm/path/to/sslCert/1', '/ssm/path/to/sslCert/2']
      }
    }

  }),
  authorizationModes,
});

Note that we should be able to provide a more streamlined interface in the data schema builder; the additional structure in the CDK construct is required to allow disjoint-union-type behavior in a JSII-compatible interface.

Description of how you validated changes

  • Unit tests
  • E2E tests. Ideally, we'd set up a database that uses a totally custom cert for validation, but Aurora doesn't support custom SSL. Instead, we'll validate that the handler & layer use the SSL cert we pass:
    • Test 1 specifies an invalid SSL cert, and is expected to fail attempting to connect to the database
    • Test 2 specifies a valid SSL cert that duplicates the RDS bundle, and is expected to succeed

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@palpatim palpatim marked this pull request as ready for review June 24, 2024 15:51
@palpatim palpatim requested review from a team as code owners June 24, 2024 15:51
@@ -364,7 +364,7 @@ export class SQLLambdaModelDataSourceStrategyFactory {
export type SqlModelDataSourceDbConnectionConfig = SqlModelDataSourceSecretsManagerDbConnectionConfig | SqlModelDataSourceSsmDbConnectionConfig | SqlModelDataSourceSsmDbConnectionStringConfig;

// @public
export interface SqlModelDataSourceSecretsManagerDbConnectionConfig {
export interface SqlModelDataSourceSecretsManagerDbConnectionConfig extends SslCertSsmPathAwareConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not ideal. When using secrets manager for credentials, I think we shouldn't use (mix) SSM for SSL certificate alone.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can
(1) for now, support ssl certificate only when using SSM config (which is enough to support Gen2 DX).
(2) later, we will support secret manager for SSL certificate.

Copy link
Member Author

@palpatim palpatim Jun 25, 2024

Choose a reason for hiding this comment

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

Discussed offline. Will remove from Secrets Manager Will rework the type interface to apply it to the SqlModelDataSourceDbConnectionConfig. Requirement is that we preserve future API extensibility, at the cost of additional structure at the declaration. Upcoming commit will accomplish this, and I'll summarize the change in this PR description for API BR & PM approval.

@palpatim palpatim force-pushed the palpatim.feat.custom-ssl-certs branch from 215fc4c to 57d26ad Compare June 26, 2024 18:40
- test: add initial rds-lambda tests
- test: add e2e tests
- test: add partial Aspects support for beta lambda layer
@palpatim palpatim force-pushed the palpatim.feat.custom-ssl-certs branch from 57d26ad to 7c837a6 Compare June 26, 2024 19:28
atierian
atierian previously approved these changes Jun 26, 2024
Copy link
Member

@atierian atierian left a comment

Choose a reason for hiding this comment

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

API BR approved.
(I did not review the implementation)

sundersc
sundersc previously approved these changes Jun 26, 2024
phani-srikar
phani-srikar previously approved these changes Jun 26, 2024
@palpatim palpatim dismissed stale reviews from phani-srikar, sundersc, and atierian via 9b8f6fe June 27, 2024 15:39
@palpatim
Copy link
Member Author

Reviewed offline with @swaminator and @atierian, both approved API changes.

@palpatim palpatim merged commit f444517 into main Jun 28, 2024
6 of 7 checks passed
@palpatim palpatim deleted the palpatim.feat.custom-ssl-certs branch June 28, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants