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

Feature/md 7071 set security headers #8

Closed
wants to merge 2 commits into from

Conversation

marns93
Copy link
Contributor

@marns93 marns93 commented Nov 8, 2023

@felix11h FYI
We added additional security headers for our websites.

@marns93 marns93 requested a review from pchorus November 8, 2023 13:41
@marns93 marns93 force-pushed the feature/MD-7071-set-security-headers branch from 7524e2a to c5a364a Compare November 8, 2023 13:50
return (
f"aws s3 sync dist/ {ARTIFACTS_BUCKET} --cache-control 'max-age=60' --delete --no-progress",
f"aws s3 cp {ARTIFACTS_BUCKET} {ARTIFACTS_BUCKET} --recursive --no-progress --exclude '*' "
f"--include {pattern} --metadata-directive REPLACE --content-type '{mime_type}' --cache-control '{max_age}'",
_get_metadata_command(),
Copy link

Choose a reason for hiding this comment

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

I think it would be important to check whether this overrides cache-control or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will not override the other settings.
But we have to check if it works like it is, because during the upload x-amz-meta- will be added automatically as a prefix...

Bildschirmfoto 2023-11-08 um 15 02 28

Copy link

Choose a reason for hiding this comment

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

Maybe S3 doesn't support it in the first place? Interestingly CloudFront advises to go for Lambda:

https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/example-function-add-security-headers.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So another option would be a CloudFront function, which returns the headers for the response. But we will execute the function for every response for the app... :(
Will check what options we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:D @zyv Same comment and idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think a better option would be adding response headers policy with CloudFront.
@zyv WDYT?

Bildschirmfoto 2023-11-08 um 15 10 51

Copy link

Choose a reason for hiding this comment

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

But we will execute the function for every response for the app... :(

Yes, but this with Lambda@Edge I think is annoying, but non-factor in terms of both costs and performance.

Copy link

Choose a reason for hiding this comment

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

Oh wow, this is cool. Do you have a link to the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we will close this PR and create a new one in moneymeets-pulumi by settings the response headers with CloudFront. There are more security headers we could set, which can be discussed.

@marns93
Copy link
Contributor Author

marns93 commented Nov 8, 2023

Close this PR, because we can't set the headers with the expected names. We will add a custom response policy to our CloudFront distribution. (#8 (comment))

@marns93 marns93 closed this Nov 8, 2023
@marns93 marns93 deleted the feature/MD-7071-set-security-headers branch November 9, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants