Skip to content

[DT-3825] Move tests to resource/service so they can be exercised to confirm re… - #2992

Open
otchet-broad wants to merge 10 commits into
developfrom
otchet-dt-3825-confirm-es-in-env
Open

[DT-3825] Move tests to resource/service so they can be exercised to confirm re…#2992
otchet-broad wants to merge 10 commits into
developfrom
otchet-dt-3825-confirm-es-in-env

Conversation

@otchet-broad

Copy link
Copy Markdown
Contributor

Addresses

https://broadworkbench.atlassian.net/browse/DT-3825

Summary

Ticket A-1 needs a written record of which Elasticsearch security features (X-Pack Security,
DLS, FLS, API keys, run_as) are available in each Consent environment, because that record
decides between Epic D (native DLS/FLS enforcement in the cluster) and Epic E
(application-layer filtering).

Getting that record previously meant either cluster network access or a copy of each
environment's credential. Neither is available, and neither should be needed: every environment
already runs a Consent deployment holding its own cluster credential. This adds an admin-only
endpoint that reports the inventory from inside the application, so an Admin token per
environment is the only thing required.

What this adds

GET /api/elasticSearch/capabilities (@RolesAllowed(ADMIN)), backed by
ElasticSearchCapabilityService. It reports cluster name, version, distribution
(Elasticsearch vs. OpenSearch), edition/license, X-Pack Security state, the authenticated
principal and its roles, the credential's cluster privileges, the security-relevant cluster
settings, a capability verdict per feature, REST-client compatibility, a recommendation, and
interpretive notes.

Two modes:

  • Read-only (default) — creates, modifies, and deletes nothing. DLS, FLS, and API-key
    support cannot be proven without writing, so they come back INFERRED_SUPPORTED /
    LICENSE_BLOCKED, reasoned from the license tier. run_as and X-Pack Security are observed.
  • ?writeProbes=true — mints a short-lived API key and authenticates as it, creates a role
    carrying a DLS query and an FLS grant, then searches the real dataset index through keys
    carrying those filters to check the cluster enforces them (a match_none DLS key must
    return zero documents; a key granting one field must return only that field). This matters
    because a Basic-licensed cluster accepts a DLS role descriptor at creation and fails only
    later at search time — a failure mode no license inference can predict.

?runAsUser=<name> optionally targets the run_as probe at a specific user instead of the
credential's own principal.

Verdicts distinguish the things that are easy to conflate

