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.

Sign up for GitHub

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
Closed

Feature/md 7071 set security headers #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da2d30f
refactor(action): rename function for cache-control
marns93 Nov 8, 2023
c5a364a
feat(action): set additional security headers for html files
marns93 Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions s3_artifact/action.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import json
import os
import subprocess
from dataclasses import dataclass
Expand Down Expand Up @@ -58,7 +59,7 @@ def deploy(config: S3ArtifactConfig, artifacts_s3_path: str, environment: str) -


def upload(config: S3ArtifactConfig, target: str) -> Sequence[str]:
def _prepare_metadata_command(cache_config: S3ArtifactCustomMetadataConfig):
def _prepare_cache_control_and_content_type_command(cache_config: S3ArtifactCustomMetadataConfig):
content_type = cache_config.mime_type or DEFAULT_MIME_TYPES[Path(cache_config.path).suffix]
content_type_option = f"--content-type '{content_type}'"

Expand All @@ -70,9 +71,21 @@ def _prepare_metadata_command(cache_config: S3ArtifactCustomMetadataConfig):
def _get_default_cache_control():
return f"--cache-control '{config.default_cache_control}'" if config.default_cache_control else ""

metadata = json.dumps(
{
"X-Frame-Options": "SAMEORIGIN",
"Content-Security-Policy": "frame-src 'self'; frame-ancestors 'self'; object-src 'none';",
"Strict-Transport-Security": "max-age=31536000; includeSubDomains",
"X-Content-Type-Options": "nosniff",
},
)
return (
f"aws s3 sync {config.local_artifacts_path} {target} {_get_default_cache_control()} {S3_SYNC_OPTIONS}",
*(_prepare_metadata_command(custom_metadata) for custom_metadata in config.custom_metadata),
*(
_prepare_cache_control_and_content_type_command(custom_metadata)
for custom_metadata in config.custom_metadata
),
f"aws s3 cp {target} {target} {S3_CP_OPTIONS} --exclude '*' --include '*.html' --metadata '{metadata}'",
)


Expand Down
21 changes: 19 additions & 2 deletions tests/test_action.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import json
import os
from dataclasses import asdict
from tempfile import NamedTemporaryFile
Expand Down Expand Up @@ -29,11 +30,24 @@ def _get_website_config(cache: Sequence[S3ArtifactCustomMetadataConfig] = ()) ->
)


def _get_upload_commands(pattern: str, mime_type: str, max_age: str) -> tuple[str, str]:
def _get_metadata_command():
return (
f"aws s3 cp {ARTIFACTS_BUCKET} {ARTIFACTS_BUCKET} --recursive --no-progress --exclude '*' --include '*.html' "
f"--metadata '{json.dumps({
"X-Frame-Options": "SAMEORIGIN",
"Content-Security-Policy": "frame-src 'self'; frame-ancestors 'self'; object-src 'none';",
"Strict-Transport-Security": "max-age=31536000; includeSubDomains",
"X-Content-Type-Options": "nosniff",
})}'"
)


def _get_upload_commands(pattern: str, mime_type: str, max_age: str) -> tuple[str, str, str]:
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.

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

https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/understanding-response-headers-policies.html#understanding-response-headers-policies-security

There are some pre-defined ones, which are not exactly what we need, but we can create our custom one.

https://us-east-1.console.aws.amazon.com/cloudfront/v4/home#/policies/responseHeaders/create

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.

)


Expand All @@ -42,7 +56,10 @@ def test_upload(self):
# Test success without special metadata
self.assertEqual(
first=upload(config=_get_website_config(), target=ARTIFACTS_BUCKET),
second=(f"aws s3 sync dist/ {ARTIFACTS_BUCKET} --cache-control 'max-age=60' --delete --no-progress",),
second=(
f"aws s3 sync dist/ {ARTIFACTS_BUCKET} --cache-control 'max-age=60' --delete --no-progress",
_get_metadata_command(),
),
)

# Test success with special metadata
Expand Down