jerryshao commented on code in PR #14601:
URL: https://github.com/apache/gravitino/pull/14601#discussion_r4475673651
##########
docs/gravitino-mcp-server.md:
##########
@@ -364,7 +364,7 @@ Every tool invocation is recorded as one structured JSON
line in `gravitino-mcp-
| Field | Description
|
|--------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `timestamp` | UTC ISO-8601 time of the call.
|
-| `principal` | Caller identity: username for `Basic` simple auth,
`bearer:<first-8-chars>` for a Bearer token,
`oauth:<first-8-chars-of-client-id>` when OAuth client-credentials is the
service identity (stdio or HTTP with no caller header), or `anonymous` when no
identity is present. |
+| `principal` | Caller identity: username for `Basic` simple auth,
`bearer:<first-16-hex-of-sha256-of-token>` for a Bearer token (a fingerprint of
the token, so it changes when the token is refreshed),
`oauth:<first-16-hex-of-sha256-of-client-id>` when OAuth client-credentials is
the service identity (stdio or HTTP with no caller header), or `anonymous` when
no identity is present. |
Review Comment:
[Nit] The doc correctly says the value changes when the token is refreshed.
Consider adding that the principal is therefore a per-token fingerprint, not a
stable user identity, and that per-user investigation should use the Gravitino
server-side audit log.
##########
mcp-server/tests/unit/test_audit.py:
##########
@@ -108,8 +110,37 @@ def test_empty_header_returns_anonymous(self):
def test_none_like_empty_returns_anonymous(self):
self.assertEqual(audit._extract_principal(None), "anonymous")
- def test_short_token_uses_full_token(self):
- self.assertEqual(audit._extract_principal("Bearer abc"), "bearer:abc")
+ def test_short_token_is_fingerprinted(self):
+ self.assertEqual(
+ audit._extract_principal("Bearer abc"),
+ "bearer:ba7816bf8f01cfea",
+ )
+
+ def test_jwts_sharing_a_header_get_distinct_principals(self):
+ tokens = [_jwt_for(subject) for subject in ("alice", "bob", "carol")]
+ self.assertEqual(len({token.split(".")[0] for token in tokens}), 1)
+ principals = {
+ audit._extract_principal(f"Bearer {token}") for token in tokens
+ }
+ self.assertEqual(len(principals), 3)
+
+ def test_same_claims_under_another_signature_get_another_principal(self):
+ self.assertNotEqual(
+ audit._extract_principal(f"Bearer {_jwt_for('alice')}"),
+ audit._extract_principal(f"Bearer {_jwt_for('alice', b'forged')}"),
+ )
+
+ def test_jwt_principal_is_the_fingerprint_of_the_whole_token(self):
Review Comment:
[Nit] These hardcoded digests will fail together if the format changes,
which is fine and intentional. Optionally, compute the expected value with
`hashlib.sha256(...)` for at least one of them so the test also documents how
the value is derived.
##########
mcp-server/mcp_server/core/audit.py:
##########
@@ -54,7 +55,8 @@ def _extract_principal(authorization: str) -> str:
return "anonymous"
user = decoded.split(":", 1)[0]
return user if user else "anonymous"
- return f"{scheme}:{credential[:8]}"
+ digest = hashlib.sha256(credential.encode("utf-8", "surrogatepass"))
Review Comment:
[Nit] The hash is unkeyed, so a short or low-entropy credential (for example
a static `ApiKey 1234` under a custom scheme) can be brute-forced offline from
the audit log. That is still much better than logging the first 8 characters,
and I don't think an HMAC with a configured secret is worth the extra config
for now. It would be worth saying in the docs that the fingerprint is meant for
high-entropy tokens.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
