fix(deps): bump cryptography to >=50.0.0 for VULN-106433 - #222
Conversation
Remediate PKCS#7 decrypt Bleichenbacher oracle (CVE-2026-69247) in pkcs7_decrypt_der/pem/smime for cryptography 44.0.0–49.x. Add explicit floor and refresh poetry.lock to resolve cryptography 50.0.0.
shuningc
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-5). It may contain mistakes.
Verdict: needs_discussion — The lock is internally consistent and does remediate the finding, but the change silently promotes cryptography (plus cffi/pycparser) from an extras-only transitive dependency to a mandatory base requirement of a published SDK, which constrains all downstream consumers — that scoping decision needs an explicit maintainer call.
General Comments
- 🟠 major (design): The floor is applied more broadly than the vulnerability requires, and it constrains every downstream consumer of the published SDK.
Before this PR, cryptography was reachable only through the crewai/all extras (via pdfminer-six, which requires cryptography>=36.0.0 — see poetry.lock:3370). The lock diff confirms the change in scope: optional = true → optional = false, and the markers = "... extra == \"crewai\" or extra == \"all\" or extra == \"openai\"" line is deleted. The cffi and pycparser markers were broadened for the same reason. Net effect: cryptography + cffi + pycparser — native/Rust-built wheels — are now installed for every pip install splunk-ao, including users who never opt into an extra. Nothing under src/ imports cryptography.
This differs from the existing floors in that block (filelock, idna, python-dotenv, requests). Those were already non-optional base packages in the lock, so pinning them added nothing to the install graph. This one adds three new packages to the base install.
Because splunk-ao is published to PyPI, a hard >=50.0.0 in Requires-Dist also propagates to consumers, and there are already conflicting pins reachable from this repo's own extras:
openai-agents[encrypt]requirescryptography (>=45.0,<46)(poetry.lock:2995) — andsplunk-ao[openai]installsopenai-agents.litellm[proxy]pinscryptography (==46.0.7)(poetry.lock:2290) — andsplunk-ao[crewai]installslitellm.
So pip install "splunk-ao[openai]" "openai-agents[encrypt]" becomes unresolvable after this change. Scoping the floor to where the dependency actually comes from avoids that:
cryptography = { version = ">=50.0.0", optional = true }...added to the crewai and all lists in [project.optional-dependencies]. That keeps the base install unchanged, still remediates the reachable path, and leaves consumers who don't use the PDF path unconstrained.
If the intent is instead "pin it for everyone so the scanner is unambiguously clean," that is a defensible call — but please state it in the PR description, since it is a change to the package's public dependency contract rather than a lockfile-only fix.
- 🟡 minor (question): Was a patch release on the 49.x line checked before forcing a major bump?
The advisory as described concerns pkcs7_decrypt_der / pkcs7_decrypt_pem / pkcs7_decrypt_smime. If upstream backported the fix to a 49.0.x patch, a floor of >=49.0.1 would remediate the same finding while leaving the resolved major version unchanged — materially less disruptive for the consumers noted in the other comment, and it would keep the lock's cffi/pycparser marker churn out of the diff.
Relatedly: neither this repo nor (as far as I can see from the lock) any dependency in the resolved graph calls the PKCS#7 decrypt APIs, so the practical exposure here is scanner compliance rather than a reachable code path. That is still worth fixing, but it argues for the least-disruptive floor rather than the highest one.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
pyproject.toml:61-65: This block now holds five transitive-only version floors (filelock,idna,python-dotenv,requests,cryptography) — none of which are imported insrc/— with no record of which advisory each one addresses or when it can be dropped. Consider consolidating them under a single commented# Transitive security floorsheading that names the advisory per entry, so they can be pruned once the upstream dependency's own floor catches up. Prevents indefinite accumulation of constraints on the published package's dependency contract.
| filelock = ">=3.20.1" | ||
| idna = ">=3.15,<4" | ||
| python-dotenv = ">=1.2.2" | ||
| cryptography = ">=50.0.0" |
There was a problem hiding this comment.
🟡 minor (documentation): This dependency is not imported anywhere in src/ — it exists purely to raise a transitive floor. Without a comment saying so, the next contributor to run a dependency cleanup (or a tool like deptry) will flag it as unused, remove it, and silently reintroduce the vulnerability. The file already uses inline comments for non-obvious dependency choices (e.g. time-machine on line 87).
Also worth deciding on an upper bound while you're here: >=50.0.0 unbounded means every future cryptography major is auto-accepted for all consumers. The neighbouring idna = ">=3.15,<4" caps its major; requests does not. Given this is a native dependency being newly promoted into the base install, capping the major would be the safer default.
| cryptography = ">=50.0.0" | |
| # Security floor only (VULN-106433 / CVE-2026-69247): not imported directly; pulled in | |
| # transitively via pdfminer-six. Do not remove as "unused". | |
| cryptography = ">=50.0.0,<51" |
🤖 Generated by the Astra agent
| filelock = ">=3.20.1" | ||
| idna = ">=3.15,<4" | ||
| python-dotenv = ">=1.2.2" | ||
| cryptography = ">=50.0.0" | ||
| requests = ">=2.33.0" |
There was a problem hiding this comment.
🔵 nit (other): The surrounding security-floor cluster is alphabetized (filelock, idna, python-dotenv, requests); inserting cryptography between python-dotenv and requests breaks that. Moving it above filelock keeps the block scannable.
| filelock = ">=3.20.1" | |
| idna = ">=3.15,<4" | |
| python-dotenv = ">=1.2.2" | |
| cryptography = ">=50.0.0" | |
| requests = ">=2.33.0" | |
| cryptography = ">=50.0.0" | |
| filelock = ">=3.20.1" | |
| idna = ">=3.15,<4" | |
| python-dotenv = ">=1.2.2" | |
| requests = ">=2.33.0" |
🤖 Generated by the Astra agent
Keep cryptography optional so base pip install splunk-ao is unchanged and openai-agents[encrypt] / litellm[proxy] pins stay resolvable. Floor only the crewai/all path where pdfminer-six pulls cryptography transitively.
shuningc
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-5). It may contain mistakes.
Verdict: request_changes — The python_version < '3.14' marker leaves the CVE unremediated on Python 3.14 (a supported, CI-tested version) where pdfminer-six still pulls cryptography unbounded, and the openai extra — which the lock file confirms also pulls cryptography — got no floor at all.
General Comments
- 🟠 major (security): The remediation is incomplete in two directions, and CI will not catch either.
The locked marker for cryptography (poetry.lock:872) is:
markers = "python_version <= \"3.13\" and (extra == \"crewai\" or extra == \"all\" or extra == \"openai\") or extra == \"openai\" or extra == \"all\" or extra == \"crewai\""
That is the full reachable set: crewai, all, and openai — on all supported Python versions (note the trailing unconditional or extra == ... clauses). This PR covers two of the three extras, and only below Python 3.14. Concretely:
- Python 3.14 +
[crewai]/[all]:cryptography (>=50.0.0)is gated behindpython_version < '3.14', butpdfminer-six— the very path the description names as the reachable one — is not python-gated (pyproject.toml:68). On 3.14 the extra resolves tomcp+pdfminer-six, andpdfminer-sixrequires onlycryptography >=36.0.0(poetry.lock:3371). A vulnerable version is installable. Python 3.14 is inrequires-python, in the classifiers, and in the CI matrix (.github/workflows/ci-tests.yaml:25). [openai]:mcpdepends onpyjwt[crypto](poetry.lock:2418), andpyjwt'scryptoextra pullscryptography >=3.4.0(poetry.lock:4212). Sopip install "splunk-ao[openai]"installscryptographywith no floor.
Because poetry.lock pins 50.0.0 for every extra/Python combination, poetry install --all-extras in CI will pass on 3.14 and the FOSSA scan against the lock will come back clean — the gap only manifests for downstream consumers resolving from the published Requires-Dist. The test-plan items as written (poetry lock reports 50.0.0, pip install "splunk-ao[crewai]" on the dev interpreter) would not detect it either. Worth adding a 3.14 leg to whichever check you use to verify the floor.
- 🟡 minor (question): The design note justifies avoiding a base-install floor by citing conflicts with
openai-agents[encrypt](cryptography>=45,<46, poetry.lock:2996) andlitellm[proxy](==46.0.7, poetry.lock:2291). Neither of those extras is enabled by this project, so neither participates in resolution here — which is whypoetry lockhappily produced 50.0.0. The argument only bites for a consumer who separately installsopenai-agents[encrypt]alongsidesplunk-ao.
That distinction matters because it is the same trade-off for the missing openai extra floor: adding cryptography (>=50.0.0) there would be genuinely unsatisfiable for anyone combining it with openai-agents[encrypt]. So there is a real decision to make rather than a mechanical fix — either accept that conflict and add the floor, or deliberately leave [openai] consumers exposed and say so in the description and the ticket. Could you state which one you intend? As written the PR reads as "CVE remediated" while one of the three reachable extras is untouched, which is the part I can't sign off on.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
pyproject.toml:28-33: There is no automated guard that the[project.optional-dependencies]entries stay in sync with[tool.poetry.dependencies], and no check that security floors hold across the full supported Python matrix. Sincepoetry.lockpins one resolution for every extra/interpreter combination, both CI and lock-based FOSSA scans are blind to marker-scoped gaps like the 3.14 one in this PR. A small job that runspip install --dry-run "splunk-ao[<extra>]"per extra per Python version and asserts the resolved floors would catch this class of regression at the point it is introduced.
| @@ -28,9 +28,9 @@ packages = [ | |||
| [project.optional-dependencies] | |||
| langchain = ["langchain-core", "langchain", "langsmith (>=0.8.0)"] | |||
| openai = ["openai (>=2.8.0,<3.0.0)", "packaging (>=24.2,<25.0)", "openai-agents (>=0.4.0,<1.0.0)", "mcp (>=1.27.2,<2)"] | |||
There was a problem hiding this comment.
🟠 major (security): The openai extra also pulls cryptography and did not get a floor. Chain: mcp → pyjwt[crypto] (poetry.lock:2418) → cryptography >=3.4.0 (poetry.lock:4212). The locked marker for cryptography names extra == "openai" explicitly (poetry.lock:872), so this is not hypothetical — pip install "splunk-ao[openai]" installs cryptography, currently with no lower bound.
Adding "cryptography (>=50.0.0)" here is the symmetric fix, but note the trade-off flagged in my PR-level comment: it becomes unsatisfiable for consumers who also install openai-agents[encrypt] (>=45,<46). If that is an acceptable cost, add it; if not, the description and ticket should record that [openai] is knowingly left exposed. Either way it shouldn't be silently omitted.
🤖 Generated by the Astra agent
| filelock = ">=3.20.1" | ||
| idna = ">=3.15,<4" | ||
| python-dotenv = ">=1.2.2" | ||
| cryptography = { version = ">=50.0.0", optional = true, python = ">=3.11,<3.14" } |
There was a problem hiding this comment.
🟡 minor (documentation): Still worth the inline comment the earlier review asked for: nothing under src/ imports cryptography, so a future dependency cleanup (or deptry) will read this as dead weight and remove it. The file already annotates non-obvious choices this way (see time-machine on line 87). Note the placement too — this sits in the middle of the alphabetized non-optional block (filelock, idna, python-dotenv, requests), while the other optional entries are grouped after requests (lines 66-68). It belongs with those.
Suggested wording, assuming the python gate is dropped per my other comment:
# Security floor only (VULN-106433 / CVE-2026-69247): not imported under src/;
# pulled in transitively via pdfminer-six and mcp -> pyjwt[crypto].
# Do not remove as "unused".
cryptography = { version = ">=50.0.0", optional = true }🤖 Generated by the Astra agent
Add cryptography>=50.0.0 to the openai extra (mcp/pyjwt[crypto] path) and remove the python_version<3.14 gate from the all extra so pdfminer-six on 3.14 cannot resolve a vulnerable transitive cryptography.
shuningc
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-sonnet-5). It may contain mistakes.
Verdict: approve — The security-relevant issues raised in earlier review rounds (Python 3.14 gate reintroducing the CVE, missing floor on the openai extra) have been fixed in the current diff; only minor documentation/organization gaps remain.
General Comments
- 🟡 minor (documentation): The PR description flags a real trade-off (an earlier review comment noted
cryptography>=50.0.0on theopenaiextra is unsatisfiable alongside a consumer's ownopenai-agents[encrypt]>=45,<46install) but the description/test-plan never mentions it. Worth a one-line note in the PR description or a code comment so a future "why can't I install X" report isn't a surprise.
Remove duplicate dependency keys introduced by an applied suggested change (filelock, idna, python-dotenv, requests, uv, mcp, pdfminer-six) and rerun poetry lock so CI can parse pyproject.toml again.
Summary
cryptography>=50.0.0as an optional Poetry dependency, scoped to the three extras that actually pull it transitively:crewai/all: viapdfminer-six(PDF parsing path)openai: viamcp→pyjwt[crypto]poetry.lockviapoetry update cryptography --lock; resolves cryptography 50.0.0 withoptional = true.Design note
This is intentionally not a base-install floor.
cryptographyis not imported undersrc/; pinning it in baseRequires-Distwould add native wheels to every consumer. The floor is limited to the extras where cryptography is reachable.The floor is not gated behind
python_version < 3.14. Python 3.14 is supported (requires-python, classifiers, CI matrix) andpdfminer-sixis not python-gated, so a 3.14-only gate would leave the CVE reachable on[crewai]/[all].Test plan
poetry lockreportscryptography50.0.0,optional = truepip install splunk-aodoes not installcryptographypip install "splunk-ao[crewai]"and"splunk-ao[openai]"resolvecryptography>=50.0.0from publishedRequires-Dist(not just the lock)