From ad8b96150b6dfa80ba05f982bfcb4d9279f6704b Mon Sep 17 00:00:00 2001 From: biffgaut Date: Thu, 7 Sep 2023 22:09:18 -0400 Subject: [PATCH] Address Sonarqube issues --- .../aws-apigateway-dynamodb/lib/index.ts | 84 +++++++++++++++---- .../test/lambda-elasticsearch-kibana.test.ts | 19 +++++ .../test/lambda-opensearch.test.ts | 19 +++++ .../aws-wafwebacl-alb/lib/index.ts | 3 +- .../core/lib/alb-helper.ts | 3 + .../core/lib/elasticsearch-helper.ts | 5 +- .../core/lib/kendra-helper.ts | 4 +- .../core/lib/opensearch-helper.ts | 5 +- .../core/lib/s3-bucket-helper.ts | 38 ++++++++- .../core/lib/sagemaker-helper.ts | 6 +- .../core/lib/sns-helper.ts | 5 +- .../core/lib/sqs-helper.ts | 6 +- .../core/test/sagemaker-helper.test.ts | 21 +++++ 13 files changed, 195 insertions(+), 23 deletions(-) diff --git a/source/patterns/@aws-solutions-constructs/aws-apigateway-dynamodb/lib/index.ts b/source/patterns/@aws-solutions-constructs/aws-apigateway-dynamodb/lib/index.ts index 05724b22d..4fcd666a5 100644 --- a/source/patterns/@aws-solutions-constructs/aws-apigateway-dynamodb/lib/index.ts +++ b/source/patterns/@aws-solutions-constructs/aws-apigateway-dynamodb/lib/index.ts @@ -205,23 +205,19 @@ export class ApiGatewayToDynamoDB extends Construct { super(scope, id); defaults.CheckProps(props); - if ((props.createRequestTemplate || props.additionalCreateRequestTemplates || props.createIntegrationResponses) - && props.allowCreateOperation !== true) { + if (this.CheckCreateRequestProps(props)) { throw new Error(`The 'allowCreateOperation' property must be set to true when setting any of the following: ` + `'createRequestTemplate', 'additionalCreateRequestTemplates', 'createIntegrationResponses'`); } - if ((props.readRequestTemplate || props.additionalReadRequestTemplates || props.readIntegrationResponses) - && props.allowReadOperation === false) { + if (this.CheckReadRequestProps(props)) { throw new Error(`The 'allowReadOperation' property must be set to true or undefined when setting any of the following: ` + `'readRequestTemplate', 'additionalReadRequestTemplates', 'readIntegrationResponses'`); } - if ((props.updateRequestTemplate || props.additionalUpdateRequestTemplates || props.updateIntegrationResponses) - && props.allowUpdateOperation !== true) { + if (this.CheckUpdateRequestProps(props)) { throw new Error(`The 'allowUpdateOperation' property must be set to true when setting any of the following: ` + `'updateRequestTemplate', 'additionalUpdateRequestTemplates', 'updateIntegrationResponses'`); } - if ((props.deleteRequestTemplate || props.additionalDeleteRequestTemplates || props.deleteIntegrationResponses) - && props.allowDeleteOperation !== true) { + if (this.CheckDeleteRequestProps(props)) { throw new Error(`The 'allowDeleteOperation' property must be set to true when setting any of the following: ` + `'deleteRequestTemplate', 'additionalDeleteRequestTemplates', 'deleteIntegrationResponses'`); } @@ -261,8 +257,9 @@ export class ApiGatewayToDynamoDB extends Construct { // Setup API Gateway Method // Create - if (props.allowCreateOperation && props.allowCreateOperation === true && props.createRequestTemplate) { - const createRequestTemplate = props.createRequestTemplate.replace("${Table}", this.dynamoTable.tableName); + if (this.ImplementCreateOperation(props)) { + // ImplementCreateOperation has confirmed that createRequestTemplate exists) + const createRequestTemplate = props.createRequestTemplate!.replace("${Table}", this.dynamoTable.tableName); this.addActionToPolicy("dynamodb:PutItem"); defaults.addProxyMethodToApiResource({ service: "dynamodb", @@ -276,7 +273,7 @@ export class ApiGatewayToDynamoDB extends Construct { }); } // Read - if (props.allowReadOperation === undefined || props.allowReadOperation === true) { + if (this.ImplementReaOperation(props)) { const readRequestTemplate = props.readRequestTemplate ?? `{ \ "TableName": "${this.dynamoTable.tableName}", \ @@ -301,8 +298,9 @@ export class ApiGatewayToDynamoDB extends Construct { }); } // Update - if (props.allowUpdateOperation && props.allowUpdateOperation === true && props.updateRequestTemplate) { - const updateRequestTemplate = props.updateRequestTemplate.replace("${Table}", this.dynamoTable.tableName); + if (this.ImplementUpdateOperation(props)) { + // ImplementUpdateOperation confirmed the existence of updateRequestTemplate + const updateRequestTemplate = props.updateRequestTemplate!.replace("${Table}", this.dynamoTable.tableName); this.addActionToPolicy("dynamodb:UpdateItem"); defaults.addProxyMethodToApiResource({ service: "dynamodb", @@ -316,7 +314,7 @@ export class ApiGatewayToDynamoDB extends Construct { }); } // Delete - if (props.allowDeleteOperation && props.allowDeleteOperation === true) { + if (this.ImplementDeleteOperation(props)) { const deleteRequestTemplate = props.deleteRequestTemplate ?? `{ \ "TableName": "${this.dynamoTable.tableName}", \ @@ -342,6 +340,64 @@ export class ApiGatewayToDynamoDB extends Construct { } } + private CheckReadRequestProps(props: ApiGatewayToDynamoDBProps): boolean { + if ((props.readRequestTemplate || props.additionalReadRequestTemplates || props.readIntegrationResponses) + && props.allowReadOperation === false) { + return true; + } + return false; + } + private CheckUpdateRequestProps(props: ApiGatewayToDynamoDBProps): boolean { + if ((props.updateRequestTemplate || props.additionalUpdateRequestTemplates || props.updateIntegrationResponses) + && props.allowUpdateOperation !== true) { + return true; + } + return false; + } + private CheckDeleteRequestProps(props: ApiGatewayToDynamoDBProps): boolean { + if ((props.deleteRequestTemplate || props.additionalDeleteRequestTemplates || props.deleteIntegrationResponses) + && props.allowDeleteOperation !== true) { + return true; + } + return false; + } + + private CheckCreateRequestProps(props: ApiGatewayToDynamoDBProps): boolean { + if ((props.createRequestTemplate || props.additionalCreateRequestTemplates || props.createIntegrationResponses) + && props.allowCreateOperation !== true) { + return true; + } + return false; + } + + private ImplementCreateOperation(props: ApiGatewayToDynamoDBProps): boolean { + if (props.allowCreateOperation && props.allowCreateOperation === true && props.createRequestTemplate) { + return true; + } + return false; + } + + private ImplementReaOperation(props: ApiGatewayToDynamoDBProps): boolean { + if (props.allowReadOperation === undefined || props.allowReadOperation === true) { + return true; + } + return false; + } + + private ImplementUpdateOperation(props: ApiGatewayToDynamoDBProps): boolean { + if (props.allowUpdateOperation && props.allowUpdateOperation === true && props.updateRequestTemplate) { + return true; + } + return false; + } + + private ImplementDeleteOperation(props: ApiGatewayToDynamoDBProps): boolean { + if (props.allowDeleteOperation && props.allowDeleteOperation === true) { + return true; + } + return false; + } + private addActionToPolicy(action: string) { this.apiGatewayRole.addToPolicy(new iam.PolicyStatement({ resources: [ diff --git a/source/patterns/@aws-solutions-constructs/aws-lambda-elasticsearch-kibana/test/lambda-elasticsearch-kibana.test.ts b/source/patterns/@aws-solutions-constructs/aws-lambda-elasticsearch-kibana/test/lambda-elasticsearch-kibana.test.ts index 59452bd7d..94ea22461 100644 --- a/source/patterns/@aws-solutions-constructs/aws-lambda-elasticsearch-kibana/test/lambda-elasticsearch-kibana.test.ts +++ b/source/patterns/@aws-solutions-constructs/aws-lambda-elasticsearch-kibana/test/lambda-elasticsearch-kibana.test.ts @@ -93,6 +93,25 @@ test('check properties with no CW Alarms ', () => { expect(construct.elasticsearchRole).toBeDefined(); }); +test('Check that TLS 1.2 is the default', () => { + const stack = new cdk.Stack(); + + const props: LambdaToElasticSearchAndKibanaProps = { + lambdaFunctionProps: getDefaultTestLambdaProps(), + domainName: 'test-domain', + createCloudWatchAlarms: false + }; + + new LambdaToElasticSearchAndKibana(stack, 'test-lambda-opensearch-stack', props); + const template = Template.fromStack(stack); + template.hasResourceProperties("AWS::Elasticsearch::Domain", { + DomainEndpointOptions: { + EnforceHTTPS: true, + TLSSecurityPolicy: "Policy-Min-TLS-1-2-2019-07" + }, + }); +}); + test('check lambda function custom environment variable', () => { const stack = new cdk.Stack(); const props: LambdaToElasticSearchAndKibanaProps = { diff --git a/source/patterns/@aws-solutions-constructs/aws-lambda-opensearch/test/lambda-opensearch.test.ts b/source/patterns/@aws-solutions-constructs/aws-lambda-opensearch/test/lambda-opensearch.test.ts index a3620631e..47013d07c 100644 --- a/source/patterns/@aws-solutions-constructs/aws-lambda-opensearch/test/lambda-opensearch.test.ts +++ b/source/patterns/@aws-solutions-constructs/aws-lambda-opensearch/test/lambda-opensearch.test.ts @@ -69,6 +69,25 @@ test('Check properties with no CloudWatch alarms ', () => { expect(construct.openSearchRole).toBeDefined(); }); +test('Check that TLS 1.2 is the default', () => { + const stack = new cdk.Stack(); + + const props: LambdaToOpenSearchProps = { + lambdaFunctionProps: getDefaultTestLambdaProps(), + openSearchDomainName: 'test-domain', + createCloudWatchAlarms: false + }; + + new LambdaToOpenSearch(stack, 'test-lambda-opensearch-stack', props); + const template = Template.fromStack(stack); + template.hasResourceProperties("AWS::OpenSearchService::Domain", { + DomainEndpointOptions: { + EnforceHTTPS: true, + TLSSecurityPolicy: "Policy-Min-TLS-1-2-2019-07" + }, + }); +}); + test('Check for an existing Lambda object', () => { const stack = new cdk.Stack(); diff --git a/source/patterns/@aws-solutions-constructs/aws-wafwebacl-alb/lib/index.ts b/source/patterns/@aws-solutions-constructs/aws-wafwebacl-alb/lib/index.ts index 55d74c8d1..d37c75e8b 100644 --- a/source/patterns/@aws-solutions-constructs/aws-wafwebacl-alb/lib/index.ts +++ b/source/patterns/@aws-solutions-constructs/aws-wafwebacl-alb/lib/index.ts @@ -67,7 +67,8 @@ export class WafwebaclToAlb extends Construct { }; // Setup the Web ACL Association - new waf.CfnWebACLAssociation(scope, `${id}-WebACLAssociation`, aclProps); + // Before turning off SonarQube for the line, reduce the line to it's minimum + new waf.CfnWebACLAssociation(scope, `${id}-WebACLAssociation`, aclProps); // NOSONAR this.loadBalancer = props.existingLoadBalancerObj; } diff --git a/source/patterns/@aws-solutions-constructs/core/lib/alb-helper.ts b/source/patterns/@aws-solutions-constructs/core/lib/alb-helper.ts index 8441bd6b5..34f8b8a03 100644 --- a/source/patterns/@aws-solutions-constructs/core/lib/alb-helper.ts +++ b/source/patterns/@aws-solutions-constructs/core/lib/alb-helper.ts @@ -112,6 +112,9 @@ export function AddListener( protocol: "HTTPS", }; + // NOSONAR: (typescript:S5332) + // This listener is explicitly created to redirect non TLS connections + // The lack of SSL/TLS is intentional const httpListener = new elb.ApplicationListener( scope, `${id}-redirect`, diff --git a/source/patterns/@aws-solutions-constructs/core/lib/elasticsearch-helper.ts b/source/patterns/@aws-solutions-constructs/core/lib/elasticsearch-helper.ts index 73e8d2a02..be82d4981 100644 --- a/source/patterns/@aws-solutions-constructs/core/lib/elasticsearch-helper.ts +++ b/source/patterns/@aws-solutions-constructs/core/lib/elasticsearch-helper.ts @@ -83,7 +83,10 @@ export function buildElasticSearch(scope: Construct, props: BuildElasticSearchPr const defaultCfnDomainProps = DefaultCfnDomainProps(props.domainName, cognitoKibanaConfigureRole, props); const finalCfnDomainProps = consolidateProps(defaultCfnDomainProps, props.clientDomainProps, constructDrivenProps); - const esDomain = new elasticsearch.CfnDomain(scope, `ElasticsearchDomain`, finalCfnDomainProps); + // tlsSecurityPolicy is set in DefaultCfnDomainProps() - it is the + // default behavior, but Sonarqube cannot follow the program flow to confirm this. + // This is confirmed by the 'Check that TLS 1.2 is the default' test in aws-lambda-elasticsearch + const esDomain = new elasticsearch.CfnDomain(scope, `ElasticsearchDomain`, finalCfnDomainProps); // NOSONAR addCfnSuppressRules(esDomain, [ { diff --git a/source/patterns/@aws-solutions-constructs/core/lib/kendra-helper.ts b/source/patterns/@aws-solutions-constructs/core/lib/kendra-helper.ts index 0b88b9edc..65d94c116 100644 --- a/source/patterns/@aws-solutions-constructs/core/lib/kendra-helper.ts +++ b/source/patterns/@aws-solutions-constructs/core/lib/kendra-helper.ts @@ -18,13 +18,13 @@ import * as kendra from 'aws-cdk-lib/aws-kendra'; import * as iam from 'aws-cdk-lib/aws-iam'; -import { addCfnSuppressRules, consolidateProps } from "./utils"; +import { addCfnSuppressRules, consolidateProps, generatePhysicalName, overrideProps } from "./utils"; import { Aws } from 'aws-cdk-lib'; // Note: To ensure CDKv2 compatibility, keep the import statement for Construct separate import { Construct } from 'constructs'; import { DefaultKendraIndexProps } from './kendra-defaults'; -import { generatePhysicalName, overrideProps } from "./utils"; +import { } from "./utils"; export interface BuildKendraIndexProps { readonly kendraIndexProps?: kendra.CfnIndexProps | any; diff --git a/source/patterns/@aws-solutions-constructs/core/lib/opensearch-helper.ts b/source/patterns/@aws-solutions-constructs/core/lib/opensearch-helper.ts index b3844204f..7393177e3 100644 --- a/source/patterns/@aws-solutions-constructs/core/lib/opensearch-helper.ts +++ b/source/patterns/@aws-solutions-constructs/core/lib/opensearch-helper.ts @@ -82,7 +82,10 @@ export function buildOpenSearch(scope: Construct, props: BuildOpenSearchProps): const defaultCfnDomainProps = DefaultOpenSearchCfnDomainProps(props.openSearchDomainName, cognitoDashboardConfigureRole, props); const finalCfnDomainProps = consolidateProps(defaultCfnDomainProps, props.clientDomainProps, constructDrivenProps); - const opensearchDomain = new opensearch.CfnDomain(scope, `OpenSearchDomain`, finalCfnDomainProps); + // tlsSecurityPolicy is set in DefaultOpenSearchCfnDomainProps() - it is the + // default behavior, but Sonarqube cannot follow the program flow to confirm this. + // This is confirmed by the 'Check that TLS 1.2 is the default' test in aws-lambda-opensearch + const opensearchDomain = new opensearch.CfnDomain(scope, `OpenSearchDomain`, finalCfnDomainProps); // NOSONAR addCfnSuppressRules(opensearchDomain, [ { diff --git a/source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts b/source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts index 7bcf603cf..5149aa76f 100644 --- a/source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts +++ b/source/patterns/@aws-solutions-constructs/core/lib/s3-bucket-helper.ts @@ -61,7 +61,19 @@ export function createLoggingBucket(scope: Construct, const combinedBucketProps = consolidateProps(DefaultS3Props(), loggingBucketProps); // Create the Logging Bucket - const loggingBucket: s3.Bucket = new s3.Bucket(scope, bucketId, combinedBucketProps); + // NOSONAR (typescript:S6281) + // Block Public Access is set by DefaultS3Props, but Sonarqube can't detect it + // It is verified by 's3 bucket with default props' in the unit tests + // NOSONAR (typescript:S6245) + // Encryption is turned on in the default properties that Sonarqube doesn't see + // Verified by unit test 's3 bucket with default props' + // NOSONAR (typescript:S6249) + // enforceSSL is turned on in the default properties that Sonarqube doesn't see + // Verified by unit test 's3 bucket with default props' + // NOSONAR (typescript:typescript:S6249) + // versioning is turned on in the default properties that Sonarqube doesn't see + // Verified by unit test 's3 bucket with default props' + const loggingBucket: s3.Bucket = new s3.Bucket(scope, bucketId, combinedBucketProps); // NOSONAR // Extract the CfnBucket from the loggingBucket const loggingBucketResource = loggingBucket.node.findChild('Resource') as s3.CfnBucket; @@ -94,6 +106,18 @@ export function createAlbLoggingBucket(scope: Construct, const combinedBucketProps = consolidateProps(DefaultS3Props(), loggingBucketProps); // Create the Logging Bucket + // NOSONAR (typescript:S6281) + // Block Public Access is set by DefaultS3Props, but Sonarqube can't detect it + // It is verified by 's3 bucket with default props' in the unit tests + // NOSONAR (typescript:S6245) + // Encryption is turned on in the default properties that Sonarqube doesn't see + // Verified by unit test 's3 bucket with default props' + // NOSONAR (typescript:S6249) + // enforceSSL is turned on in the default properties that Sonarqube doesn't see + // Verified by unit test 's3 bucket with default props' + // NOSONAR (typescript:typescript:S6249) + // versioning is turned on in the default properties that Sonarqube doesn't see + // Verified by unit test 's3 bucket with default props' const loggingBucket: s3.Bucket = new s3.Bucket(scope, bucketId, combinedBucketProps); // Extract the CfnBucket from the loggingBucket @@ -163,6 +187,18 @@ export function buildS3Bucket(scope: Construct, const combinedBucketProps = consolidateProps(defaultBucketProps, props.bucketProps); + // NOSONAR (typescript:S6281) - Block Public Access is set by DefaultS3Props, + // but Sonarqube can't detect it + // It is verified by 's3 bucket with default props' in the unit tests + // NOSONAR (typescript:S6245) + // Encryption is turned on in the default properties that Sonarqube doesn't see + // Verified by unit test 's3 bucket with default props' + // NOSONAR (typescript:S6249) + // enforceSSL is turned on in the default properties that Sonarqube doesn't see + // Verified by unit test 's3 bucket with default props' + // NOSONAR (typescript:typescript:S6249) + // versioning is turned on in the default properties that Sonarqube doesn't see + // Verified by unit test 's3 bucket with default props' const s3Bucket: s3.Bucket = new s3.Bucket(scope, resolvedBucketId, combinedBucketProps ); return { bucket: s3Bucket, loggingBucket }; diff --git a/source/patterns/@aws-solutions-constructs/core/lib/sagemaker-helper.ts b/source/patterns/@aws-solutions-constructs/core/lib/sagemaker-helper.ts index 9ea8f7786..58f48d381 100644 --- a/source/patterns/@aws-solutions-constructs/core/lib/sagemaker-helper.ts +++ b/source/patterns/@aws-solutions-constructs/core/lib/sagemaker-helper.ts @@ -295,11 +295,15 @@ export function buildSagemakerNotebook( sagemakerNotebookProps = consolidateProps(sagemakerNotebookProps, props.sagemakerNotebookProps); // Create the notebook + // NOSONAR: (typescript:S6319) + // keyID is created above in the if (props.sagemakerNotebookProps?.kmsKeyId === undefined) + // block. It is then passed to DefaultSagemakerNotebookProps() + // This behavior is validated in unit test const sagemakerInstance: sagemaker.CfnNotebookInstance = new sagemaker.CfnNotebookInstance( scope, 'SagemakerNotebook', sagemakerNotebookProps - ); + ); // NOSONAR if (vpcInstance) { return { notebook: sagemakerInstance, vpc: vpcInstance, securityGroup }; } else { diff --git a/source/patterns/@aws-solutions-constructs/core/lib/sns-helper.ts b/source/patterns/@aws-solutions-constructs/core/lib/sns-helper.ts index 5e0c61beb..883a82b92 100644 --- a/source/patterns/@aws-solutions-constructs/core/lib/sns-helper.ts +++ b/source/patterns/@aws-solutions-constructs/core/lib/sns-helper.ts @@ -160,7 +160,10 @@ export function buildTopic(scope: Construct, props: BuildTopicProps): BuildTopic } // Create the SNS Topic - const topic: sns.Topic = new sns.Topic(scope, 'SnsTopic', snsTopicProps); + // NOSONAR (typescript:S6327) - The masterKey is set in the if statement above, SONAR is + // not catching it. Behavior is confirmed in the + // 'Test deployment with no properties using AWS Managed KMS Key' unit test + const topic: sns.Topic = new sns.Topic(scope, 'SnsTopic', snsTopicProps); // NOSONAR applySecureTopicPolicy(topic); diff --git a/source/patterns/@aws-solutions-constructs/core/lib/sqs-helper.ts b/source/patterns/@aws-solutions-constructs/core/lib/sqs-helper.ts index bd49f1214..15fcec8c7 100644 --- a/source/patterns/@aws-solutions-constructs/core/lib/sqs-helper.ts +++ b/source/patterns/@aws-solutions-constructs/core/lib/sqs-helper.ts @@ -111,7 +111,11 @@ export function buildQueue(scope: Construct, id: string, props: BuildQueueProps) queueProps.encryptionMasterKey = buildEncryptionKey(scope, props.encryptionKeyProps); } - const queue = new sqs.Queue(scope, id, queueProps); + // NOSONAR (typescript:S6330) + // encryption is set to QueueEncryption.KMS_MANAGED by default in DefaultQueueProps, but + // Sonarqube can't parse the code well enough to see this. Encryption is confirmed by + // the 'Test deployment without imported encryption key' unit test + const queue = new sqs.Queue(scope, id, queueProps); // NOSONAR applySecureQueuePolicy(queue); diff --git a/source/patterns/@aws-solutions-constructs/core/test/sagemaker-helper.test.ts b/source/patterns/@aws-solutions-constructs/core/test/sagemaker-helper.test.ts index ac6e09bc8..a13347c82 100644 --- a/source/patterns/@aws-solutions-constructs/core/test/sagemaker-helper.test.ts +++ b/source/patterns/@aws-solutions-constructs/core/test/sagemaker-helper.test.ts @@ -81,6 +81,27 @@ test('Test deployment w/ existing VPC', () => { }); }); +test('Test default values encrypt notebook', () => { + // Stack + const stack = new Stack(); + const sagemakerRole = new iam.Role(stack, 'SagemakerRole', { + assumedBy: new iam.ServicePrincipal('sagemaker.amazonaws.com'), + }); + + // Build Sagemaker Notebook Instance + defaults.buildSagemakerNotebook(stack, { + role: sagemakerRole, + deployInsideVpc: false, + }); + // Assertion + const template = Template.fromStack(stack); + template.hasResourceProperties('AWS::SageMaker::NotebookInstance', { + KmsKeyId: { + Ref: "EncryptionKey1B843E66" + }, + }); +}); + test('Test deployment w/ override', () => { // Stack const stack = new Stack();