CapabilityVerdict separates LICENSE_BLOCKED (the cluster's answer) from NOT_PERMITTED
(this credential's answer) from UNAVAILABLE (absent, or accepted-and-silently-ignored) from
the INFERRED_* values. Only a license verdict is a fact about the cluster, so only it should
carry weight in the Epic D/E decision. When the deployment's credential lacks
manage_security / manage_api_key, the write probes report themselves as inconclusive and
fall back to the license reading rather than recording a privilege refusal as a verdict against
the native path.

Probe safety

Everything created is namespaced duos-capability-probe-* / duos_dlsfls_probe_*, carries a
10-minute expiry regardless of what happens next, is torn down in a finally block, and any
teardown failure is surfaced in the response notes rather than left in the server log. Every
probe key carries an explicit role_descriptors block — including the plain round-trip key,
whose descriptor grants nothing — so no key inherits a snapshot of the deployment credential's
permissions.

Wiring change: one shared RestClient

ConsentModule now provides RestClient as a singleton and registers a named
ElasticSearchClientShutdown Managed to close it. ElasticSearchService takes the injected
client rather than building its own. Each RestClient owns a connection pool and background
threads, so a client per consumer multiplied pools against the same cluster and left them
unclosed at shutdown.

Removed

  • scripts/es-security-audit.sh — reimplemented the whole verdict matrix in untested bash.
  • ElasticSearchSecurityProbeTest — drove the security APIs through the production
    ElasticSearchSupport.createRestClient path but was inert unless ES_PROBE_URL was set, so
    it never ran in CI.

The service supersedes both: same calls, unit-tested verdict logic, and a successful response
from any environment is itself the REST-client compatibility evidence the probe test existed to
supply. Two implementations that can disagree about "is DLS enforced here" are worse than one
that is tested.

Docs

  • docs/plans/es-security-capability-record.md — the record itself: how it's produced, the
    measured local and control-cluster results, empty tables awaiting dev / staging /
    production, the REST-client compatibility conclusion, and the decision rule fixed in advance
    so the measurement determines the outcome.
  • DEVNOTES.md — the local ES section rewritten; it described a 5.5.0 image and manual compose
    edits that no longer match the shipped config/docker-compose.yaml. Now covers running with
    security on, the trial-license-on-an-existing-volume trap, and
    ES_SECURITY_ENABLED=false for a security-disabled run.
  • OpenAPI: paths/elasticSearchCapabilities.yaml and
    schemas/ElasticSearchCapabilityReport.yaml, referenced from api-docs.yaml.

Findings so far

Measured against a local trial-licensed 9.4.4 cluster in write-probe mode, all five
capabilities came back SUPPORTED, with DLS observed enforced (0 of 1158 documents through a
match_none key) and FLS observed projecting. Control clusters on 9.3.3 and 9.4.4 establish
the tier baseline: API keys and run_as are Basic-tier features; DLS and FLS are not.
Epic D therefore carries a license dependency Epic E does not.

The decision stays pending — local runs as the elastic superuser, which is not the shared
credential any deployed environment uses. The dev, staging, and production rows are the
remaining work, and dev is the right place to run write probes first in a deployed environment.

Testing

  • ElasticSearchCapabilityServiceTest — 32 tests over a mocked RestClient, keyed by
    method + endpoint (and by role descriptor where three key creations share both), covering
    each verdict path in both modes and both license tiers.
  • ElasticSearchCapabilityResourceTest — 4 tests.
  • ConsentModuleTest — 3 new tests: the client is a singleton, both ES services share it
    (exactly one shutdown registration), and it is actually closed on stop().

To exercise it against a live cluster:

curl -s -H "Authorization: Bearer $(gcloud auth print-access-token)" \
    'https://<env-host>/api/elasticSearch/capabilities?writeProbes=true' | jq

Have you read CONTRIBUTING.md lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

Copilot AI 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.

Pull request overview

Adds an admin-only Elasticsearch “capability inventory” endpoint to let Consent deployments self-report which ES security features (X-Pack Security, API keys, DLS/FLS, run_as) are available/enforced in each environment, with optional write probes for end-to-end enforcement validation. This fits into the existing Dropwizard + Guice architecture by introducing a new Resource -> Service pair, updating DI to provide a shared ES RestClient, and documenting/typing the response via OpenAPI.

Changes:

  • Introduces GET /api/elasticSearch/capabilities (@RolesAllowed(ADMIN)) and ElasticSearchCapabilityService to probe cluster security capabilities (read-only by default; optional writeProbes=true).
  • Updates Guice wiring to provide a singleton RestClient with lifecycle shutdown management, and injects it into ES services.
  • Adds comprehensive unit tests for capability verdict logic + resource wiring, and adds OpenAPI path/schema plus supporting docs/DEVNOTES updates.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/main/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityService.java Implements the capability probing logic (read-only + optional write probes) and report assembly
src/main/java/org/broadinstitute/consent/http/resources/ElasticSearchCapabilityResource.java Adds the admin-only API endpoint exposing the capability report
src/main/java/org/broadinstitute/consent/http/models/elastic_search/ElasticSearchCapabilityReport.java Defines the JSON response model for the capability report
src/main/java/org/broadinstitute/consent/http/models/elastic_search/ElasticSearchCapability.java Defines a single capability line item in the report
src/main/java/org/broadinstitute/consent/http/models/elastic_search/CapabilityVerdict.java Adds verdict enum values distinguishing observed vs inferred vs blocked vs unknown outcomes
src/main/java/org/broadinstitute/consent/http/ConsentModule.java Provides a singleton ES RestClient, registers shutdown, and wires the new capability service
src/main/java/org/broadinstitute/consent/http/ConsentApplication.java Registers the new capability resource
src/main/resources/assets/api-docs.yaml Adds the new endpoint to the top-level OpenAPI doc
src/main/resources/assets/paths/elasticSearchCapabilities.yaml Defines the OpenAPI path contract for the new endpoint
src/main/resources/assets/schemas/ElasticSearchCapabilityReport.yaml Defines the OpenAPI schema for the report payload
src/test/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityServiceTest.java Adds detailed unit tests for probing/verdict behavior, including write-probe teardown assertions
src/test/java/org/broadinstitute/consent/http/resources/ElasticSearchCapabilityResourceTest.java Adds resource-level tests for parameter passing and error handling
src/test/java/org/broadinstitute/consent/http/ConsentModuleTest.java Adds DI/lifecycle tests ensuring the ES client is shared and shut down
docs/plans/es-security-capability-record.md Documents how to produce and record per-environment findings and the D/E decision rule
DEVNOTES.md Updates local ES development instructions to match the current compose-based setup
Comments suppressed due to low confidence (1)

src/test/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityServiceTest.java:179

  • This helper uses Mockito lenient() stubbing, which this repo’s guidelines explicitly discourage (see .github/copilot-instructions.md). You can avoid lenient stubs here by only stubbing getRequestLine() when you’re constructing an error response (status >= 300), since it’s only needed for ResponseException.
  private Response response(int status, String body) {
    Response response = mock(Response.class);

Comment thread src/main/java/org/broadinstitute/consent/http/ConsentModule.java
@otchet-broad
otchet-broad marked this pull request as ready for review July 30, 2026 13:44
@otchet-broad
otchet-broad requested a review from a team as a code owner July 30, 2026 13:44
@otchet-broad
otchet-broad requested review from kevinmarete and rushtong and removed request for a team July 30, 2026 13:44

@rushtong rushtong 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.

This is great work, thank you 👍🏽
I had one suggestion (inline) and Claude had two minor, non-blocking observations:

  • No multi-node failover during write probes — defaultApiKeyClientFactory (ElasticSearchCapabilityService.java:1018-1024) builds its API-key client from only esClient.getNodes().get(0).getHost(), unlike the primary client which resolves the full host list (or cloud ID) via ElasticSearchSupport.createRestClient. I verified createRestClient adds no custom TLS trust store or timeout config beyond JVM defaults, so this doesn't introduce a new SSL/timeout failure mode — the only real gap is that a probe could fail to fail over if node 0 happens to be down during a multi-node cluster's write-probe pass. Low impact for a diagnostic endpoint.
  • Write side effects behind a GET — writeProbes=true creates and deletes real API keys/roles on the cluster, but is triggered via GET with a query parameter rather than POST. This breaks HTTP's "GET is safe" convention; since auth is bearer-token (not cookie-based per the documented curl -H "Authorization: Bearer ..." usage), CSRF risk is minimal, but a URL like .../capabilities?writeProbes=true could be accidentally re-triggered by link prefetching, monitoring crawlers, or anything that follows GET links if ever bookmarked/shared. Worth a passing comment, not a blocker — this is an admin-only, intentionally temporary diagnostic tool per the PR description.

Comment thread src/main/resources/assets/paths/elasticSearchCapabilities.yaml Outdated

@kevinmarete kevinmarete 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.

Looking good, just some comments from agent review:

It found two blocking correctness issues and one lower-priority privilege-handling issue that do not appear to be covered by the existing review comments.

[P1] Respect xpack.security.dls_fls.enabled

dlsFlsCapability() reads xpack.security.dls_fls.enabled, but does not use its value when determining the verdict. A cluster with a qualifying license and xpack.security.dls_fls.enabled=false will be reported as INFERRED_SUPPORTED, and the recommendation will incorrectly say Epic D is viable.

Please return UNAVAILABLE or INFERRED_UNAVAILABLE when this setting is explicitly disabled, and incorporate the setting into the recommendation logic.

[P1] Do not report inconclusive enforcement as supported

When the dataset index is empty or unreadable, no usable probe key exists, or the FLS response has no inspectable fields, enforcement is not tested. However, the code retains the role-acceptance verdict of SUPPORTED. The recommendation then states that the filters were “accepted and enforced.”

Role acceptance does not prove enforcement. These paths should remain UNKNOWN or INFERRED_SUPPORTED, and the recommendation should only claim enforcement after a completed end-to-end enforcement probe.

[P2] Align API-key privilege detection with the endpoint being called

The probe creates keys through POST /_security/api_key, but canMint checks manage_api_key || grant_api_key.

The create-key endpoint uses manage_own_api_key or manage_api_key; grant_api_key applies to the separate /api_key/grant endpoint. As written:

  • A credential with only manage_own_api_key is incorrectly reported as unable to create keys.
  • A credential with only grant_api_key is predicted to work, but this probe’s create-key request will be rejected.

Please align the privilege check and API documentation with the endpoint the probe actually uses.

@otchet-broad
otchet-broad requested a review from kevinmarete July 30, 2026 18:31

@kevinmarete kevinmarete 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.

The substantive review findings have been resolved—thank you. One stale instruction remains after moving write probes to POST: deploymentNotes() still tells callers to “Re-run with writeProbes=true,” and the PR description still documents the old GET query parameter.

Please update both to instruct callers to:

POST /api/elasticSearch/capabilities

@sonarqubecloud

Copy link
Copy Markdown

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