feat(site-publish): reconcile split-surface CORS #5

Closed
architect wants to merge 1 commits from architect/site-publish-cors into main
5 changed files with 156 additions and 2 deletions
Showing only changes of commit 95d984252a - Show all commits
+6
View File
@@ -52,6 +52,7 @@ artifacts:
- name: distributions - name: distributions
type: static type: static
content_dir: dist content_dir: dist
cors_origins: ['*']
publish: publish:
bucket: baseline-dist bucket: baseline-dist
credentials: credentials:
@@ -141,6 +142,11 @@ protected input from entering a public artifact through dereference. Split
storage endpoints are pinned to Garage, and each website storage endpoints are pinned to Garage, and each website
authority is derived from its bucket; a site cannot expose an arbitrary backend. authority is derived from its bucket; a site cannot expose an arbitrary backend.
`cors_origins` is reconciled as a bucket policy on every split publication. Values are either `*`
or HTTPS origins; browser access is limited to `GET` and `HEAD`. Omitting the field removes stale
CORS from that bucket. Protected artifacts cannot allow wildcard CORS. All immutable objects and
all bucket CORS policies complete before mutable channels change.
Each split route gets a bucket-specific `<bucket>.web.sjc001.fritzlab.net` Each split route gets a bucket-specific `<bucket>.web.sjc001.fritzlab.net`
ExternalName Service annotated to disable pass-host-header and a separate Ingress. Route ExternalName Service annotated to disable pass-host-header and a separate Ingress. Route
Ingresses share the hostname's certificate Secret. The access middleware and Ingresses share the hostname's certificate Secret. The access middleware and
+41
View File
@@ -8,6 +8,7 @@ import os
import re import re
import shutil import shutil
import subprocess import subprocess
import tempfile
from pathlib import Path from pathlib import Path
from urllib.error import HTTPError, URLError from urllib.error import HTTPError, URLError
from urllib.request import Request, urlopen from urllib.request import Request, urlopen
@@ -167,6 +168,41 @@ def publication_aws_env(artifact, credential_env_names=None):
return aws_env return aws_env
def configure_cors(bucket, origins, endpoint, aws_env):
"""Reconcile read-only browser access without exposing publication credentials."""
if origins is None:
return
if not origins:
run([
"aws", "--endpoint-url", endpoint, "s3api", "delete-bucket-cors",
"--bucket", bucket,
], env=aws_env)
return
config = {
"CORSRules": [{
"AllowedOrigins": origins,
"AllowedMethods": ["GET", "HEAD"],
"AllowedHeaders": ["*"],
"ExposeHeaders": ["ETag"],
"MaxAgeSeconds": 3600,
}],
}
with tempfile.NamedTemporaryFile("w", suffix=".json", encoding="utf-8") as handle:
json.dump(config, handle)
handle.flush()
run([
"aws", "--endpoint-url", endpoint, "s3api", "put-bucket-cors",
"--bucket", bucket, "--cors-configuration", f"file://{handle.name}",
], env=aws_env)
def configure_artifact_cors(artifact, credential_env_names=None):
configure_cors(
artifact["bucket"], artifact["cors_origins"], artifact["s3_endpoint"],
publication_aws_env(artifact, credential_env_names),
)
def publish_route_immutables(artifact, route, site_dir, credential_env_names=None): def publish_route_immutables(artifact, route, site_dir, credential_env_names=None):
"""Publish one route's immutable partitions during the global preflight.""" """Publish one route's immutable partitions during the global preflight."""
html_dir = site_dir / artifact["build_dir"] html_dir = site_dir / artifact["build_dir"]
@@ -312,6 +348,11 @@ def deploy_static(site_name, site_dir, action_dir, token, cfg):
publish_route_immutables( publish_route_immutables(
artifact_by_name[route["artifact"]], route, site_dir, credential_env_names, artifact_by_name[route["artifact"]], route, site_dir, credential_env_names,
) )
# Reconcile every browser-read policy before publishing mutable content.
# A CORS failure therefore cannot leave a new channel pointing at a release
# whose cross-origin assets browsers cannot consume.
for artifact in cfg["artifacts"]:
configure_artifact_cors(artifact, credential_env_names)
for route in cfg["routes"]: for route in cfg["routes"]:
s3_sync(artifact_by_name[route["artifact"]], route, site_dir, credential_env_names) s3_sync(artifact_by_name[route["artifact"]], route, site_dir, credential_env_names)
if cfg["compatibility"]: if cfg["compatibility"]:
+31 -1
View File
@@ -104,6 +104,26 @@ def _strings(value, label):
return values return values
def _cors_origins(value, label):
origins = _strings(value or [], label)
Review

