Repository navigation
feat(cloudflare-tunnel): manage the Access app and allow policy per route - #34
Merged
Merged
Conversation
pre-commit run --all-files fails on these files, which blocks the gate for every component.
…oute Add an optional access object to each routes entry, plus access_team_name. A protected hostname gets one self_hosted Access application and one allow policy built from the consumer's emails and email_domains. Its ingress rule gets origin_request.access with required = true so cloudflared refuses a request without a valid Access JWT. DNS records wait for the applications. Routes without access plan exactly as before. New outputs: access_application_ids and access_aud_tags, keyed by hostname.
📝 Changelog previewGenerated from this branch's Conventional Commits — this is what the next [Unreleased]Added
No-mistakes
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #32
4D item
furqbg: #32 (board: https://github.com/orgs/officialdad/projects/3)Summary
A consumer marks a
cloudflare-tunnelroute as Access-protected with anaccessobject. One apply creates the Access application, its allow policy and thecloudflaredJWT check, and a renamed hostname carries all three with it. Routes withoutaccessplan as before.Component(s)
cloudflare-tunnel (plus the root README row)
Type
Changes
routes[].access(optional, default null):emails,email_domains,policy_ids,allowed_idps. Newaccess_team_nameinput, default null.self_hostedcloudflare_zero_trust_access_applicationper distinct protected hostname, and oneallowcloudflare_zero_trust_access_policywhen the route lists emails or email domains.origin_request.accesson each protected ingress rule:required = true,team_name,aud_tag = [app aud].cloudflare_dns_recordhasdepends_onon the Access applications.access_application_idsandaccess_aud_tags, keyed by hostname.tests/access.tftest.hclonmock_provider "cloudflare".style(repo)commit: trailing whitespace stripped in vendored.agents/skillsfiles, becausepre-commit run --all-filesfailed on them.Design choice
accessobject on each route (chosen)access_applicationsmap keyed by hostnameDone-when entry met by reading the code, not by a test
terraform testcannot assert resource dependencies. The Done-when entry "cloudflare_dns_recordfor anaccessroute depends on the application" is met by the explicitdepends_onatcloudflare-tunnel/terraform/main.tf(incloudflare_dns_record.this), which a reviewer can read.terraform graphalso shows the edge. The owner accepted this on the review gate.Known gaps
access_team_name = ""passes the null-only precondition and writes an emptyteam_name. A variable validation would close it. It is left for a follow-up.Owner checklist (after merge)
scripts/release.sh X.Y.Zand pushmainplus thevX.Y.Ztag. The number is whatever is next when this merges.tokens/verifycurl.allowed_idps.infra-environments-prodrepository, as input to the consumer Terragrunt.cloudflare_tunnelpin, apply, then soak. Check that the dev hostname returns a 302 to the Zero Trust login and that a request with no JWT is refused.apply:cloudflare_tunnellabel.versions.hcl, so the hostname is never unprotected.Validation
terraform fmt/validate/tflintpass on the changed components (the gate)type(scope): subjectwith scope = componentterraform -chdir=cloudflare-tunnel/terraform test: 8 passed, 0 failedpre-commit run --all-filesexits 0What Changed
cloudflare-tunnel: adds an optionalaccessobject to each route (emails,email_domains,policy_ids,allowed_idps) and a newaccess_team_nameinput. For each protected hostname, the module creates acloudflare_zero_trust_access_application. It also creates an allowcloudflare_zero_trust_access_policywhen the route lists emails or email domains. The module sets the tunnel ingressorigin_request.accessso cloudflared checks the Access JWT, and DNS records wait until the Access apps exist. Thepolicieslist is built from known inputs only, so its length is known at plan time.allowed_idpslist are rejected. A precondition requiresaccess_team_namewhen any route is protected. Unprotected routes plan the same as before.access_application_idsandaccess_aud_tagsoutputs, atests/access.tftest.hclsuite, and updates to the component and root READMEs. Trailing whitespace is removed from the vendored.agents/skillsdocs.Risk Assessment
Testing
I built a disposable harness in /tmp with copies of the module from the branch and from main, then removed it. First I ran the mock-provider
terraform testfile, and all 8 runs pass. Then I ran realterraform planwith the real Cloudflare provider 5.26.0, a fake token, and an API URL that goes nowhere. Four protected-route shapes now plan with exit 0. That includes the emails-only and email_domains-plus-policy_ids routes that failed in round 1. All six guard cases fail with the intended error message. The plan JSON shows these values: allowed_idps passes through as given, auto_redirect_to_identity is true only with one identity provider, and an emails-only policy holds one email rule and no email_domain rule. Each protected ingress rule has required=true, team_name, and an aud_tag taken from the app. The DNS record depends on the Access application. The no-access plan is identical on main and the branch, and both access outputs are empty. A two-step mock apply shows a hostname rename moves the app and the policy to the new hostname. That rename check is not live, because a real apply needs Cloudflare credentials. The placeholder grep prints only example.com. The worktree is clean.Evidence: Real-provider plans for every scenario (8e928a2)
Evidence: Planned Access app, policy, ingress and DNS depends_on from plan JSON
Evidence: No-access plan main vs branch (IDENTICAL)
Evidence: terraform test with mock_provider
Evidence: Rename moves Access (two mock applies)
Evidence: Placeholder grep
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
cloudflare-tunnel/terraform/tests/access.tftest.hcl:48- A required Done-when criterion has no test. The criterion reads: "The sameterraform testrun:cloudflare_dns_recordfor anaccessroute depends on the application." No run block in access.tftest.hcl asserts anything aboutcloudflare_dns_record. The only evidence isdepends_on = [cloudflare_zero_trust_access_application.this]at main.tf:116.terraform testhas no assertion that can read resource dependencies, so this criterion cannot be met as literally written. The owner must decide how to close it: accept main.tf:116 plus a note in the PR body, accept a proxy assertion, or amend the criterion. Needs an owner decision because the change cannot fix this on its own.cloudflare-tunnel/terraform/main.tf:96- The precondition only rejects a nullaccess_team_name. An empty string passes it. Example:access_team_name = ""with one protected route plans cleanly and writesteam_name = ""intoorigin_request.access. The README says cloudflared then rejects every request, so the hostname goes dark. Fix: add avalidationonvariable "access_team_name"(variables.tf:76) that requires null ortrimspace(var.access_team_name) != "".cloudflare-tunnel/terraform/main.tf:80- This line reads the module's own policy id with[for p in [try(cloudflare_zero_trust_access_policy.this[each.key], null)] : p.id if p != null]. The pattern is hard to read, andtry()hides any error in that reference.local.access_policy_by_hostnamealready says whether this hostname has a policy. Clearer form:contains(keys(local.access_policy_by_hostname), each.key) ? [cloudflare_zero_trust_access_policy.this[each.key].id] : []. The output does not change.🔧 **Test** - 2 issues found → auto-fixed ✅
cloudflare-tunnel/terraform/main.tf:80- A realterraform plan(cloudflare/cloudflare 5.26.0, the version~> 5.0resolves to) fails for every route whose access block hasemailsoremail_domains. The error isError: Value Conversion Error ... Received unknown value, however the target type cannot handle unknown values. Path: policies. The try()/for/if pattern on main.tf:80 makes the wholepolicieslist unknown until the new policy exists. Consumers hit this on their first plan: aqaros-admin with a staff email list, and any emails-only or email_domains+policy_ids route.terraform testpasses only becausemock_providerskips the provider's unknown-value check. Fix, checked live with the same inputs (plans 5 to add, exit 0):contains(keys(local.access_policy_by_hostname), each.key) ? [cloudflare_zero_trust_access_policy.this[each.key].id] : [],. This is the rewrite from the round-1 findingpolicies-try-simplify. The owner declined that finding as a readability change, and this run left the decision alone. The new evidence shows it is a correctness bug, so the owner must re-decide. A mock-only test cannot catch this regression. CI needs a real-provider plan, or the owner accepts that gap.Error: Value Conversion Error ... Path: policies, exit 1, main.tf:78-83Invalid value for variablewith the allowed_idps messageResource precondition failed ... access_team_name is requiredterraform -chdir=<copy of cloudflare-tunnel/terraform> test(mock_provider, 8 run blocks)terraform planon main (31b9149) and branch (b9b407d) with the same no-access routes, real provider, API base URL set to 127.0.0.1:9, thenterraform show -json | jqdiff of resource_changesterraform planon branch with one protected route (emails + allowed_idps = ["example-idp-id"]) using the real providerterraform planon a scratch copy with main.tf:80 rewritten to the contains() form, same inputsterraform planon branch: policy_ids-only, emails-only, email_domains+policy_idsterraform planon branch rejection cases: allowed_idps = [], allowed_idps = [" "], access = {}, wildcard email, half-protected hostname, missing access_team_nameterraform graphplus the plan JSONconfigurationdepends_on for cloudflare_dns_record.thisgit grep -nE "@|[0-9a-f]{32}" -- cloudflare-tunnel/Triedterraform applyof main against a local fake Cloudflare API to prove a zero-change tag bump. The repo guard tf-guard.sh denied it, and this run did not work around the guard.🔧 Fix applied.
✅ Re-checked - no issues remain.
terraform test(mock_provider "cloudflare", tests/access.tftest.hcl) in a disposable copy of cloudflare-tunnel/terraformterraform plan -refresh=falsewith real cloudflare/cloudflare 5.26.0, a fake token andCLOUDFLARE_BASE_URL=http://127.0.0.1:9for tfvars: emails_one_idp, domains_plus_policy, policy_ids_only, two_idps_no_redirectterraform planrejection cases: reject_empty_idps, reject_blank_idp, reject_empty_access, reject_everyone_wildcard, reject_half_protected, reject_no_team_nameterraform show -json <plan> | jqto read the planned app (allowed_idps, auto_redirect_to_identity, policies), the policy include rules, the ingress origin_request.access values, and the DNS depends_onSame no-access tfvars planned on main 31b9149 and branch 8e928a2, thendiffof the planned resource_changesThrowaway tests/rename.tftest.hcl (harness only, not committed): mock apply with qr-old.example.com, then with qr-new.example.comgit grep -nE "@|[0-9a-f]{32}" -- cloudflare-tunnel/✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.