fix: fetch mise GPG key over HTTPS instead of dirmngr - #686
Conversation
gpg --recv-keys hangs indefinitely on some corporate networks because dirmngr's DNS resolver ignores timeouts. Fetch the key via curl so make setup stays bounded and still verifies the installer signature. Signed-off-by: Kendrick Boyd <kendrickb@nvidia.com>
WalkthroughThe mise installer now uses HTTPS-based release-key retrieval when ChangesMise installer verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🔵 Low · up to The PR changes how mise is installed and verified during bootstrap; it is mergeable with owner awareness that the signed-install CI path still needs a focused smoke test and the container’s GnuPG packages are unpinned, creating bounded integration and reproducibility risks. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
bug
Greptile SummaryUpdates mise bootstrap security and reliability:
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "fix: pin mise release-key fingerprint an..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8a6c12dc-f25d-46fc-b6bc-d1f0f69e53d4
📒 Files selected for processing (3)
CONTRIBUTING.mdMakefiletools/install-mise.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Greptile Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
CONTRIBUTING.md
**/*.{md,markdown}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown}: Bold is acceptable only in markdown tables where it's the conventional way to mark header-like cells in the body
Use##headers to segment markdown sections instead of bold text
Use--(em-dash) instead of-(hyphen) for asides in markdown
Files:
CONTRIBUTING.md
**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Do not use decorative bold in Markdown body text, list items, or docstrings; use single backticks for code identifiers, paths, and commands.
Write Google-style docstrings in
src/nemo_safe_synthesizer/so API reference pages can be generated automatically.
Files:
CONTRIBUTING.md
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Every source file requires the SPDX copyright and license header appropriate to its file format.
End files with a newline, remove trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
Files:
CONTRIBUTING.mdMakefiletools/install-mise.sh
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
CONTRIBUTING.mdMakefiletools/install-mise.sh
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files with
.py,.sh,.yaml,.yml, or.mdextensions require SPDX copyright headers, except files listed in.copyrightignore.
Files:
CONTRIBUTING.mdtools/install-mise.sh
Makefile
⚙️ CodeRabbit configuration file
Treat Makefile targets as the validation source of truth. Check that new targets are documented with target-name: ## Description, declared .PHONY where appropriate, and use uv/mise conventions.
Files:
Makefile
**/*.{sh,bash}
📄 CodeRabbit inference engine (AGENTS.md)
Never use
~inside double-quoted strings in shell scripts -- use$HOMEor an absolute path instead
Files:
tools/install-mise.sh
tools/**
⚙️ CodeRabbit configuration file
Review tools as developer and CI infrastructure. Check that scripts use uv or Makefile wrappers instead of ad hoc python/pip commands, preserve read-only behavior for check targets, fail with clear messages, avoid hidden network or filesystem side effects, and stay consistent with STYLE_GUIDE.md and CONTRIBUTING.md. Tooling may use print() when it is a standalone script or intentional CLI output.
Files:
tools/install-mise.sh
**/*.sh
⚙️ CodeRabbit configuration file
Review shell scripts for #!/usr/bin/env bash, set -euo pipefail where appropriate, quoting, repo root detection, and shellcheck compliance.
Files:
tools/install-mise.sh
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer
Timestamp: 2026-07-30T17:51:49.741Z
Learning: All commits merged to `main` must follow Conventional Commits syntax: `<type>(<scope>): <description>` or `<type>: <description>`, with a lowercase valid type and a description of at most 100 characters.
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer
Timestamp: 2026-07-30T17:51:49.741Z
Learning: All contributions must include a DCO `Signed-off-by` trailer, and commits must also have a verified cryptographic signature.
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer
Timestamp: 2026-07-30T17:51:49.741Z
Learning: Never move a published release tag; if code changes, create and validate the next release candidate instead.
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer
Timestamp: 2026-07-30T17:51:49.741Z
Learning: The stable release tag must point to the same tested commit SHA as the validated release candidate.
🔇 Additional comments (4)
tools/install-mise.sh (2)
30-37: LGTM!Also applies to: 76-83, 182-182
203-203: 🔒 Security & PrivacyNo issue: the key fetch is already fingerprint-pinned.
gpg_import_release_key()downloadshttps://keys.openpgp.org/vks/v1/by-fingerprint/${MISE_GPG_KEY}, sogpg --decryptis only trusting that imported release key, not an arbitrary key from the response.> Likely an incorrect or invalid review comment.CONTRIBUTING.md (1)
35-35: LGTM!Makefile (1)
14-14: LGTM!
curl_fetch already retries with --max-time; wrapping it again could stretch key retrieval for many minutes before unsigned fallback. Signed-off-by: Kendrick Boyd <kendrickb@nvidia.com>
zywind
left a comment
zywind
left a comment
There was a problem hiding this comment.
Review
Correct fix for the reported hang, well-commented, and a net simplification. One security gap worth closing before merge, plus a few cleanups.
I verified the endpoint: GET /vks/v1/by-fingerprint/24853EC9… → 200 application/pgp-keys, 3257 bytes. Dropping gpg-agent/dirmngr is sound — public-key import and detached-signature verification need neither.
Main issue: the imported key's fingerprint is never checked
gpg_import_release_key (tools/install-mise.sh:105) uses $MISE_GPG_KEY only to build the URL. It imports whatever bytes come back, and gpg --decrypt at line 198 then accepts a signature from any key in the ephemeral keyring. So a TLS-intercepting proxy — precisely the corporate-network class of environment this PR targets — can serve a substitute key and its own install.sh.sig, and the "signed" path passes silently.
--recv-keys <fpr> had the same weakness, so this isn't a regression, but the PR carries the security label and the fetch is now content-addressed by URL alone.
Cheapest fix — assert the fingerprint after import:
gpg_import_release_key() {
curl_fetch "${MISE_GPG_KEY_URL}/${MISE_GPG_KEY}" \
| gpg --batch --no-tty --import
# The URL is not a guarantee: verify the imported key is actually the
# one we pinned before trusting any signature it makes.
gpg --batch --no-tty --with-colons --list-keys "0x${MISE_GPG_KEY}" \
| grep -q "^fpr:::::::::${MISE_GPG_KEY}:"
}Stronger still: check VALIDSIG <fpr> from gpg --status-fd on the --decrypt call, which pins the signing key rather than merely a keyring member. Strongest and simplest: vendor the ASCII-armored key in-repo (tools/mise-release-key.asc) and drop the network fetch for the key entirely — the fingerprint is already hardcoded in three places, so the key material is no more of a maintenance burden, and it removes keys.openpgp.org from the critical path for both hangs and trust.
Other findings
MISE_REQUIRE_SIGNED_INSTALLis set nowhere in the repo. Neithercontainers/Dockerfile.test_ci:22nor.github/actions/setup-python-env/action.yml:118sets it, so CI silently degrades to unsignedcurl | shon any transient failure — with only a stderr warning. The script's own docstring recommends it "for CI/release pipelines." Worth setting while you're in the area.containers/Dockerfile.test_ci:7stillapt-get installsdirmngr gpg-agent. Now dead weight in the image and contradicts the PR's premise; removing them also exercises the new gpg-only path in CI, which covers the unchecked box in the test plan.curl --retrywriting into a pipe can emit partial bytes before retrying, sogpgmay see a truncated then re-sent stream. Impact is benign here (import/verify fails → fallback), but consider--retry-all-errorsand fetching to a temp file before piping if you want retries to be genuinely clean.- Stale wording left behind: line 21 (
keyserver/CDN flake) and line 86 (missing toolchain or keyserver/CDN) still reference the keyserver path this PR removes. Line 7's "when the local toolchain supports it" is now a bit overstated for a single-binary check. - Minor: adding
-H 'Accept: application/pgp-keys'makes the VKS request explicit about the format it expects.
Style, tests, docs
- Shell style matches the file's existing conventions;
set -euo pipefailcorrectly propagatescurlfailures through the pipe. The "don't wrap this in another retry loop" comment explaining--max-timereset semantics is exactly the kind of local-invariant note the style guide asks for. CONTRIBUTING.mdand theMakefilehelp text are updated consistently with the code — no stale references there.- No test coverage for
tools/install-mise.sh. Not introduced by this PR, but a small test asserting the fallback fires whengpgis absent, and thatMISE_REQUIRE_SIGNED_INSTALL=1hard-fails, would guard the branch logic this PR just rewrote.
Recommendation
LGTM after adding the fingerprint assertion. The Dockerfile cleanup, MISE_REQUIRE_SIGNED_INSTALL=1 in CI, and the stale-comment fixes are quick follow-ups that fit naturally in the same PR.
I had my agent doing further review and found some issues
binaryaaron
left a comment
binaryaaron
left a comment
There was a problem hiding this comment.
whoops, disregard above
Assert the imported VKS key matches MISE_GPG_KEY so a TLS MITM cannot substitute a signing key, fetch to files before gpg, require signed installs in CI, and drop unused dirmngr/gpg-agent from the test image. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Kendrick Boyd <kendrickb@nvidia.com>
|
Agent authored commit to address @zywind review comment. I read through the changes and they look good to me at the level of basic shell script knowledge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
containers/Dockerfile.test_ci (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPin the APT package versions.
The modified Dockerfile installs
gpgwithout a version. Rebuilds can therefore select different GnuPG packages. Pin the APT package set or use a reproducible Debian snapshot.As per path instructions, “Modified Dockerfiles should use ... reproducible pinned dependencies.”
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 19accb5f-5e25-47e8-be9c-a04809fc4bd1
📒 Files selected for processing (3)
.github/actions/setup-python-env/action.ymlcontainers/Dockerfile.test_citools/install-mise.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/install-mise.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.14)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: End-user Wheel Install
- GitHub Check: Greptile Review
- GitHub Check: Typecheck
- GitHub Check: Analyze (Python)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
.github/**
⚙️ CodeRabbit configuration file
Review GitHub configuration for branch protection expectations, CODEOWNERS alignment, least privilege permissions, pinned actions where practical, and consistency with CONTRIBUTING.md.
Files:
.github/actions/setup-python-env/action.yml
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: ReadAGENTS.local.mdif it exists and give its instructions top priority.
When a task matches a repository-specific skill, read the corresponding skill under.agents/skills/instead of duplicating its workflow instructions.
Do not commit unless the user explicitly asks for a commit or PR work.
When committing, require DCO sign-off and GPG signing usinggit commit --signoff --gpg-sign(or-s -S); never manually addSigned-off-byor use--no-gpg-sign.
Use feature branches based onmain; branch names commonly include an issue-number prefix such as<author>/123-short-name.
For recurring testing, building, syncing, bootstrapping, worktree, and GitHub workflows, use the matching skill under.agents/skills/.
For a full GPU/development environment, useuv sync --frozen --extra cu129 --extra engine --group dev; bareuv sync --frozenis incomplete and can causety, import-check, and GPU-test failures.End files with a newline, contain no trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
Files:
containers/Dockerfile.test_ci
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
containers/Dockerfile.test_ci
containers/Dockerfile*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Use multi-stage production builds, copy
uvfrom the Astral image, cache pip/uv and APT directories, setUV_LINK_MODE=copywith cache mounts, use--no-install-recommends, run as non-rootappuser, and provide correct PID 1 signal handling.
Files:
containers/Dockerfile.test_ci
containers/**
⚙️ CodeRabbit configuration file
Review container changes against STYLE_GUIDE.md and containers/README.md. Check cache mounts, uv usage, non-root runtime behavior, CUDA settings, and copy order.
Files:
containers/Dockerfile.test_ci
🪛 Betterleaks (1.7.3)
containers/Dockerfile.test_ci
[high] 23-23: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
containers/Dockerfile.test_ci (1)
22-26: LGTM!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
3 participants
Summary
gpg --recv-keys(dirmngr keyserver) with an HTTPS VKS fetch viacurl, somake setupno longer hangs forever on corporate DNS where dirmngr's resolver stalls and GnuPG 2.x ignores legacy keyserver timeouts.gpgfor the signed install path (no longergpg-agent+dirmngr); keep signature verification and the unsignedmise.runfallback.Makefile/CONTRIBUTING.mdnotes to match.Test plan
gpg --recv-keysagainstkeys.openpgp.orgon NVIDIA DNSinstall.sh.sigverify succeedstools/install-mise.shend-to-end (mise 2026.7.5 installed successfully)make install-miseis idempotent after installsetup-python-env/ container) still installs miseSummary by CodeRabbit
Documentation
gpgis available, without requiring additional GPG components.Bug Fixes
Chores