(Connectors) MongoDB connector - #279607
tara-elastic wants to merge 61 commits into
Conversation
# Conflicts: # packages/kbn-optimizer/src/worker/webpack.config.ts
🔍 Preview links for changed docs |
Elastic Docs Style Checker (Vale)Summary: 2 suggestions found 💡 Suggestions (2): Optional style improvements. Apply when helpful.
The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
# Conflicts: # src/platform/packages/shared/kbn-connector-specs/src/all_specs.ts # src/platform/packages/shared/kbn-connector-specs/src/connector_icons_map.ts
tara-elastic
changed the title
# Conflicts: # docs/reference/connectors-kibana/_snippets/data-context-sources-connectors-list.md # docs/reference/toc.yml # src/platform/packages/shared/kbn-connector-specs/src/connector_icons_map.ts
|
Dependency Review Bot Analysis 🔍 Found 2 new third-party dependencies:
Self ChecklistTo help with the review, please update the PR description to address the following points for each new third-party dependency listed above:
Thank you for providing this information! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the MongoDB connector spec. Two security findings on the read-only enforcement — both are cases where the agent-facing (isTool: true) surface can trigger server-side JavaScript execution despite the intended read-only guarantee. Details in the inline comments. The rest (bundle externals, docs, auth-source handling, per-call client lifecycle, disclosed SSRF gap) looks consistent with the PR's stated scope.
Generated by Claude Reviewer for #279607 · 26.1 AIC · ⌖ 1.12 AIC
…ally, or add an exception to src/dev/yarn_deduplicate/index.ts and then commit the changes and push to your branch
There was a problem hiding this comment.
Reviewed the MongoDB connector. The earlier security findings (JS-execution operators, unescaped nameFilter, $facet limit bypass, unbounded pipeline, embedded-URI credentials, server-only static import) all look addressed in the current revision, and the SRV allowlist/TOCTOU handling and test coverage are thorough. Two remaining concrete concerns on agent-facing inputs, noted inline.
Generated by Claude Reviewer for #279607 · opus · 226.3 AIC · ⌖ 14.3 AIC · ⊞ 5.1K
| skip: z | ||
| .number() | ||
| .int() | ||
| .min(0) |
There was a problem hiding this comment.
skip is an agent-facing (isTool: true via find) numeric input with no upper bound, unlike its sibling limit (bounded 1–1000). A large skip (e.g. 1e9) forces MongoDB to walk and discard that many documents server-side before returning results — the classic large-skip performance cliff — giving an LLM-supplied value a cheap server-side DoS vector. This is the same unbounded-agent-input class the repo guideline calls out (and that was applied to the pipeline array's .max(100)).
| .min(0) | |
| .min(0) | |
| .max(100_000) |
(bound value illustrative — pick whatever ceiling still supports realistic pagination.)
| return { ...stage, $facet: clampedFacet }; | ||
| } | ||
| if ('$lookup' in stage) { | ||
| const lookup = stage.$lookup as Record<string, unknown>; |
There was a problem hiding this comment.
The volume clamp bounds $lookup only in its pipeline form; the equality-join form (localField/foreignField, no pipeline) short-circuits here untouched. That form produces an unbounded as array per outer document, and the outer $limit only bounds the outer document count — so up to maxLimit documents can each carry an arbitrarily large joined array. This is the same nested data-volume gap the $facet/pipeline-form $lookup clamping was built to close, and it leaves the docs' claim ("none of them can be used to return more than the limit's worth of documents") not fully true for equality-form $lookup on an agent-facing read tool. Bounding it requires rewriting the equality form into pipeline form with an injected $limit, or documenting the residual gap.
| * — the same accessor HTTP-based connectors use — and must recover the raw username/password | ||
| * from the header to hand to their driver's binary protocol. | ||
| */ | ||
| export const parseBasicAuthHeader = ( |
There was a problem hiding this comment.
I have seen this a bunch of times now... we should probably extract it next time (it is also in my MySql PR)
There was a problem hiding this comment.
Can you change the MySQL client type to use this now?
# Conflicts: # docs/reference/toc.yml # yarn.lock
There was a problem hiding this comment.
Reviewed the MongoDB connector. The read-only enforcement (assertReadOnly recursion, clampPipelineLimit), URI/credential handling, and SRV allowlist pinning are solid and well-tested. One inline note about a residual allowlist-bypass path via the driver's replica-set/sharded-cluster topology discovery.
Generated by Claude Reviewer for #279607 · claude · opus · 220.8 AIC · ⌖ 22.9 AIC · ⊞ 5.5K
There was a problem hiding this comment.
Reviewed the MongoDB connector. The security surface is thorough and well-tested (host-allowlist enforcement including SRV resolution, DNS-rebinding pinning, recursive read-only operator rejection, and nested $limit clamping). One inline comment on a credential-leak inconsistency in the URI-parse path used by read handlers.
Generated by Claude Reviewer for #279607 · claude · opus · 173.8 AIC · ⌖ 23.4 AIC · ⊞ 5.5K
# Conflicts: # src/platform/packages/shared/kbn-connector-specs/src/lib/clients/client_type_spec.ts
|
@elasticmachine merge upstream |
# Conflicts: # .github/CODEOWNERS # docs/reference/connectors-kibana/_snippets/data-context-sources-connectors-list.md # docs/reference/toc.yml # packages/kbn-optimizer/src/worker/webpack.config.ts # packages/kbn-rspack-optimizer/src/config/externals.test.ts # packages/kbn-rspack-optimizer/src/config/externals.ts # src/platform/packages/shared/kbn-connector-specs/src/lib/clients/index.ts
💔 Build Failed
Failed CI StepsMetrics [docs]Page load bundle
Unknown metric groupsshared chunk count
shared chunks total size
total optimizer output size
History
|
9 participants
Summary
Adds a spec-based MongoDB connector to
@kbn/connector-specs, built on the client-registryframework (#280445, which closed kibana#275613).
Talks to MongoDB over the native driver (binary wire protocol, not HTTP) using a connection URI
plus basic auth (username/password stored as encrypted secrets).
The
mongodbclient type (lib/clients/mongodb_client_type.ts) is the first client typeregistered in
ClientRegistry/clientTypes— it owns connecting, host-allowlist enforcement, andcredential decoding via a pooled, leased client (
ctx.getClient('mongodb')). The connector'saction handlers (
specs/mongodb/mongodb.ts) just lease the client and run queries. Credentials aredecoded from the same
CredentialAccessor.getAuthHeaders()HTTP-based connectors use, via a newshared helper (
lib/clients/parse_basic_auth_header.ts) that recovers username/password from theAuthorization: Basicheader for non-HTTP client types.Actions:
isTool: true):find,aggregate,count,listCollectionsisTool: false, never exposed to agents):insertOne,updateOne,deleteOneaggregaterecursively rejects write/code-execution stages ($out,$merge,$function,$accumulator), including inside$facet,$lookup, and$unionWithsub-pipelines, so theread-only tool surface can't be used to mutate data.
Also touches
x-pack/platform/plugins/shared/actions: addsresolveSrvHosts(name, serviceName?)and
getTlsOptions(logger, verificationMode, sslOverrides)to theConnectorNetworkSettingsinterface (
client_type_spec.ts) and implements both increate_connector_network_settings.ts,using
node:dns/promisesand@kbn/actions-utils'sgetNodeSSLOptionsrespectively. Both have tolive in the actions plugin rather than
@kbn/connector-specsbecause that package is isomorphic(
shared-common) and can't import Node builtins or@kbn/actions-utils's Node-only proxy-agentcode, even dynamically:
mongodb+srv://connection strings carry only a single DNS seed name — the real target hostsare resolved via SRV records — so the client type has to resolve and allowlist-check those
targets too, not just the seed.
getTlsOptionslets the client type applyxpack.actions.ssl/customHostSettingsthe same waythe Axios connector path does, without pulling
getCustomAgents'shttps-proxy-agentdependencychain into the isomorphic package.
Docs added at
docs/reference/connectors-kibana/mongodb-action-type.md, wired into the TOC andthe third-party connectors snippet list.
Release note
Adds a MongoDB connector with
find,aggregate,count, andlistCollectionsavailable as Agent Builder tools.Third-party dependencies
Per the automated dependency review bot, addressing each point below for the 2 new dependencies:
mongodb(6.21.0) — official MongoDB Node.js driverstandalone) over the native binary wire protocol, from the connector's server-side action handlers.
connector infra (the injected Axios client) can't reach it. The official driver is the
standard, actively-maintained way to speak MongoDB CRUD/aggregation without reimplementing
wire-protocol, auth negotiation, and TLS handling in-house.
uniformly across Atlas and self-hosted deployments; hand-rolling the wire protocol was ruled
out as out of scope and a correctness/security risk.
capability, not a duplicate of something already vendored.
mongodb-connection-string-url(3.0.2) — MongoDB connection URI parsermongodb:///mongodb+srv://connection URI to extractthe default database name (from the URI path) and detect an existing
authSourcequeryparam, so the connector can support "database in the URI" as a fallback to the explicit
databaseaction input and avoid overriding a user-specifiedauthSource.mongodbitself (the driver usesit internally for identical parsing), so declaring it directly reuses the same vetted parser
the driver ships with rather than hand-rolling URI parsing.
URLclass was considered and rejected —mongodb+srv://is not a spec-compliant URL scheme thatURLhandles correctly (it involvesSRV record lookups and comma-separated host lists), so it would silently mishandle valid Atlas
connection strings.
mongodb; this change adds an explicit direct dependency since connector code imports it too.Both packages are excluded from browser bundles via
webpack.config.ts/externals.ts(
commonjsexternals) and are only ever loaded server-side through dynamicimport()insideaction handlers — see the "New native dependency" risk note below.
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.Identify risks
Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.
xpack.actions.allowedHostsviactx.networkSettings.ensureHostnameAllowed(). Formongodb+srv://URIs, the connector resolves the DNS SRV records itself and validates every resolved target host, not just the seed name in the URI (mongodb_client_type.ts,create_connector_network_settings.ts). Covered by tests inmongodb_client_type.test.tsandcreate_connector_network_settings.test.ts.mongodbnpm driver (and itsmongodb-connection-string-urldependency) to the workspace. Both are excluded from the browser bundle viawebpack.config.ts/externals.ts(commonjsexternals) and only loaded server-side via dynamicimport(). See the "Third-party dependencies" section above for purpose/justification detail.insertOne/updateOne/deleteOnecan mutate/delete data, but areisTool: falseand therefore never callable by agents. They also aren't reachable from anywhere yet:metadata.supportedFeatureIdsis['agentBuilder']only for this introducing PR (new connector types can't declare user-facing features like'workflows'until registered in every Production-NonCanary version, per the two-step release policy — seecreate-connector/SKILL.md). A follow-up PR adding'workflows'tosupportedFeatureIdsis required before these actions become reachable from a workflow.