[SPEC] Add finer grained read restrictions as part of loadTable#13879
[SPEC] Add finer grained read restrictions as part of loadTable#13879singhpk234 wants to merge 45 commits into
Conversation
1234341 to
6422478
Compare
9c1b305 to
6c5b955
Compare
|
Re: Schema evolution cases |
Update - actively trying a POC for this in my fork (PR : singhpk234#270 if folks are interested to give some early feedback) |
b1e909b to
f5ed94e
Compare
…ULL note Per rdblue review: predicates (not expressions in general) are what cannot produce NULL. Updates the required-row-filter description in both rest-catalog-open-api.yaml and the regenerated .py.
Introduces the Action abstraction from the ReadRestrictions spec (PR apache#13879) as a standalone addition, covering the 10 predefined column projection actions: - MaskAlphanum, ShowFirst4, ShowLast4, MaskToFixedValue, ReplaceWithNull, TruncateToYear, TruncateToMonth, Sha256Global, Sha256QueryLocal, ApplyExpression - Plus an Unknown forward-compat carrier for unrecognized server-side action types so the client fails closed at bind time Each action carries a field id plus any action-specific payload and owns its own bind(Type) returning a SerializableFunction. This mirrors the Transform<S, T> pattern used in partition transforms. Null input produces null output for every action, per spec. No REST wire-format plumbing or engine integration yet; those follow in separate PRs.
… map Extends canBind() and defaultValueFor() to cover the complex types listed in the ReadRestrictions spec for mask-to-fixed-value: - variant -> empty Variant (V1 metadata + empty object value) - geometry -> WKB POINT EMPTY (little-endian, X=Y=NaN) - geography -> WKB POINT EMPTY (same encoding) - list -> empty list - map -> empty map struct is intentionally deferred (TODO) until the recursive type-specific default helper lands; the spec allows excluding nested types per rdblue review (apache#13879).
Per rdblue review: POINT EMPTY's WKB encoding is not defined in Iceberg's format spec (Appendix G defers to OGC SFA, which does not specify empty-geometry WKB), and is not formally standardized in GeoParquet, PostGIS, Apache Parquet's geospatial spec, or any other source that Iceberg cites. The "POINT(NaN, NaN)" convention is widely-used implementation lore but never canonicalized. This also aligns with format/spec.md L333 which states that columns of unknown, variant, geometry, and geography types must default to null and explicitly disallows non-null initial-default / write-default values for those types. Drops: - geometry: POINT EMPTY - geography: POINT EMPTY Tightens Applicable-to clause to exclude unknown, geometry, geography. variant is kept in the fixed-value table because Iceberg's Java API defines a canonical empty Variant byte sequence (EMPTY_V1_BUFFER), giving an implementation-anchorable encoding that geometry/geography lack.
| - float: 0.0 | ||
| - double: 0.0 | ||
| - decimal(p, s): 0 (zero with s digits after the decimal point, e.g. 0.00 for decimal(p,2)) | ||
| - string: "XXXXXXXX" |
There was a problem hiding this comment.
Reopening to flag a few concerns the thread didn't engage. What other systems use as the default string constant for the equivalent "replace with fixed value" masking:
| System | Default string constant | Source |
|---|---|---|
SQL Server Dynamic Data Masking default() |
"XXXX" (4 X's, or fewer if column narrower) |
define-a-dynamic-data-mask |
| BigQuery Default masking value | "" (empty string) |
column-data-masking-intro#masking_options |
Oracle DBMS_REDACT FULL redaction |
" " (single space) |
Oracle Data Redaction 3.1 |
| Snowflake masking policy | user-defined; docs example uses '********' |
CREATE MASKING POLICY example |
PostgreSQL Anonymizer MASKED WITH VALUE |
user-defined; docs example uses 'CONFIDENTIAL' |
Destruction |
Of the three systems that ship a mandated default, none use 8 X's: SQL Server picks 4 X's, BigQuery picks empty, Oracle picks a single space. The specific choice of "XXXXXXXX" has no precedent.
Other concerns:
-
Breaks the spec's own internal pattern. Every other fixed value in this action is the type's empty/minimum form:
int: 0,binary: empty byte sequence,list: empty list,map: empty map,variant: {}. The matching minimum forstringis""."XXXXXXXX"is the only entry that violates the convention. -
The mask-alphanum analogy isn't quite applicable here.
mask-alphanumpreserves length and replaces characters in place — that's where the familiar X-pattern comes from.mask-to-fixed-valueis the "replace entirely with a constant" mode, a different design. Users who want X-pattern masking already havemask-alphanum.
Suggest string: "" — matches BigQuery, matches this action's per-type empty/collection pattern.
- Use 'reader' consistently in loadTable read-restrictions prose - Rename 'descendant field-id' to 'nested field-id' to match spec terminology - State both actors in the duplicate field-id rule (server must not return, reader must fail) - Forbid mask-to-fixed-value on containers nesting unknown/geometry/geography Regenerate rest-catalog-open-api.py.
Introduces the IcebergFunction<S, T> abstraction from the ReadRestrictions spec (PR apache#13879), covering the 9 predefined column projections used by fine-grained access control: - MaskAlphanum, ShowFirst4, ShowLast4 - MaskToFixedValue, ReplaceWithNull - TruncateToYear, TruncateToMonth - Sha256Global, Sha256QueryLocal (via SaltedFunction, requires salt >= 16 B) Plus UnknownFunction as a forward-compat carrier for unrecognized server-side names so clients fail closed at bind time. Each function carries a field id and any per-function payload and owns bind(Type), returning a SerializableFunction. Mirrors the Transform<S, T> pattern used in partition transforms. Null input produces null output for every function except MaskToFixedValue, which returns the type-specific fixed value per spec. No REST wire-format plumbing or engine integration yet; those follow in separate PRs.
| action: mask-alphanum | ||
| required-row-filter: | ||
| type: eq | ||
| term: region |
There was a problem hiding this comment.
Projections use field-id. Should the row filter use field id too?
There was a problem hiding this comment.
required-row-filter is an iceberg expression which refers to one or many field ids
There was a problem hiding this comment.
We need new expression to spec to have IDReference, added a pr for this #17138
flyrain
left a comment
There was a problem hiding this comment.
LGTM. Thanks a lot for working on it, @singhpk234 !
laurentgo
left a comment
There was a problem hiding this comment.
overall okay with the current proposal, my only possible regret is the lack of header/signal sent by client to advertise support as a sanity check for servers
| Replaces the column value with a type-specific fixed value. Readers must use exactly the values listed below to ensure consistency across implementations. | ||
| Fixed values by type: - boolean: false - int: 0 - long: 0 - float: 0.0 - double: 0.0 - decimal(p, s): 0 (the unscaled value is 0) - string: "XXXXXXXX" - date: 1970-01-01 - time: 00:00:00 - timestamp: 1970-01-01T00:00:00 - timestamptz: 1970-01-01T00:00:00+00:00 - timestamp_ns: 1970-01-01T00:00:00.000000000 - timestamptz_ns: 1970-01-01T00:00:00.000000000+00:00 - uuid: 00000000-0000-0000-0000-000000000000 - fixed(n): n zero bytes - binary: empty byte sequence - variant: {} - list: empty list [] - map: empty map {} - struct: struct with each field set to its type-specific default (applied recursively) | ||
| NULL input is also replaced with the type-specific fixed value; NULL is not preserved. | ||
| Applicable to: all data types except unknown, geometry, and geography. Because those three types have no fixed value defined above, a catalog server must not return mask-to-fixed-value for a struct, list, or map that contains a field of type unknown, geometry, or geography at any nesting depth. |
There was a problem hiding this comment.
(nit) not sure if it is better to come up with a list where this action is supported vs a list where this action is not supported. It seems safer to use the former so that new types would be "blocked" until the spec is updated, vs making the sure this part of the spec is also updated when you type is being addred (if needed).
Or maybe Iceberg spec could define a default value per type when it makes sense and refer to that generically
|
|
||
| NULL input is also replaced with the type-specific fixed value; NULL is not preserved. | ||
|
|
||
| Applicable to: all data types except unknown, geometry, and geography. |
There was a problem hiding this comment.
(nit) not sure if it is better to come up with a list where this action is supported vs a list where this action is not supported. It seems safer to use the former so that new types would be "blocked" until the spec is updated, vs making the sure this part of the spec is also updated when you type is being addred (if needed).
Or maybe Iceberg spec could define a default value per type when it makes sense and refer to that generically
There was a problem hiding this comment.
I agree allowlist might be better in this scenario because v4 is gonna add more type and its gonna add more confusion let me give this more thought
| action: mask-alphanum | ||
| required-row-filter: | ||
| type: eq | ||
| term: region |
There was a problem hiding this comment.
required-row-filter is an iceberg expression which refers to one or many field ids
|
|
||
| MaskAlphanum: | ||
| description: > | ||
| Redacts the column value using the following rules to transform Unicode code points: |
There was a problem hiding this comment.
(nit) it seems we are using the word replaces for most actions, I wonder if we should align by saying "Replaces the column value with a redacted value created using the following rules..."
| - float: 0.0 | ||
| - double: 0.0 | ||
| - decimal(p, s): 0 (zero with s digits after the decimal point, e.g. 0.00 for decimal(p,2)) | ||
| - string: "XXXXXXXX" |
There was a problem hiding this comment.
I don't care as well, discussing it more is just bikeshedding at this point.
| - geography: POINT EMPTY | ||
| - list: empty list [] | ||
| - map: empty map {} | ||
| - struct: struct with each field set to its type-specific default (applied recursively) |
| 5. The reader must fail the query if a projection references a field-id | ||
| that is not present in the read schema. |
There was a problem hiding this comment.
I don't think this is correct. A read restriction should be valid if the referenced id is in any valid schema for the table (not just the current one or the one used in the current read). The way this is worded is confusing because a schema could be used to read that isn't the current schema and the restrictions may reference columns that did not exist in that schema. We should limit this.
There was a problem hiding this comment.
We discussed this in sync. My understanding is that we're concerned about what happens when projection schema and read schema differ. Here's a table to illustrate.
| # | Field-id in projection schema? | Field-id in read schema? | example When this happens | Recommendation |
|---|---|---|---|---|
| 1 | ✓ Yes | ✓ Yes | Normal read | Enforce policy |
| 2 | ✓ Yes | ✗ No | Time travel to a snapshot predating the column | Fail the query — a policy that favors reads would need to define how to fill in missing columns. Recommend deferring; BigQuery takes the same position and fails this case. |
| 3 | ✗ No | ✓ Yes | Column dropped via ALTER TABLE, reading a historical snapshot where the column still existed |
Allow read — not selecting data that no longer exists is itself a valid form of protection (@rdblue stated this principle in sync) |
| 4 | ✗ No | ✗ No | n/a included for completeness | n/a |
I'd like to recommend the following spec text.
(5) The reader MUST fail the query if a projection references a field-id
that is not present in the snapshot schema.If we would like to be more clear about allowing historical reads we can add a clarification, but I think (5) is sufficient.
(6) The reader MUST skip enforcement if a field-id is present in the snapshot schema but not referenced by the projection.
cc: @singhpk234.
There was a problem hiding this comment.
Thanks Dan and Nevin
This is what i ended up writing :
A reader must enforce projections on the columns it is actually reading. Projections referencing columns that are not being read do not apply.
We want to say if you are reading a column and if there is read restriction projection for it then you must apply that read restriction, if you can't you must fail and if you are not reading that particular column (doesn't matter if its part of the snapshot schema you are reading vs even if there is a column which is present in the snapshot schema).
Let me know you thouhgts
- Reframe rule 5 around what the reader is actually reading per Ryan's sync framing, dropping the invented "read schema" vocabulary that Dan flagged as confusing. - Replace "authenticated principal" and "global policy" (undefined terms per Dan) with "authentication scope" in the loadTable caching note and drop the phrase from required-row-filter. - Scope the fail rule to conforming readers per huaxingao's nit. - Reword the "missing or empty" clause to drop the overloaded "field" per flyrain's nit. - Move the required/non-nullable note into ReplaceWithNull's "Applicable to" line per flyrain's nit.
Per laurentgo's review nit on yaml:3835 — flip from an exclusion list
("all types except unknown, geometry, geography") to an inclusion by
reference to the "Fixed values by type" list above. New Iceberg types
are then implicitly blocked until the spec explicitly extends the
fixed-values list to cover them. The container-nesting carve-out is
dropped: it's redundant with the MUST NOT clause because a struct
containing a field with no fixed value isn't a "type with a fixed value
defined above" — the recursive definition simply doesn't apply.
About the proposal
This aims at returning, policy evaluation result (Access decisions) for fine grained access policies based on the calling user as part of the loadTable response.
This defines a new object called
ReadRestrictionswhich an optional field catalog would use to convey the access decision, The expectation is these rules (projections and the row filters) are correctly applied and enforced by the client, which brings an implicit requirement to have a trusted partner, establishing trust between callers engine and catalog is not scope of this proposal as its totally up to the catalog on how its established via OAuth Delegation Flow or mTLS.The
ReadRestrictionsreturns back projections which are modeled as Actions and the row filters are modeled as Expressionwe have been iterating about the actions in the community sync :)
https://docs.google.com/document/d/1iGNydKY7XT1N5Nz056vDPM0P8v0MFymGqNtOlUGUp-c/edit?tab=t.0#heading=h.tevndn85fps
https://lists.apache.org/thread/xddrotmxs7tyoc4jc5npwqs40vs120yr
Future Extension
Expression Expansion in Progress, which can allow expressions to contain udfs
Proposal : https://docs.google.com/document/d/1VthBz0S2I39TeQM8oiF9_gSPQu_gHAjWXvdFpv0QqDk
Additional consideration
clients expression handling capability : lets say we introduce a new expression or a transform in the IRC spec, and the catalog returns this new expression, but the client is running on a lower version so it doesn't understand this newly added expression, IMHO its fine if the client fails in interpreting / parsing the expression as it doesn't understands it, open to broader feedback, (similar discussion in past for spark community (here))
Schema evolution cases, consider a column got renamed, essentially the DDM and RAP should be Binded, having the field-id of the column they are referring to additionally sent to the client to correctly apply the read restrictions consder one had a filter on column A (when the policy was attached) which got renamed to column B later when reading the latest version of the schema reader could essentially apply filter on B essentially adhering to the column projection rule - https://iceberg.apache.org/spec/#column-projection
Acknowledgement
Many thanks to the entire Iceberg community for the extensive discussions and invaluable feedback over the years, which have been instrumental in shaping this proposal into its current form.
Issues
dev lists
[1] https://lists.apache.org/thread/4swop72zgcr8rrmwvb51rlk0vnb8joyz
[2] https://lists.apache.org/thread/8t2zh9nchklm4zwjj89vnq9fg9wv45o4
docs
[1] https://docs.google.com/document/d/14nmuxxfzQsYo59o0Fbpb-pxOlzS6bVtduL8P8pwKZ6U/edit?tab=t.0#bookmark=id.eekzk8xl6uo
[2] https://docs.google.com/document/d/108Y0E8XsZi91x-UY0_aHLEbmXDNmxmS5BnDjunEKvTM/edit?tab=t.0
syncs
[1] https://www.youtube.com/watch?v=RRyohCUDnME