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

Enhance security: Cloudfront distributions should use security response headers #529

Merged

Conversation

nikpodsh
Copy link
Contributor

Feature or Bugfix

  • Feature

Detail

To avoid the following attacks: cross site scripting, click-hijacking, + also MIME type sniffing,
the security response headers have been added to all cloudfront distribution.

Since cloudfront's CloudFrontWebDistribution is not supporting response policies (it can only be done by escape hatches), all distribution have been migrated to Distribution interface. Along with that, the legacy caching was migrated to CACHE_OPTIMIZED which is recommended for S3 origins.
The headers in the documentation distribution:

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

chamcca and others added 4 commits June 13, 2023 16:28
### Feature or Bugfix
- Bugfix

### Detail
- fix how dynamic SQL with varying table names is generated

### Relates
- <URL or Ticket>

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
…ride react-redux to non-vulnerable version (data-dot-all#521)

### Feature or Bugfix
- Bugfix

### Detail
- Upgraded `fast-xml-parser`
- In the process I also found that other dependency libraries included
vulnerabilities. In particular `react-redux` and `nth-check`, the parent
packages `aws-amplify`, `react-scripts` and `appbaseio/reactivesearch`
have been updated. For this last one, the latest version still uses a
vulnerable version of `react-redux` so I added a ovverride clause in the
package.json

### Relates
- Related to
https://github.com/NaturalIntelligence/fast-xml-parser/releases/tag/v4.2.4

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

### Detail
- Resolve nth-check in sub-dependencies to version 2.0.1

### Relates

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
@nikpodsh
Copy link
Contributor Author

The headers in the documentation app
Headers for documentation
The headers in the data.all app
Headers for data all app
The policy is associated with the distributions:
The policy is associated

@dlpzx
Copy link
Contributor

dlpzx commented Jun 22, 2023

Hi thanks @nikpodsh!!! I have checked the PR and I deployed it in an existing deployment (merging your branch in my deployment branch)The headers include the secure headers policy controls 👍

The only issue that I saw is that the PR creates a new distribution instead of updating the existing one. This means that the domain name that customers were using would be invalid with this PR and their administrators will need to communicate end-users the new domain name (in the case they are using a CloudFront generated domain). I checked that in the new domain the connection with the Cognito user pool works and that all resources that were previously created still work, so it is just a communication challenge.
image

In the deployment, instead of using CloudFront ugly-domain names, customers can use their own custom domains by specifying this option in the cdk.json file. I have not tested this option but from the code it seems like it won't present the aforementioned issue as the distribution will use an alternate domain

@nikpodsh nikpodsh changed the base branch from main to v1m6m0 June 22, 2023 14:35
@nikpodsh
Copy link
Contributor Author

Changed the target branch to v1.6.0. Three are a few commits that have been merged to main but we don't have them in v1.6.0. Since awslabs:v1m6m0 is protected and can't be rebased to a fresh main, I will leave them in this PR. There should not be any collision then we will try to merge v1.6 -> main

@dlpzx dlpzx self-requested a review June 23, 2023 11:36
Copy link
Contributor

@dlpzx dlpzx left a comment

Choose a reason for hiding this comment

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

I see that it also includes the latest changes in main, good :)

@nikpodsh nikpodsh merged commit a48e79a into data-dot-all:v1m6m0 Jun 23, 2023
@dlpzx dlpzx mentioned this pull request Jul 11, 2023
dlpzx added a commit that referenced this pull request Jul 19, 2023
### Feature or Bugfix
Release PR with the following list of features. Refer to each PR for the
details

### Detail
- #498 
- #482 
- #543
- #524 (which also solves #531)
- #532 
- #535 
- #497 
- #515
- #529 
- #562 
- #455 
- #572 
- #567 
- #573 
- #579 
- #578 
- #582 

### Breaking changes - release notes
- ⚠️ IMPORTANT: upgrade to a version >V1.5.0 before upgrading to V1.6 to
avoid deletion of resources in custom resource deletion
- ⚠️ IMPORTANT: requires an update of environments and then datasets
after upgrading. Either using cdk.json parameter
`enable_update_dataall_stacks_in_cicd_pipeline`, waiting for overnight
update stack task, or manually updating first environments and then
datasets
- CloudFront distribution replace for #529 
- Additional EC2 permissions in CDK Synth CodeBuild stage for #543 -->
this can be avoided by upgrading to v1.5.6 before upgrading to v1.6.0
- local development affected by more restrictive pivotRole trust policy


### Relates
V1.6.0 release

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

---------

Co-authored-by: Gezim Musliaj <102723839+gmuslia@users.noreply.github.com>
Co-authored-by: Noah Paige <69586985+noah-paige@users.noreply.github.com>
Co-authored-by: nikpodsh <124577300+nikpodsh@users.noreply.github.com>
Co-authored-by: chamcca <40579012+chamcca@users.noreply.github.com>
Co-authored-by: Nikita Podshivalov <nikpodsh@amazon.com>
Co-authored-by: dbalintx <132444646+dbalintx@users.noreply.github.com>
Co-authored-by: mourya-33 <134511711+mourya-33@users.noreply.github.com>
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.

3 participants