fix(identity): accept cloudfunctions.net auth blocking token audiences - #313
Conversation
The verifier only accepted an audience containing run.app, but firebase-tools registers the cloudfunctions.net URL when it creates a blocking function and the run.app URL when it updates one. A newly created blocking function therefore rejected every token with a 500 until the next deploy moved it to the update path. Accept either form, matching the Node SDK. The existing sys.modules mock in the identity tests is replaced with an attribute patch, since it only held while nothing had imported the real token_verifier module.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the token verification logic to support multiple expected audiences, specifically accommodating both 'run.app' and 'cloudfunctions.net' URL formats. The review highlights a potential security vulnerability in the current audience validation implementation, which uses a substring check that could be susceptible to audience confusion attacks. It is recommended to implement strict URL parsing and hostname validation. Additionally, the reviewer suggests expanding the test suite to include cases that specifically target potential bypasses of the audience check.
cabljac
left a comment
cabljac
left a comment
There was a problem hiding this comment.
@IzaakGough approving, ready for the Firebase team to review. A few small things inline, the main one being the substring audience match.
| # audience is either form depending on how the function was last deployed. | ||
| expected_audiences=[ | ||
| "run.app", | ||
| f"{app.project_id}.cloudfunctions.net/", |
There was a problem hiding this comment.
I think the substring match might accept a bit more than we intend. I added a probe test on this branch and these both pass verification:
https://us-east1-other-test-project.cloudfunctions.net/fnfor projecttest-projecthttps://evil.example.com/?x=run.app
Real risk seems low, since iss pins the project and Google signs the token, and Node has the same looseness. Tightening looks cheap though, so could we match on the parsed host instead? Something like:
host = urlsplit(audience).hostname or ""
host.endswith(".run.app") or host.endswith(f"-{self.project_id}.cloudfunctions.net")Whenever we accept a value by pattern, it's usually worth writing the test for the closest lookalike first.
There was a problem hiding this comment.
Done in 16fd338, matching on the parsed host.
One thing worth noting on the suffix form: host.endswith(f"-{project_id}.cloudfunctions.net") still accepts the probe you found. us-east1-other-test-project.cloudfunctions.net ends with -test-project.cloudfunctions.net, and so do the x1- and a- prefixed variants. Suffix matching cannot separate them, since region and project id both contain hyphens, so the region is pinned to its naming shape instead:
rf"[a-z]+(?:-[a-z]+)*\d+-{re.escape(project_id)}\.cloudfunctions\.net"All 47 regions in gstatic.com/ipranges/cloud.json satisfy that, including ones newer than the SupportedRegion enum. It is an assumption about Google naming though, and a region shaped differently would be rejected, so I called that out in the PR body as the line to revisit.
urlsplit(...).hostname handles your ?x=run.app probe, as you had it. I also kept the run.app side un-scoped to the project, as in Node, so iss is still what ties a token to this project.
Framed it in the body as tidying rather than a security fix, since reaching this check needs a Google-signed token for this project either way.
There was a problem hiding this comment.
I don't disagree that these tighter checks on cloudfunctions.net urls will work but ... is this over-engineering? It seems like we're adding extra checks only to one code path (cloudfunctions.net) but not another (run.app) and that just checking for either host suffix is probably sufficient?
There was a problem hiding this comment.
Following up on this after looking closer at what the aud check is actually protecting against here:
-
sig+issalready bind the token to Google and to this project:
_JWTVerifier.verify()verifies the RS256 signature againstID_TOKEN_CERT_URI(securetoken@system.gserviceaccount.com) and requires exact equality oniss == f"https://securetoken.google.com/{self.project_id}". Becauseissis strictly checked:- A blocking token from a suffix-colliding project (
other-my-project) hasiss = "https://securetoken.google.com/other-my-project"and is rejected by theisscheck regardless of whataudcontains. audis signed by Google rather than caller-controlled, and Identity Platform only registers valid Cloud Functions / Cloud Run trigger URLs when configuring blocking functions.
- A blocking token from a suffix-colliding project (
-
What
audactually needs to distinguish:
The only other tokens signed bysecuretoken@system.gserviceaccount.comwithiss == f"https://securetoken.google.com/{project_id}"are regular Firebase Auth ID tokens, whereaudis the bareproject_idstring (never a URL). The real security role of theaudcheck inAuthBlockingTokenVerifieris preventing token-type confusion so a regular Firebase ID token for the same project cannot be accepted as an Auth Blocking token. -
Why simple host suffix matching is better here:
Since a Firebaseproject_idcannot contain dots or slashes, checking thataudis anhttps://URL whose hostname ends with.run.appor.cloudfunctions.netcompletely separates blocking tokens from regular ID tokens. Pinningcloudfunctions.netto a region regex ([a-z]+(?:-[a-z]+)*\d+) doesn't add security over the.run.appbranch, and risks breaking if GCP ever introduces a region name that doesn't match that pattern.
Could we simplify _blocking_audience_matcher to just check the host suffix?
_BLOCKING_HOST_SUFFIXES = (".run.app", ".cloudfunctions.net")
def _blocking_audience_matcher(audience: object) -> bool:
if not isinstance(audience, str):
return False
parts = urlsplit(audience)
host = parts.hostname or ""
return parts.scheme == "https" and host.endswith(_BLOCKING_HOST_SUFFIXES)(Or if we still want project_id in the cloudfunctions.net suffix for parity with Node's ${projectId}.cloudfunctions.net/, host.endswith((".run.app", f"-{project_id}.cloudfunctions.net")) avoids the region regex while keeping the host parser.)
There was a problem hiding this comment.
Thanks for digging into this. Agreed, iss already binds the token to the project, so aud only needs to tell blocking tokens apart from regular ID tokens. I went with your first snippet in 61614c9: an https URL whose host ends with .run.app or .cloudfunctions.net, with the region regex dropped.
I left out the -{project_id}.cloudfunctions.net variant because it still accepts other-my-project for my-project, so it wouldn't add anything over iss, and it would scope one host but not the other.
On the tests, a regular ID token (aud = project id) is now an aud rejection case. The prefixed-project cases now use their own project's iss, and the test checks they're rejected on iss (which means they got past the aud check). I've updated the PR body to match.
The bare pytest.raises passed for any InvalidAuthBlockingTokenError, including an audience rejection, so it did not pin what its name says.
Reject a non-string "aud" with InvalidAuthBlockingTokenError rather than letting the membership test raise TypeError, fix the class comment to name the renamed kwarg, drop a stale test comment, and declare cryptography in the dev group since the tests import it directly.
The audience check substring-matched the token's `aud`, so it accepted a URL that merely contained the expected text: a project id ending with this one (`other-my-project` against `my-project`), or the text placed in a path, query or fragment on any host. Match the parsed host instead, with the gen-1 region pinned to its naming shape. This is deliberately stricter than firebase-admin-node, which substring-matches. The run.app form stays un-scoped to the project, as in Node, so `iss` is still what ties a token to this project. `expected_audiences` becomes `audience_matcher`, since the list was only a truthiness flag plus error text once matching moved to the host.
| ) | ||
|
|
||
| def matches(audience): | ||
| if not isinstance(audience, str): |
There was a problem hiding this comment.
I realize it worked this way in the old code effectively too, but do you know why we're only handling the case where audience is a string. RFC 7519 §4.1.3 allows the "aud" claim to be either a single string or an array of strings.
There was a problem hiding this comment.
I did some digging into the security bits more, but that also came back with an answer that today we always only write a string so this is fine (at least until that changes)
| # audience is either form depending on how the function was last deployed. | ||
| expected_audiences=[ | ||
| "run.app", | ||
| f"{app.project_id}.cloudfunctions.net/", |
There was a problem hiding this comment.
I don't disagree that these tighter checks on cloudfunctions.net urls will work but ... is this over-engineering? It seems like we're adding extra checks only to one code path (cloudfunctions.net) but not another (run.app) and that just checking for either host suffix is probably sufficient?
iss already binds the token to the project, so aud only needs to tell blocking tokens apart from regular ID tokens. Drops the region regex.
3 participants
Fixes #311.
run.app. firebase-tools registers the cloudfunctions.net URL when it creates a blocking function and the run.app URL when it updates one, so a newly created function rejected every token with a 500 until a later deploy took the update path. It now accepts either, matching the Node SDK fix in Allow v2 auth blocking functions to use run.app or cf.net URLs firebase-functions#1831.audmust be anhttpsURL whose host ends with.run.appor.cloudfunctions.net, so the expected text in a path, query or fragment no longer matches. Neither host is project-scoped:issalready binds the token to this project, andaudonly has to tell blocking tokens apart from regular ID tokens, whoseaudis the bare project id.Tests sign real tokens and serve the signing key as the certs, so acceptance is checked end to end. Verified against a real project: a first deploy registered a cloudfunctions.net URI and Google sign-in failed with this exact error, which the new test pins. The host matching is covered by the suite only, not by a live token.