Skip to content

docs: add Security and Governance and compliance pages - #850

Merged
atriaybagur merged 5 commits into
developfrom
claude/docs-security-page
Aug 5, 2026
Merged

docs: add Security and Governance and compliance pages#850
atriaybagur merged 5 commits into
developfrom
claude/docs-security-page

Conversation

@atriaybagur

@atriaybagur atriaybagur commented Jul 31, 2026

Copy link
Copy Markdown
Member

Description

The published documentation had no security page and no governance page. What existed was a four-paragraph Security section inside Platform Support, covering per-trust API keys and payload encryption and nothing else — no MFA, no RBAC, no cohort-query boundary, no cloud posture, no FL transport security, no supply chain, and nothing at all on information governance. A partner trust assessing FLIP had nowhere to look.

This adds two top-level pages at the end of the Documentation toctree, fixes the stale section they supersede, and clears every pre-existing warning from the docs build.

1. security.rst — what protects the platform

Describes the controls layer by layer:

  • Network and perimeter — outbound-only trusts, no inbound rules, SSM-only operator access, optional site-to-site VPN
  • Cloud infrastructure — AWS Landing Zone Accelerator, least-privilege IAM, private subnets, CMK encryption, RDS Proxy IAM auth, IaC, federated CI credentials
  • Identity and access — Cognito SRP, token verification, mandatory MFA on every request, three-role default-deny RBAC, XNAT/DQR separation, per-trust machine credentials
  • The clinical data boundary — read-only role, sqlglot parse-and-re-emit, AST-decided statement types, schema pinning, threshold suppression (the threshold being each trust's own disclosure floor, set in its deployment kit), project-scoped cache
  • Federated learning and privacy — mutual TLS on both backends, no hub credentials on FL clients, accurate description of the update filter
  • Data in transit and at rest — payload encryption stated precisely (a single platform-wide symmetric key today, with authenticated per-trust keys in delivery), S3/RDS/EBS encryption, pre-signed URL ceiling, credential rotation, Orthanc auth
  • Diagnostics and disclosure control — the correlation-identifier mechanism and the CI guard against raw exception text stated as in delivery, not shipped, alongside what holds today
  • Software supply chain and change control — secret scanning, 72-hour dependency cooldown, test-gated publishing, sha-pinned deploys
  • Assurance and vulnerability reporting — pointing at SECURITY.md

2. governance-and-compliance.rst — who decides, and how we map to the frameworks

A different question for a different reader (information governance and Caldicott functions, ethics committees, commissioners):

  • Per-project, per-trust veto — approval is not a blanket agreement to participate; the decision runs on the trust's own deployment so it cannot be bypassed, and declining excludes that trust without affecting the federation
  • Data residency — what actually crosses the boundary in each direction
  • The suppression threshold is the trust's own setting — each trust sets its disclosure floor in its own deployment kit; trusts need not agree on a shared value, and the hub cannot lower it
  • National Data Opt-Out — applied by periodic reconciliation of the OMOP dataset rather than in the query layer, so it propagates to every query the platform can make
  • Network architecture as a governance guarantee, access governance, independent assurance
  • Three appendices mapping the platform against Cyber Essentials, the NHS DSPT (via the NDG's ten Data Security Standards), and the NCSC Cyber Assessment Framework, with an explicit status column

Statuses distinguish controls that are the platform's from those belonging to the deploying trust, and from those that are shared. An all-green mapping would be misleading — several of these standards are genuinely organisational.

3. Platform Support trimmed

The old Security section is removed as superseded — its content (per-trust API keys, shared-key payload encryption) is covered in far more detail on the new page.

Networking repeated the outbound-only posture three times across the section and again in the ports preamble. Trimmed to one paragraph plus a pointer to Security, keeping everything operational — the architecture figure, the full firewall-rules table, make forward-trust, and "port 22 is never opened". Its VPN paragraph said the tunnel "is not yet implemented ... but is planned as part of the production rollout", which no longer matches; reworded to "provisioned on request rather than by default".

4. Docs build is now warning-free

The build previously emitted eight warnings, all from docstrings written as if they were Markdown. Three source docstrings accounted for all eight (the constants one counted twice, because autoapi emits both a module and a package page):

  • flip/constants/flip_constants.py — bullet list with no preceding blank line → Unexpected indentation / Block quote ends without a blank line
  • flip/flower/metrics.py — indented example after a plain : made the previous line a definition-list term → changed to ::
  • flip_api/scripts/delete_trust.py — a Markdown pipe table; RST parses leading | as a line block and |---...---| as a substitution reference → moved into a literal block

All comment-only; no behaviour affected. make -C docs docs now reports build succeeded with no warnings.

Stated deliberately rather than glossed

Because the audience is information governance, the pages mark the line between shipped controls and in-flight work explicitly (the last two tightened in the review round):

  • The FL update filter is described as statistical clipping and sparsification, and explicitly not as differential privacy — there is no calibrated noise and no privacy budget. Presenting it as DP to an ethics committee would misstate it.
  • Researcher-supplied training code runs on trust hardware with access to that trust's data. That is inherent to federated learning, so the page says so plainly and then sets out the controls around it.
  • Payload encryption today uses a single platform-wide symmetric key, with integrity provided by the TLS layer; authenticated encryption with per-trust identified keys is described as in delivery (FLIP-PT-004: authenticated AES-GCM payload encryption + per-trust keys #845), not as current behaviour.
  • The fixed-message-plus-correlation-identifier error mechanism and its CI guard are described as in delivery, with today's behaviour (some error paths still return exception text) stated plainly.

Neither page carries file paths, line numbers, or internal finding identifiers, so both are safe to share with external stakeholders.

Linked Issues

Checklist

  • Follows the project's coding conventions and style guide
  • Updates documentation
  • I have added tests that prove my fix is effective or that my feature works — n/a, documentation only
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Type of Change

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • In-line docstrings updated.
  • Documentation updated, tested make -C docs/ docs.

Testing

  • make -C docs docs builds clean, zero warnings (previously eight), re-verified after the review-round changes.
  • Verified the rendered HTML rather than only the build log: both pages exist, all section headings render, the six list-tables in the appendices render as tables, both pages appear in the Documentation sidebar in the right order, and the old #security anchor is gone from admin-platform-support.html.
  • All cross-references resolve — :ref:security, `:ref:`rbac-roles, :ref:deploy-flip-node-on-prem``.
  • ruff clean on the three touched Python files; mypy clean on delete_trust.py; the flip-api tests covering it pass.

Not verified here: flip-utils' own test suite could not run in this environment (its dependencies are not installed and cannot be fetched). Both touched files parse and are ruff-clean, and autoapi re-parsed them to regenerate the API docs, but the suite itself has not run against these changes. They are docstring-only edits.

Additional Notes

Sibling pages use sentence-case titles without a shared prefix (Overview, FLIP components, System administration), so the pages are titled Security and Governance and compliance. "Security" is what a reader scans the sidebar for, and neither title dates the way "Security Review" would as the platform evolves.

The per-trust threshold wording depended on the kit-file plumbing from #839 (COHORT_QUERY_THRESHOLD in the trust kit). #839 merged on 2026-08-05 (01292201), so that ordering constraint is discharged and both passages now describe shipped behaviour: the setting ships in the GSTT/KCH/generic kit examples, reaches data-access-api via ${COHORT_QUERY_THRESHOLD:-10} in both compose_trust.development.yml and compose_trust.production.yml, and the max(threshold, get_settings().COHORT_QUERY_THRESHOLD) clamp in data_access_api/services/cohort.py means a hub-supplied value can only raise the floor. The encryption and correlation-identifier paragraphs stay marked in delivery#845 is still open.

Commit 4 of 4 touches flip-utils/ and flip-api/ rather than docs/. It is documentation-only in effect — three docstrings — but happy to split it out if you would rather keep the PR to docs/.

The published documentation had no security page. What existed was a four-paragraph
"Security" section inside Platform Support, covering per-trust API keys and payload
encryption and nothing else — no mention of MFA, RBAC, the cohort-query boundary, the
cloud posture, FL transport security, or the supply chain. A partner trust assessing
FLIP had nowhere to look.

Adds `security.rst` at the end of the Documentation toctree, describing the controls
layer by layer: network and perimeter, cloud infrastructure, identity and access, the
clinical data boundary, federated learning, data in transit and at rest, disclosure
control in error paths, supply chain and change control, and how to report a
vulnerability.

Removes the old section from Platform Support. Beyond being superseded, it had gone
stale: it stated that payloads are encrypted with a shared `AES_KEY_BASE64`, which no
longer describes the scheme. Platform Support keeps Networking, Backup/Restore and
Access, which are operator-facing rather than assurance-facing.

Two things are stated deliberately rather than glossed, because the audience is
information governance:

- The FL update filter is described as statistical clipping and sparsification, and
  explicitly **not** as differential privacy — there is no calibrated noise and no
  privacy budget. Presenting it as DP to an ethics committee would misstate it.
- Researcher-supplied training code runs on trust hardware with access to that trust's
  data. That is inherent to federated learning, so the page says so plainly and then
  sets out the controls around it.

The page carries no file paths, line numbers, or internal finding identifiers, so it is
safe to share with external stakeholders.

Cross-references the existing on-prem deployment guide for trust firewall rules and the
RBAC role matrix for the full permission table. Sphinx builds clean — the eight
remaining warnings are pre-existing autoapi issues in generated files, unrelated to this
page.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU
Signed-off-by: Claude <noreply@anthropic.com>
Two follow-ups from review of the new Security page.

**User Roles.** The roles sentence now links to the permission matrix at the point the
three roles are named, rather than deferring to a pointer at the end of the paragraph —
a reader who wants the detail is looking for it right there. Uses the existing
`rbac-roles` cross-reference so it resolves in every build, including the PR preview.

**Networking in Platform Support** repeated the outbound-only posture three times
across the section and again in the ports preamble, most of which the Security page now
covers. Trimmed to one paragraph of substance plus a pointer, keeping everything
operational: the architecture figure, the ports table, the SSM port-forwarding command,
and the fact that port 22 is never opened.

The VPN paragraph also said the tunnel "is not yet implemented — current deployments
rely on HTTPS over the public internet — but is planned as part of the production
rollout", which no longer matches: it is provisioned on request. Reworded accordingly,
and phrased so it does not imply every deployment has one.

Sphinx builds clean; verified both new cross-links resolve in the rendered HTML and the
ports table is intact.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU
Signed-off-by: Claude <noreply@anthropic.com>
The docs build carried eight warnings, all from docstrings that autoapi renders as
reStructuredText but which were written as if they were Markdown. Three source
docstrings account for all eight — the constants one is counted twice because autoapi
emits both a module page and a package page from it.

- `flip/constants/flip_constants.py` — a bullet list with no blank line before it, so
  RST read the continuation line as an unexpected indent inside a paragraph
  (*Unexpected indentation*, *Block quote ends without a blank line*). Added the blank
  line; it now renders as an actual list.
- `flip/flower/metrics.py` — an indented usage example after a plain `:` made the
  preceding line a definition-list term (*Definition list ends without a blank line*).
  Changed to `::` so it is an explicit literal block.
- `flip_api/scripts/delete_trust.py` — a Markdown pipe table. RST parses leading `|`
  as a line block and reads `|---...---|` as a substitution reference, which produced
  the two *Undefined substitution referenced* errors and the *Line block ends without a
  blank line* warning. Moved into a literal block, which keeps the table readable both
  in the source and on the rendered page.

All are comment-only changes; no behaviour is affected. `make -C docs docs` now reports
**build succeeded** with no warnings, and the three sites were checked in the rendered
HTML rather than only in the build log.

Verified: ruff clean on all three files, mypy clean on `delete_trust.py`, and the
flip-api tests covering it pass. flip-utils' suite could not run here — its dependencies
are not installed in this environment and cannot be fetched — but both files parse and
ruff is clean, and autoapi re-parsed them to regenerate the docs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU
Signed-off-by: Claude <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Companion to the Security page, at the end of the Documentation toctree. Security
answers "what protects this"; this answers "who decides, and how does it map onto the
frameworks we are assessed against" — a different question, asked by a different reader
(information governance and Caldicott functions, ethics committees, commissioners).

Covers the governance properties that are not obvious from the code:

- **Per-project, per-trust veto.** Approval is not a blanket agreement to participate. A
  trust approves each project individually, the decision runs on its own deployment so
  it cannot be bypassed, and declining excludes that trust without affecting the rest of
  the federation.
- **Data residency** — what actually crosses the boundary in each direction, stated
  explicitly, since it is the first question an IG function asks.
- **National Data Opt-Out** — applied by periodic reconciliation of the OMOP dataset
  rather than in the query layer, so it propagates to every query the platform can make.
  Two caveats are recorded rather than glossed: reconciliation is periodic rather than
  per-query, so the interval is the control question; and the platform inherits the
  decision rather than enforcing it.
- **Network architecture as a governance guarantee** — kept short and pointed at the
  Security page for the mechanism, rather than restating it.
- Access governance and independent assurance.

Three appendices map the platform against **Cyber Essentials**, the **NHS DSPT** (via
the National Data Guardian's ten standards), and the **NCSC Cyber Assessment
Framework**, as `list-table`s with an explicit status column. Statuses distinguish
controls that are the platform's from those that belong to the deploying trust, and from
those that are shared — an all-green mapping would be misleading, since several of these
standards are genuinely organisational.

Cross-references Security, User Roles, and the on-prem deployment guide rather than
duplicating them. Sphinx builds clean with no warnings; verified the six tables render
and all three cross-links resolve.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UY3k5789vkwUEL613TKxjU
Signed-off-by: Claude <noreply@anthropic.com>
… threshold

Address review: the security page described per-trust authenticated payload
encryption (key identifiers, AEAD) and the correlation-ID error mechanism with
its CI guard as current behaviour; both are in-flight work. Rewritten to state
today's shared-key scheme and error paths precisely, with the upgrades marked
as in delivery. The rotation paragraph and the CAF B3 appendix row are aligned
to match.

Also state that the cohort suppression threshold is the trust's own disclosure
floor, set per trust in its deployment kit, on both the security page and the
governance page.

Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
Comment thread docs/source/security.rst
Comment thread docs/source/governance-and-compliance.rst

@garciadias garciadias left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These two new pages are well-written and thoroughly researched — I independently re-verified the checkable technical claims (MFA enforcement, RDS Proxy IAM auth, the sqlglot-based query validation, the AES single-key model, WAF monitoring-mode status, constant-time credential comparisons) and all of them match the current implementation, and the Sphinx build is clean.

One real issue, though: both pages state as present-tense fact that each trust sets its own cohort-suppression threshold in its deployment kit. That isn't true on develop yet — COHORT_QUERY_THRESHOLD is currently a hardcoded default that no trust kit or compose file actually wires up, which is exactly what PR #839 (open at time of review) delivers. Requesting changes so this doesn't ship a false compliance claim to IG/Caldicott readers if it merges ahead of #839 — either hold this PR until #839 lands, or soften the wording to "in delivery" in the meantime (see inline comments).

@garciadias garciadias assigned atriaybagur and unassigned garciadias Aug 4, 2026
@atriaybagur
atriaybagur requested a review from garciadias August 5, 2026 14:09
@atriaybagur atriaybagur assigned garciadias and unassigned atriaybagur Aug 5, 2026
@garciadias garciadias assigned atriaybagur and unassigned garciadias Aug 5, 2026
@atriaybagur
atriaybagur merged commit 682d15f into develop Aug 5, 2026
30 checks passed
@atriaybagur
atriaybagur deleted the claude/docs-security-page branch August 5, 2026 14:17
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.

4 participants