Skip to content

Commit e0ef16e

Browse files
patelchaitanyntkathole
patelchaitany
authored and
ntkathole
committed
fix: Authorize registry server writes before deserializing UDFs
ApplyValidationReference and ApplyMaterialization built the full domain object inside the argument list of their permission check: assert_permissions( resource=FeatureView.from_proto(request.feature_view), actions=[AuthzedAction.WRITE_ONLINE], ) Python evaluates arguments first, so from_proto() — which dill-loads the caller-supplied UDF — always ran before assert_permissions() could reject the request. Unpickling executes arbitrary code, so an unauthorized caller achieved code execution and only then received a permission error. Load metadata first, authorize, and deserialize only after the check passes. The permission check needs just name/project/tags, never the callable itself. FeatureView.from_proto() already accepted skip_udf; this adds the same parameter to ValidationReference.from_proto(), gating both the GEProfiler and GEProfile dill payloads. As a result profiler may now legitimately be unset, so it becomes Optional and the profile property raises a clear error rather than failing on None. This covers the registry server paths only. Deserialization on the feature server is not addressed here. Signed-off-by: Chaitany Patel <[email protected]>
1 parent a03e323 commit e0ef16e

2 files changed

Lines changed: 55 additions & 27 deletions

File tree

‎sdk/python/feast/registry_server.py‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1358,13 +1358,20 @@ def ListDatasetJobs(
13581358
def ApplyValidationReference(
13591359
self, request: RegistryServer_pb2.ApplyValidationReferenceRequest, context
13601360
):
1361-
validation_reference = cast(
1362-
ValidationReference,
1363-
assert_permissions_to_update(
1364-
resource=ValidationReference.from_proto(request.validation_reference),
1365-
getter=self.proxied_registry.get_validation_reference,
1366-
project=request.project,
1367-
),
1361+
# Authorize against metadata only: from_proto() dill-loads the profiler,
1362+
# so building the full object before the permission check would run
1363+
# caller-supplied code pre-authorization.
1364+
validation_reference_meta = ValidationReference.from_proto(
1365+
request.validation_reference, skip_udf=True
1366+
)
1367+
assert_permissions_to_update(
1368+
resource=validation_reference_meta,
1369+
getter=self.proxied_registry.get_validation_reference,
1370+
project=request.project,
1371+
)
1372+
1373+
validation_reference = ValidationReference.from_proto(
1374+
request.validation_reference
13681375
)
13691376
self.proxied_registry.apply_validation_reference(
13701377
validation_reference=validation_reference,
@@ -1453,7 +1460,7 @@ def ApplyMaterialization(
14531460
self, request: RegistryServer_pb2.ApplyMaterializationRequest, context
14541461
):
14551462
assert_permissions(
1456-
resource=FeatureView.from_proto(request.feature_view),
1463+
resource=FeatureView.from_proto(request.feature_view, skip_udf=True),
14571464
actions=[AuthzedAction.WRITE_ONLINE],
14581465
)
14591466

‎sdk/python/feast/saved_dataset.py‎

Lines changed: 40 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ class ValidationReference:
261261
dataset_name: str
262262
description: str
263263
tags: Dict[str, str]
264-
profiler: Profiler
264+
profiler: Optional[Profiler]
265265

266266
_profile: Optional[Profile] = None
267267
_dataset: Optional[SavedDataset] = None
@@ -270,7 +270,7 @@ def __init__(
270270
self,
271271
name: str,
272272
dataset_name: str,
273-
profiler: Profiler,
273+
profiler: Optional[Profiler] = None,
274274
description: str = "",
275275
tags: Optional[Dict[str, str]] = None,
276276
):
@@ -316,28 +316,49 @@ def profile(self) -> Profile:
316316
"to get validation reference object."
317317
)
318318

319+
if not self.profiler:
320+
raise RuntimeError(
321+
"Failed to calculate profile: profiler is not set on this "
322+
"ValidationReference."
323+
)
324+
319325
self._profile = self.profiler.analyze_dataset(self._dataset.to_df())
320326
return self._profile
321327

322328
@classmethod
323-
def from_proto(cls, proto: ValidationReferenceProto) -> "ValidationReference":
324-
profiler_attr = proto.WhichOneof("profiler")
325-
if profiler_attr == "ge_profiler":
326-
from feast.dqm.profilers.ge_profiler import GEProfiler
327-
328-
profiler = GEProfiler.from_proto(proto.ge_profiler)
329-
else:
330-
raise RuntimeError("Unrecognized profiler")
329+
def from_proto(
330+
cls, proto: ValidationReferenceProto, skip_udf: bool = False
331+
) -> "ValidationReference":
332+
"""Build a ValidationReference from its proto.
331333
332-
profile_attr = proto.WhichOneof("cached_profile")
333-
if profile_attr == "ge_profile":
334-
from feast.dqm.profilers.ge_profiler import GEProfile
335-
336-
profile = GEProfile.from_proto(proto.ge_profile)
337-
elif not profile_attr:
338-
profile = None
339-
else:
340-
raise RuntimeError("Unrecognized profile")
334+
Args:
335+
proto: the serialized validation reference.
336+
skip_udf: when True, leave ``profiler`` and the cached profile unset
337+
instead of deserializing them. Both are dill payloads, so callers
338+
that only need identity metadata (notably authorization checks in
339+
the registry server) must not deserialize them.
340+
"""
341+
profiler: Optional[Profiler] = None
342+
profile: Optional[Profile] = None
343+
344+
if not skip_udf:
345+
profiler_attr = proto.WhichOneof("profiler")
346+
if profiler_attr == "ge_profiler":
347+
from feast.dqm.profilers.ge_profiler import GEProfiler
348+
349+
profiler = GEProfiler.from_proto(proto.ge_profiler)
350+
else:
351+
raise RuntimeError("Unrecognized profiler")
352+
353+
profile_attr = proto.WhichOneof("cached_profile")
354+
if profile_attr == "ge_profile":
355+
from feast.dqm.profilers.ge_profiler import GEProfile
356+
357+
profile = GEProfile.from_proto(proto.ge_profile)
358+
elif not profile_attr:
359+
profile = None
360+
else:
361+
raise RuntimeError("Unrecognized profile")
341362

342363
ref = ValidationReference(
343364
name=proto.name,

0 commit comments

Comments
 (0)