Skip to content

docs: expand security threat model - #1886

Open
zeroshade wants to merge 4 commits into
apache:mainfrom
zeroshade:docs/security-threat-model-review
Open

docs: expand security threat model#1886
zeroshade wants to merge 4 commits into
apache:mainfrom
zeroshade:docs/security-threat-model-review

Conversation

@zeroshade

Copy link
Copy Markdown
Member

Summary

  • adapt the expanded Iceberg Python threat-model structure to Iceberg Go's actual catalog, REST, configuration, IO, and CLI boundaries
  • preserve Iceberg Go-owned credential isolation across separately constructed catalog/client instances while distinguishing caller-shared state
  • clarify private vulnerability reporting, delegated credentials, endpoint routing, resource limits, table provenance, and destructive external-path behavior
  • make scanner calibration conditional and require human review instead of blanket rejection

Review notes

The Go model intentionally does not copy Python's blanket same-process isolation disclaimer or assume universal table-root containment. Availability-only findings remain hardening by default unless they violate a documented Iceberg Go-owned boundary or demonstrate another security impact.

Verification

  • go test ./... — 28 packages passed; 6 packages had no tests
  • git diff --check upstream/main..HEAD
  • final task review and whole-branch security-policy review completed with no remaining Critical or Important findings

@laskoviymishka laskoviymishka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few non-blocking comments on the parts I'd want us to be deliberate about before this merges.

Comment thread SECURITY-THREAT-MODEL.md Outdated
precondition.

This model is scoped to the Apache Iceberg Go repository itself:
In this document, a **new audience** is any log, error, CLI text or JSON output,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two terms, 'new audience' here and 'documented Iceberg Go-owned availability boundary' just below, end up governing almost every Boundary, In-Scope, and Out-of-Scope entry, but they're defined inside the Purpose prose. Someone landing directly on Boundary 3 or Out-of-Scope 7 (which the doc itself says is a real access pattern for maintainers and scanners) hits them with no definition in view. Not blocking, but I think a short Definitions section right before Trust Boundaries would make the governing terms much easier to find. wdyt?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. These terms carry enough weight that they should be independently linkable rather than embedded in the Purpose prose. I’ll move them into a short Definitions section before the substantive scope and boundary rules.

Comment thread SECURITY-THREAT-MODEL.md
- catalog and REST catalog clients
- transport, credential, and configuration handling implemented in this repo
- command-line or helper tooling shipped in this repo
A **documented Iceberg Go-owned availability boundary** is an explicit

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every Out-of-Scope override leans on this term ('reassess if a finding violates a documented Iceberg Go-owned availability boundary'), but the only example given is the Puffin 256 MB limit. A maintainer triaging, say, an Avro decode cap or a manifest slice limit can't tell whether it's a documented and owned boundary without auditing the code first. Not a blocker, but a short non-exhaustive list of the enforced limits, or even just a pointer like 'see the puffin and table package docs', would let the rule be applied without a codebase dig.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I’ll add a short non-exhaustive list containing the Puffin blob limit and the documented REST metrics-report deadline. I’ll also state that incidental integer, slice, batching, or worker limits do not qualify unless a public API or project document establishes them as an enforced resource boundary. That should make the rule usable without requiring a codebase dig while avoiding a stale inventory of internal constants.

Comment thread SECURITY-THREAT-MODEL.md Outdated
This means a malicious catalog supplying incorrect or malicious metadata is
usually not an Iceberg Go vulnerability by itself.
Iceberg Go internally creates REST sessions and per-catalog authentication,
transport, metrics, and delegated-credential state. Separately constructed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small inconsistency worth resolving: 'metrics' is listed here (and in Security Goals) as Iceberg Go-owned state that separately constructed catalogs must not share, but Out-of-Scope 6 says sharing a registry implementation is caller-owned and not a security issue. An app that deliberately shares a metrics.Registry across two catalogs lands in both buckets at once, so a maintainer could reach opposite conclusions depending on which section they read first. Metrics also aren't usually credential-bearing, so either dropping 'metrics' from the isolation set here, or a sentence on why cross-catalog metric aggregation is security-relevant, would settle it. Non-blocking.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch—the bare term “metrics” obscures two different ownership cases. The intended in-scope state is a catalog-created REST reporter/dispatcher, which uses that catalog’s authenticated client and therefore must remain isolated. A caller deliberately sharing a metrics.Reporter, or registering a factory that returns shared state, is caller-owned. I’ll replace the broad wording with those concrete terms in Security Goals, Boundary 5, In-Scope 2, and Out-of-Scope 6.

Signed-off-by: Matt Topol <matt@columnar.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants