diff --git a/.github/workflows/deploy-all.yml b/.github/workflows/deploy-all.yml index c89d51d69..103e37a6a 100644 --- a/.github/workflows/deploy-all.yml +++ b/.github/workflows/deploy-all.yml @@ -315,25 +315,23 @@ jobs: run: | FAILED=false - # determine-deployment is checked for anything other than success, not - # just "failure". When it stops the run the four deploy jobs are - # SKIPPED rather than failed, so testing only those would report "all - # deployments completed successfully" for a run that deployed nothing. - # - # The four deploy jobs are deliberately still tested for "failure" - # only. A CANCELLED run leaves them `cancelled` and reaches the - # success line, which is the same false report. That predates this - # change, so it is tracked in #1810 rather than fixed here. - # - # Note this step is the only success-by-default check in the repo. All - # four per-cloud `summary` jobs allowlist on `== "success"` instead, - # so an unenumerated result falls to their failure branch and errs - # safe. This denylist is the outlier, not the pattern. - if [[ "${{ needs.determine-deployment.result }}" != "success" ]] || \ - [[ "${{ needs.deploy-aws-lambda.result }}" == "failure" ]] || \ - [[ "${{ needs.deploy-aws-fargate.result }}" == "failure" ]] || \ - [[ "${{ needs.deploy-gcp.result }}" == "failure" ]] || \ - [[ "${{ needs.deploy-azure.result }}" == "failure" ]]; then + # Allowlist, not denylist: determine-deployment must be "success". A + # selected deploy job must be "success"; "skipped" is only ok for a + # job that was not selected. Anything else, including "cancelled", + # counts as a failed run. + check_job() { + local selected="$1" result="$2" + if [[ "$selected" == "true" ]]; then + [[ "$result" == "success" ]] + else + [[ "$result" == "skipped" || "$result" == "success" ]] + fi + } + check_job "${{ needs.determine-deployment.outputs.deploy-aws-lambda }}" "${{ needs.deploy-aws-lambda.result }}" || FAILED=true + check_job "${{ needs.determine-deployment.outputs.deploy-aws-fargate }}" "${{ needs.deploy-aws-fargate.result }}" || FAILED=true + check_job "${{ needs.determine-deployment.outputs.deploy-gcp }}" "${{ needs.deploy-gcp.result }}" || FAILED=true + check_job "${{ needs.determine-deployment.outputs.deploy-azure }}" "${{ needs.deploy-azure.result }}" || FAILED=true + if [[ "${{ needs.determine-deployment.result }}" != "success" ]]; then FAILED=true fi diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index f009e1b5f..8f008e464 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3500,16 +3500,15 @@ components: nullable: true description: Set while a collection is in flight. last_collection_partial: - nullable: true - description: > - Non-null when the most recent persisted collection kept rows from - at least one incomplete sweep. Cleared by the next collection whose - sweeps are all complete. Carries counts and reason categories only. - allOf: - - $ref: '#/components/schemas/RecommendationsCollectionPartial' + $ref: '#/components/schemas/RecommendationsCollectionPartial' RecommendationsCollectionPartial: type: object + nullable: true + description: > + Non-null when the most recent persisted collection kept rows from + at least one incomplete sweep. Cleared by the next collection whose + sweeps are all complete. Carries counts and reason categories only. required: [providers] properties: providers: @@ -3576,7 +3575,8 @@ components: max_step_commitment: type: number nullable: true - exclusiveMinimum: 0 + minimum: 0 + exclusiveMinimum: true description: >- Ceiling on the total commitment (upfront + monthly * term) one ramp step may auto-purchase. Omit on create for no cap (auto-purchase of diff --git a/internal/api/openapi_dialect_test.go b/internal/api/openapi_dialect_test.go new file mode 100644 index 000000000..703ba41cb --- /dev/null +++ b/internal/api/openapi_dialect_test.go @@ -0,0 +1,49 @@ +package api + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +// Issue #1075: the spec declares OpenAPI 3.0.3, where nullable needs an +// explicit type, $ref siblings are ignored and exclusiveMinimum is a boolean +// paired with minimum. +func TestOpenAPIDialect303Schemas(t *testing.T) { + type schema struct { + Ref string `yaml:"$ref"` + Type string `yaml:"type"` + Nullable bool `yaml:"nullable"` + Required []string `yaml:"required"` + Properties map[string]schema `yaml:"properties"` + Minimum *int `yaml:"minimum"` + ExclusiveMinimum any `yaml:"exclusiveMinimum"` + } + var spec struct { + Components struct { + Schemas map[string]schema `yaml:"schemas"` + } `yaml:"components"` + } + require.NoError(t, yaml.Unmarshal(openapiSpec, &spec)) + schemas := spec.Components.Schemas + + prop := schemas["RecommendationsFreshness"].Properties["last_collection_partial"] + require.NotEmpty(t, prop.Ref, "3.0.3 ignores $ref siblings, so the property must be a bare $ref") + partial := schemas[strings.TrimPrefix(prop.Ref, "#/components/schemas/")] + + // null is accepted only if the resolved schema is nullable with a type. + assert.True(t, partial.Nullable, "null last_collection_partial must validate") + assert.Equal(t, "object", partial.Type, "nullable requires an explicit type in 3.0.3") + // {"providers": []} satisfies required + type. + assert.Equal(t, []string{"providers"}, partial.Required) + assert.Equal(t, "array", partial.Properties["providers"].Type) + + stepCap := schemas["PlanRequest"].Properties["max_step_commitment"] + require.NotNil(t, stepCap.Minimum) + // minimum 0 with exclusiveMinimum true: positive accepted, 0 rejected. + assert.Equal(t, 0, *stepCap.Minimum) + assert.Equal(t, true, stepCap.ExclusiveMinimum, "zero must be rejected; exclusiveMinimum must be boolean in 3.0.3") +} diff --git a/internal/server/scheduledauth/validator.go b/internal/server/scheduledauth/validator.go index ba5a87dbe..f193cc670 100644 --- a/internal/server/scheduledauth/validator.go +++ b/internal/server/scheduledauth/validator.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "log" + "net" "net/http" "net/url" "strings" @@ -142,7 +143,7 @@ func configureOIDC(v *Validator, cfg Config) (*Validator, error) { if cfg.JWKSURL == "" { cfg.JWKSURL = GoogleJWKSURL } - if err := validateAbsoluteURL(cfg.JWKSURL, "SCHEDULED_TASK_OIDC_JWKS_URL"); err != nil { + if err := validateHTTPSURL(cfg.JWKSURL, "SCHEDULED_TASK_OIDC_JWKS_URL"); err != nil { return nil, err } if len(cfg.Audiences) == 0 { @@ -214,7 +215,9 @@ func cleanSet(in []string, label string) (map[string]struct{}, error) { return out, nil } -func validateAbsoluteURL(raw, label string) error { +// validateHTTPSURL requires an absolute https URL. Plain http is allowed only +// for loopback hosts, which a network attacker cannot sit in front of. +func validateHTTPSURL(raw, label string) error { u, err := url.Parse(raw) if err != nil { return fmt.Errorf("%w: %s must be an absolute URL: %w", ErrConfigInvalid, label, err) @@ -222,7 +225,21 @@ func validateAbsoluteURL(raw, label string) error { if u.Scheme == "" || u.Host == "" { return fmt.Errorf("%w: %s must be an absolute URL", ErrConfigInvalid, label) } - return nil + if u.Scheme == "https" { + return nil + } + if u.Scheme == "http" && isLoopbackHost(u.Hostname()) { + return nil + } + return fmt.Errorf("%w: %s must use https", ErrConfigInvalid, label) +} + +func isLoopbackHost(host string) bool { + if strings.EqualFold(host, "localhost") { + return true + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() } // Mode returns the validator's auth mode. Useful for /health. diff --git a/internal/server/scheduledauth/validator_test.go b/internal/server/scheduledauth/validator_test.go index da3cc623d..156990b6e 100644 --- a/internal/server/scheduledauth/validator_test.go +++ b/internal/server/scheduledauth/validator_test.go @@ -884,6 +884,49 @@ func TestNew_RejectsOIDCMalformedJWKSURL(t *testing.T) { } } +func TestNew_OIDCJWKSURLScheme(t *testing.T) { + cases := []struct { + name string + jwksURL string + wantErr bool + }{ + {"http remote host rejected", "http://jwks.example.com/keys", true}, + {"ftp rejected", "ftp://jwks.example.com/keys", true}, + {"https accepted", "https://jwks.example.com/keys", false}, + {"http loopback accepted for local test doubles", "http://127.0.0.1:8080/keys", false}, + {"localhost subdomain rejected", "http://localhost.evil.com/keys", true}, + {"loopback userinfo trick rejected", "http://127.0.0.1@evil.com/keys", true}, + {"localhost trailing dot rejected", "http://localhost./x", true}, + {"loopback lookalike domain rejected", "http://127.0.0.1.nip.io/keys", true}, + {"http ipv6 loopback accepted", "http://[::1]:8080", false}, + {"http LOCALHOST accepted", "http://LOCALHOST", false}, + {"http 127.0.0.2 accepted", "http://127.0.0.2", false}, + {"http unspecified ip rejected", "http://0.0.0.0/k", true}, + {"http private ip rejected", "http://10.0.0.1/k", true}, + {"http link-local metadata ip rejected", "http://169.254.169.254/k", true}, + {"empty falls back to Google JWKS URL", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := New(Config{ + Mode: ModeOIDC, + JWKSURL: tc.jwksURL, + Audiences: []string{"https://api.example.com"}, + Subjects: []string{testSchedulerSubject}, + }) + if tc.wantErr { + if !errors.Is(err, ErrConfigInvalid) { + t.Fatalf("expected ErrConfigInvalid, got: %v", err) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} + func TestNew_Rejects_BearerWithoutSecret(t *testing.T) { _, err := New(Config{Mode: ModeBearer}) if !errors.Is(err, ErrConfigInvalid) { diff --git a/known_issues/38_openapi_contract.md b/known_issues/resolved/38_openapi_contract.md similarity index 66% rename from known_issues/38_openapi_contract.md rename to known_issues/resolved/38_openapi_contract.md index 076cb82cd..b77e2e4ce 100644 --- a/known_issues/38_openapi_contract.md +++ b/known_issues/resolved/38_openapi_contract.md @@ -1,6 +1,6 @@ # Known Issues: OpenAPI dialect validation -> **Audit status (2026-10-18):** `1 needs triage · 0 resolved` +> **Audit status (2026-10-18):** `0 needs triage · 1 resolved` ## LOW: two existing schemas fail OpenAPI 3.0.3 structural validation @@ -16,4 +16,6 @@ from the insurance contract documentation. **Why deferred:** dialect corrections affect unrelated contracts and are tracked separately to keep the insurance documentation change focused. -**Status:** Open. Tracked in #1075. +**Issue:** [#1075](https://github.com/LeanerCloud/cloud-commitments-platform/issues/1075) + +**Status:** Fixed in this change: `last_collection_partial` has an explicit `type: object`, and `max_step_commitment` uses `minimum: 0` with boolean `exclusiveMinimum`. `TestOpenAPIDialect303Schemas` pins both. diff --git a/terraform/environments/azure/secrets.tf b/terraform/environments/azure/secrets.tf index f26213a85..055dfeba6 100644 --- a/terraform/environments/azure/secrets.tf +++ b/terraform/environments/azure/secrets.tf @@ -30,7 +30,9 @@ module "secrets" { soft_delete_retention_days = var.soft_delete_retention_days purge_protection_enabled = var.purge_protection_enabled # Default-deny network ACL: only ip_addresses in var.allowed_ip_addresses and - # the Container App subnet reach the Key Vault data plane. Dev environments + # the private subnet reach the Key Vault data plane. The private subnet + # exists only when create_private_subnet = true (default false), so by + # default no subnet is allowlisted. Dev environments # that need cross-network access from an operator laptop should add the IP # explicitly; they should not widen this to "Allow". default_network_acl_action = var.key_vault_default_network_acl_action diff --git a/terraform/modules/compute/aws/lambda/variables.tf b/terraform/modules/compute/aws/lambda/variables.tf index 8a8cf1ed8..9f57ae23a 100644 --- a/terraform/modules/compute/aws/lambda/variables.tf +++ b/terraform/modules/compute/aws/lambda/variables.tf @@ -123,8 +123,10 @@ variable "function_url_auth_type" { unreachable without a valid AWS identity. Use "NONE" only in environments where direct access without CloudFront is acceptable (e.g. local dev, or environments not yet provisioned with CloudFront). - Pair with cloudfront_distribution_arn when switching to AWS_IAM so the module can - create the necessary resource-based policy granting CloudFront invocation rights. + This module only creates the public lambda:InvokeFunctionUrl permission for "NONE". + With "AWS_IAM" the caller must grant invocation itself, e.g. an + aws_lambda_permission for principal cloudfront.amazonaws.com scoped to the + distribution ARN (see environments/aws/compute.tf). EOT type = string default = "AWS_IAM" diff --git a/terraform/modules/compute/azure/aks/main.tf b/terraform/modules/compute/azure/aks/main.tf index 475cf7fb4..07b051e67 100644 --- a/terraform/modules/compute/azure/aks/main.tf +++ b/terraform/modules/compute/azure/aks/main.tf @@ -398,7 +398,7 @@ resource "kubernetes_deployment" "app" { value_from { secret_key_ref { name = kubernetes_secret.database[0].metadata[0].name - key = "password" + key = "password_secret_name" } } } diff --git a/terraform/modules/compute/azure/aks_hardening_test.go b/terraform/modules/compute/azure/aks_hardening_test.go index dd914efdf..6621b4cb2 100644 --- a/terraform/modules/compute/azure/aks_hardening_test.go +++ b/terraform/modules/compute/azure/aks_hardening_test.go @@ -54,3 +54,33 @@ func TestAKSClusterIsPrivateWithEntraRBAC(t *testing.T) { t.Error(`var.authorized_ip_ranges must validate against "0.0.0.0/0", so disabling the private cluster can't silently reopen the API server to the whole internet`) } } + +// Issue #1115: DB_PASSWORD_SECRET reads a secret_key_ref key from the +// database Kubernetes Secret; a key renamed in the Secret's data map but not +// in the env reference makes the pod fail with CreateContainerConfigError. +func TestAKSDatabaseSecretKeyRefsExistInSecretData(t *testing.T) { + data, err := os.ReadFile("aks/main.tf") + if err != nil { + t.Fatalf("reading aks/main.tf: %v", err) + } + content := string(data) + + secret := regexp.MustCompile(`(?s)resource\s+"kubernetes_secret"\s+"database"\s*\{.*?\n\s*data\s*=\s*\{(.*?)\n\s*\}`).FindStringSubmatch(content) + if secret == nil { + t.Fatal(`could not isolate the kubernetes_secret.database data map`) + } + keys := map[string]bool{} + for _, m := range regexp.MustCompile(`(?m)^\s*([A-Za-z_][A-Za-z0-9_]*)\s*=`).FindAllStringSubmatch(secret[1], -1) { + keys[m[1]] = true + } + + refs := regexp.MustCompile(`secret_key_ref\s*\{\s*name\s*=\s*kubernetes_secret\.database\[0\]\.metadata\[0\]\.name\s*key\s*=\s*"([^"]+)"`).FindAllStringSubmatch(content, -1) + if len(refs) == 0 { + t.Fatal(`found no secret_key_ref pointing at kubernetes_secret.database`) + } + for _, r := range refs { + if !keys[r[1]] { + t.Errorf("secret_key_ref key %q is not defined in kubernetes_secret.database data (keys: %v)", r[1], keys) + } + } +}