You can mistype this field as cors_origins: "", false, 0, or {}, and each value becomes []. Deployment then calls delete-bucket-cors, so malformed input looks successful while browser access disappears. Preserve omission as the delete signal, but reject present values that aren't lists; add these falsey cases to the contract test.

You can mistype this field as `cors_origins: ""`, `false`, `0`, or `{}`, and each value becomes `[]`. Deployment then calls `delete-bucket-cors`, so malformed input looks successful while browser access disappears. Preserve omission as the delete signal, but reject present values that aren't lists; add these falsey cases to the contract test.
Review

value or [] turns explicit falsey non-lists (false, 0, '', {}) into []; deployment then runs delete-bucket-cors. A mistyped policy can remove working CORS instead of failing validation. Preserve only None as omission and add rejection tests.

`value or []` turns explicit falsey non-lists (`false`, `0`, `''`, `{}`) into `[]`; deployment then runs `delete-bucket-cors`. A mistyped policy can remove working CORS instead of failing validation. Preserve only `None` as omission and add rejection tests.
if len(origins) != len(set(origins)):
raise ConfigError(f"{label} must not contain duplicates")
for origin in origins:
if origin == "*":
continue
parsed = urlparse(origin)
try:
port = parsed.port
except ValueError:
port = None
if parsed.scheme != "https" or not parsed.hostname or parsed.path or parsed.params or (
Review

This accepts non-canonical authorities such as https://consumer example, uppercase hosts, and explicit default ports, then writes them unchanged. The deploy succeeds but browsers cannot emit a matching Origin. Validate the authority and exact canonical serialization, then detect duplicates after canonicalization.

This accepts non-canonical authorities such as `https://consumer example`, uppercase hosts, and explicit default ports, then writes them unchanged. The deploy succeeds but browsers cannot emit a matching Origin. Validate the authority and exact canonical serialization, then detect duplicates after canonicalization.
parsed.query or parsed.fragment or parsed.username or parsed.password
or (":" in parsed.netloc and port is None and not parsed.netloc.endswith("]"))
):
raise ConfigError(f"{label} must contain '*' or HTTPS origins")
return origins
def _hostname(value, label): def _hostname(value, label):
if not isinstance(value, str) or len(value) > 253 or value.endswith("."): if not isinstance(value, str) or len(value) > 253 or value.endswith("."):
raise ConfigError(f"{label} must be a lowercase DNS hostname without a trailing dot") raise ConfigError(f"{label} must be a lowercase DNS hostname without a trailing dot")
@@ -215,6 +235,7 @@ def _legacy_config(raw, site_name):
"website_authority": "garage-s3.storage.svc.k8s.sjc001.fritzlab.net", "website_authority": "garage-s3.storage.svc.k8s.sjc001.fritzlab.net",
"credentials": {"access_key_env": "AWS_ACCESS_KEY_ID", "secret_key_env": "AWS_SECRET_ACCESS_KEY"}, "credentials": {"access_key_env": "AWS_ACCESS_KEY_ID", "secret_key_env": "AWS_SECRET_ACCESS_KEY"},
"cache_rules": [{"path": "", "cache_control": DEFAULT_CACHE_CONTROL}], "cache_rules": [{"path": "", "cache_control": DEFAULT_CACHE_CONTROL}],
"cors_origins": None,
} }
return { return {
"version": 1, "version": 1,
@@ -234,7 +255,11 @@ def _legacy_config(raw, site_name):
def _artifact(item, index): def _artifact(item, index):
label = f"artifacts[{index}]" label = f"artifacts[{index}]"
item = _mapping(item, label) item = _mapping(item, label)
_known_keys(item, {"name", "type", "content_dir", "tidy", "excludes", "publish", "cache"}, label) _known_keys(
item,
{"name", "type", "content_dir", "tidy", "excludes", "publish", "cache", "cors_origins"},
label,
)
name = item.get("name") name = item.get("name")
if not isinstance(name, str) or not NAME_RE.fullmatch(name): if not isinstance(name, str) or not NAME_RE.fullmatch(name):
raise ConfigError(f"{label}.name must be a DNS label") raise ConfigError(f"{label}.name must be a DNS label")
@@ -303,6 +328,7 @@ def _artifact(item, index):
"website_authority": authority, "website_authority": authority,
"credentials": normalized_credentials, "credentials": normalized_credentials,
"cache_rules": cache_rules, "cache_rules": cache_rules,
"cors_origins": _cors_origins(item.get("cors_origins"), f"{label}.cors_origins"),
} }
@@ -389,6 +415,10 @@ def _validate_multi(cfg):
raise ConfigError(f"protected route {route['name']} cannot use shared-cache max-age") raise ConfigError(f"protected route {route['name']} cannot use shared-cache max-age")
if route["access"] == "public" and "private" in directives: if route["access"] == "public" and "private" in directives:
raise ConfigError(f"public route {route['name']} cannot use private cache policy") raise ConfigError(f"public route {route['name']} cannot use private cache policy")
if route["access"] == "protected" and "*" in artifact["cors_origins"]:
raise ConfigError(
f"protected route {route['name']} cannot allow wildcard CORS"
)
root = next(route for route in routes if route["path"] == "/") root = next(route for route in routes if route["path"] == "/")
if any(route["access"] == "protected" for route in routes) and root["access"] == "public": if any(route["access"] == "protected" for route in routes) and root["access"] == "public":
raise ConfigError("a public '/' catch-all would expose unmatched protected content") raise ConfigError("a public '/' catch-all would expose unmatched protected content")
+1
View File
@@ -15,6 +15,7 @@ artifacts:
- name: distributions - name: distributions
type: static type: static
content_dir: dist content_dir: dist
cors_origins: ['*']
publish: publish:
bucket: baseline-dist bucket: baseline-dist
credentials: credentials:
+77 -1
View File
@@ -113,6 +113,30 @@ class ConfigContractTests(unittest.TestCase):
self.assertEqual(["/dist", "/"], [route["path"] for route in cfg["routes"]]) self.assertEqual(["/dist", "/"], [route["path"] for route in cfg["routes"]])
credentials = {artifact["name"]: artifact["credentials"] for artifact in cfg["artifacts"]} credentials = {artifact["name"]: artifact["credentials"] for artifact in cfg["artifacts"]}
self.assertNotEqual(credentials["distributions"], credentials["portal"]) self.assertNotEqual(credentials["distributions"], credentials["portal"])
distributions = next(
artifact for artifact in cfg["artifacts"] if artifact["name"] == "distributions"
)
self.assertEqual(["*"], distributions["cors_origins"])
def test_cors_origins_are_https_origins_or_wildcard(self):
self.assert_invalid(
lambda raw: raw["artifacts"][1].__setitem__(
"cors_origins", ["http://consumer.example"]
),
"must contain '\\*' or HTTPS origins",
)
self.assert_invalid(
lambda raw: raw["artifacts"][1].__setitem__(
"cors_origins", ["https://consumer.example", "https://consumer.example"]
),
"must not contain duplicates",
)
def test_protected_route_rejects_wildcard_cors(self):
self.assert_invalid(
lambda raw: raw["artifacts"][0].__setitem__("cors_origins", ["*"]),
"protected route portal cannot allow wildcard CORS",
)
def test_equivalent_route_paths_are_ambiguous(self): def test_equivalent_route_paths_are_ambiguous(self):
self.assert_invalid(lambda raw: raw["routes"].append({ self.assert_invalid(lambda raw: raw["routes"].append({
@@ -131,6 +155,7 @@ class ConfigContractTests(unittest.TestCase):
raw["routes"][1]["access"] = { raw["routes"][1]["access"] = {
"mode": "protected", "middleware": "authentik-forwardauth" "mode": "protected", "middleware": "authentik-forwardauth"
} }
raw["artifacts"][1]["cors_origins"] = []
raw["artifacts"][0]["cache"]["rules"][0]["cache_control"] = ( raw["artifacts"][0]["cache"]["rules"][0]["cache_control"] = (
"public, max-age=0, must-revalidate" "public, max-age=0, must-revalidate"
) )
@@ -317,6 +342,30 @@ class BuildTests(unittest.TestCase):
class PublishingTests(unittest.TestCase): class PublishingTests(unittest.TestCase):
def test_cors_policy_is_reconciled_as_read_only_browser_access(self):
captured = {}
def capture(command, **_kwargs):
config_path = command[command.index("--cors-configuration") + 1]
captured["command"] = command
captured["config"] = json.loads(Path(config_path.removeprefix("file://")).read_text())
with patch.object(deploy, "run", side_effect=capture):
deploy.configure_cors(
"baseline-dist", ["*"], "http://garage-s3.storage.svc:3900", {}
)
self.assertIn("put-bucket-cors", captured["command"])
self.assertEqual(["GET", "HEAD"], captured["config"]["CORSRules"][0]["AllowedMethods"])
self.assertEqual(["*"], captured["config"]["CORSRules"][0]["AllowedOrigins"])
def test_empty_cors_policy_removes_stale_bucket_cors(self):
with patch.object(deploy, "run") as request:
deploy.configure_cors(
"baseline-catalogue", [], "http://garage-s3.storage.svc:3900", {}
)
self.assertIn("delete-bucket-cors", request.call_args.args[0])
def test_apps_clone_never_places_token_in_argv_or_log(self): def test_apps_clone_never_places_token_in_argv_or_log(self):
calls = [] calls = []
secret = "clone-secret-must-not-appear" secret = "clone-secret-must-not-appear"
@@ -437,15 +486,42 @@ class PublishingTests(unittest.TestCase):
patch.object(deploy, "validate_artifact_output"), patch.object( patch.object(deploy, "validate_artifact_output"), patch.object(
deploy, "publish_route_immutables", deploy, "publish_route_immutables",
side_effect=[None, RuntimeError("immutable failed")], side_effect=[None, RuntimeError("immutable failed")],
) as immutable_publish, patch.object(deploy, "s3_sync") as mutable_sync, \ ) as immutable_publish, patch.object(
deploy, "configure_artifact_cors"
) as cors_configure, patch.object(deploy, "s3_sync") as mutable_sync, \
self.assertRaisesRegex( self.assertRaisesRegex(
RuntimeError, "immutable failed" RuntimeError, "immutable failed"
): ):
deploy.deploy_static("baseline", root, root, "token", cfg) deploy.deploy_static("baseline", root, root, "token", cfg)
self.assertEqual(2, immutable_publish.call_count) self.assertEqual(2, immutable_publish.call_count)
cors_configure.assert_not_called()
mutable_sync.assert_not_called() mutable_sync.assert_not_called()
def test_all_cors_policies_complete_before_mutable_publication(self):
cfg = normalize_site_config(fixture("split-site.yaml"), "baseline.fritzlab.net")
events = []
with tempfile.TemporaryDirectory() as tmp:
root = Path(tmp)
apps = root / "apps"
apps.mkdir()
with patch.object(deploy, "validate_publication_environment"), patch.object(
deploy, "validate_artifact_output"
), patch.object(
deploy, "publish_route_immutables", side_effect=lambda *_args: events.append("immutable")
), patch.object(
deploy, "configure_artifact_cors", side_effect=lambda *_args: events.append("cors")
), patch.object(
deploy, "s3_sync", side_effect=lambda *_args: events.append("mutable")
), patch.object(deploy, "clone_apps", return_value=apps), patch.object(
deploy, "render_site_manifests"
), patch.object(deploy, "commit_and_push"):
deploy.deploy_static("baseline", root, root, "token", cfg)
self.assertEqual(
["immutable", "immutable", "cors", "cors", "mutable", "mutable"], events
)
def test_absent_artifact_is_detected_before_publish(self): def test_absent_artifact_is_detected_before_publish(self):
cfg = normalize_site_config(fixture("split-site.yaml"), "baseline.fritzlab.net") cfg = normalize_site_config(fixture("split-site.yaml"), "baseline.fritzlab.net")