Skip to content

fix(deps): bump cryptography to >=50.0.0 for VULN-106433 - #222

Merged
shuningc merged 7 commits into
mainfrom
VULN-106433-cryptography-upgrade
Aug 12, 2026
Merged

fix(deps): bump cryptography to >=50.0.0 for VULN-106433#222
shuningc merged 7 commits into
mainfrom
VULN-106433-cryptography-upgrade

Conversation

@shuningc

@shuningc shuningc commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Remediate VULN-106433 / CVE-2026-69247 (PKCS#7 decrypt Bleichenbacher oracle; fixed in cryptography 50.0.0).
  • Add cryptography>=50.0.0 as an optional Poetry dependency, scoped to the three extras that actually pull it transitively:
    • crewai / all: via pdfminer-six (PDF parsing path)
    • openai: via mcppyjwt[crypto]
  • Refresh root poetry.lock via poetry update cryptography --lock; resolves cryptography 50.0.0 with optional = true.

Design note

This is intentionally not a base-install floor. cryptography is not imported under src/; pinning it in base Requires-Dist would 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) and pdfminer-six is not python-gated, so a 3.14-only gate would leave the CVE reachable on [crewai]/[all].

Test plan

  • CI passes (unit tests, lint, FOSSA)
  • poetry lock reports cryptography 50.0.0, optional = true
  • Base pip install splunk-ao does not install cryptography
  • On Python 3.14: pip install "splunk-ao[crewai]" and "splunk-ao[openai]" resolve cryptography>=50.0.0 from published Requires-Dist (not just the lock)
  • FOSSA no longer flags VULN-106433 on this branch

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 shuningc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤖 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 = trueoptional = 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] requires cryptography (>=45.0,<46) (poetry.lock:2995) — and splunk-ao[openai] installs openai-agents.
  • litellm[proxy] pins cryptography (==46.0.7) (poetry.lock:2290) — and splunk-ao[crewai] installs litellm.

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 in src/ — with no record of which advisory each one addresses or when it can be dropped. Consider consolidating them under a single commented # Transitive security floors heading 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.

Comment thread pyproject.toml Outdated
filelock = ">=3.20.1"
idna = ">=3.15,<4"
python-dotenv = ">=1.2.2"
cryptography = ">=50.0.0"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 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.

Suggested change
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

Comment thread pyproject.toml Outdated
Comment on lines 61 to 65
filelock = ">=3.20.1"
idna = ">=3.15,<4"
python-dotenv = ">=1.2.2"
cryptography = ">=50.0.0"
requests = ">=2.33.0"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔵 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.

Suggested change
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 shuningc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤖 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:

  1. Python 3.14 + [crewai]/[all]: cryptography (>=50.0.0) is gated behind python_version < '3.14', but pdfminer-six — the very path the description names as the reachable one — is not python-gated (pyproject.toml:68). On 3.14 the extra resolves to mcp + pdfminer-six, and pdfminer-six requires only cryptography >=36.0.0 (poetry.lock:3371). A vulnerable version is installable. Python 3.14 is in requires-python, in the classifiers, and in the CI matrix (.github/workflows/ci-tests.yaml:25).
  2. [openai]: mcp depends on pyjwt[crypto] (poetry.lock:2418), and pyjwt's crypto extra pulls cryptography >=3.4.0 (poetry.lock:4212). So pip install "splunk-ao[openai]" installs cryptography with 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) and litellm[proxy] (==46.0.7, poetry.lock:2291). Neither of those extras is enabled by this project, so neither participates in resolution here — which is why poetry lock happily produced 50.0.0. The argument only bites for a consumer who separately installs openai-agents[encrypt] alongside splunk-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. Since poetry.lock pins 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 runs pip 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.

Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
@@ -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)"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟠 major (security): The openai extra also pulls cryptography and did not get a floor. Chain: mcppyjwt[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

Comment thread pyproject.toml Outdated
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" }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 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 shuningc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤖 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.0 on the openai extra is unsatisfiable alongside a consumer's own openai-agents[encrypt]>=45,<46 install) 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.

Comment thread pyproject.toml
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.
@shuningc
shuningc merged commit 16d1600 into main Aug 12, 2026
13 checks passed
@shuningc
shuningc deleted the VULN-106433-cryptography-upgrade branch August 12, 2026 17:57
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 12, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants