[fix] Make the SSH missing host key policy configurable

[MichaelUray]

Problem

The SSH connector hardcodes paramiko.AutoAddPolicy, which accepts any unknown
device host key without verification. Since the configuration push carries
device secrets (WiFi passwords, VPN private keys, PSKs), a man-in-the-middle on
the management path could impersonate a device and capture them.

Fix

Add the OPENWISP_SSH_MISSING_HOST_KEY_POLICY setting (default
paramiko.AutoAddPolicy, preserving current behaviour) so operators can
enforce host key verification, e.g. by setting it to paramiko.RejectPolicy.
When a non-AutoAdd policy is configured, the system known hosts are loaded so
trusted device host keys can still be verified. Documented in
docs/user/settings.rst.

Testing

New tests cover the default policy and the configurable (Reject) policy loading
the known hosts. ./run-qa-checks and the SSH connector tests pass.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 67ded160-7ed9-4eb9-8823-225c822b80ba

📥 Commits

Reviewing files that changed from the base of the PR and between f21dad8 and 6b29b96.

📒 Files selected for processing (4)

• docs/user/settings.rst
• openwisp_controller/connection/connectors/ssh.py
• openwisp_controller/connection/settings.py
• openwisp_controller/connection/tests/test_ssh.py

📜 Recent review details

⏰ Context from checks skipped due to timeout. (10)

• GitHub Check: Python==3.12 | django~=5.2.0
• GitHub Check: Python==3.13 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=4.2.0
• GitHub Check: Python==3.11 | django~=4.2.0
• GitHub Check: Python==3.10 | django~=4.2.0
• GitHub Check: Python==3.10 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=5.1.0
• GitHub Check: Python==3.11 | django~=5.1.0
• GitHub Check: Python==3.11 | django~=5.2.0
• GitHub Check: Kilo Code Review

🧰 Additional context used

📓 Path-based instructions (2)

**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it

Files:

• openwisp_controller/connection/settings.py
• openwisp_controller/connection/tests/test_ssh.py
• openwisp_controller/connection/connectors/ssh.py

**/*.{py,html}

📄 CodeRabbit inference engine (Custom checks)

For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.

Files:

• openwisp_controller/connection/settings.py
• openwisp_controller/connection/tests/test_ssh.py
• openwisp_controller/connection/connectors/ssh.py

🧠 Learnings (8)

📚 Learning: 2026-01-15T15:05:49.557Z

Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.

Applied to files:

• openwisp_controller/connection/settings.py
• openwisp_controller/connection/tests/test_ssh.py
• openwisp_controller/connection/connectors/ssh.py

📚 Learning: 2026-02-17T19:13:10.088Z

Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.

Applied to files:

• openwisp_controller/connection/settings.py
• openwisp_controller/connection/tests/test_ssh.py
• openwisp_controller/connection/connectors/ssh.py

📚 Learning: 2026-01-15T15:07:17.354Z

Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.

Applied to files:

• openwisp_controller/connection/settings.py
• openwisp_controller/connection/tests/test_ssh.py
• openwisp_controller/connection/connectors/ssh.py

📚 Learning: 2026-06-07T12:07:08.468Z

Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_admin.py:2335-2335
Timestamp: 2026-06-07T12:07:08.468Z
Learning: In this project’s Python test suite (files under openwisp_controller/**/tests/), don’t require or request prose/inline comments that document the breakdown of query-count changes (e.g., assertions around template/DB query counts in helpers like _verify_template_queries). Treat query-count assertions as volatile implementation details that change frequently; review should focus on whether the test asserts the expected behavior, not on explaining the specific query-count deltas in comments.

Applied to files:

• openwisp_controller/connection/tests/test_ssh.py

📚 Learning: 2026-06-07T12:07:24.608Z

Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/pki/tests/test_api.py:155-155
Timestamp: 2026-06-07T12:07:24.608Z
Learning: When reviewing Python test files in this repository, avoid recommending inline comments that explain or justify `assertNumQueries` (Django query count) expectations. Query counts can change frequently as implementations evolve, and inline explanations add maintenance burden; the expected count should be understandable without added comment blocks.

Applied to files:

• openwisp_controller/connection/tests/test_ssh.py

📚 Learning: 2026-06-25T12:20:18.414Z

Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/base/models.py:571-572
Timestamp: 2026-06-25T12:20:18.414Z
Learning: When writing or reviewing tests that override pagination behavior via OpenWispPagination.paginate_queryset(), patch `view.pagination_page_size` (not `page_size`). The method uses `getattr(view, "pagination_page_size", self.page_size)`, so tests must set the attribute on the view to affect pagination. If the view class does not define `pagination_page_size`, using `unittest.mock.patch(..., create=True)` is intentional and correct because the attribute may not exist until patched.

Applied to files:

• openwisp_controller/connection/tests/test_ssh.py

📚 Learning: 2026-06-07T12:07:25.164Z

Learnt from: stktyagi
Repo: openwisp/openwisp-controller PR: 1378
File: openwisp_controller/config/tests/test_config.py:864-865
Timestamp: 2026-06-07T12:07:25.164Z
Learning: When reviewing this repo’s Python test suite, treat changes to the *expected* query count in `assertNumQueries(...)` calls as routine test maintenance. If a PR updates the numeric argument (e.g., in `test_config.py`, `test_api.py`, `test_admin.py`, `test_pki.py`) and the test remains consistent with the feature changes, reviewers should not flag the increased number as a performance regression that requires investigation solely because the count went up; instead, focus on whether the update is intentional and the surrounding test/code changes justify the revised expectation.

Applied to files:

• openwisp_controller/connection/tests/test_ssh.py

📚 Learning: 2026-06-25T12:20:45.387Z

Learnt from: dee077
Repo: openwisp/openwisp-controller PR: 1395
File: openwisp_controller/connection/tests/test_api.py:916-932
Timestamp: 2026-06-25T12:20:45.387Z
Learning: When reviewing API pagination behavior in openwisp-controller, assume `OpenWispPagination.paginate_queryset()` allows a per-view page-size override via `getattr(view, "pagination_page_size", self.page_size)` (so `view.pagination_page_size`, if present, should affect pagination). In Python tests, it is valid to patch `pagination_page_size` on a view class even if the attribute isn’t declared on the class by default, by using `unittest.mock.patch.object(..., "pagination_page_size", ..., create=True)` so the override is available for the pagination logic during the test.

Applied to files:

• openwisp_controller/connection/tests/test_ssh.py

🔇 Additional comments (6)

openwisp_controller/connection/settings.py (1)

38-42: LGTM!

openwisp_controller/connection/connectors/ssh.py (2)

8-8: LGTM!

---

65-78: LGTM!

docs/user/settings.rst (1)

6-25: LGTM!

openwisp_controller/connection/tests/test_ssh.py (2)

4-11: LGTM!

---

35-53: LGTM!

---

📝 Walkthrough

Walkthrough

Adds the OPENWISP_SSH_MISSING_HOST_KEY_POLICY setting with a Paramiko AutoAddPolicy default. The SSH connector dynamically resolves the configured policy, loads system host keys for stricter policies, and applies the resulting policy to the SSH client. Documentation describes the setting and host key verification behavior, while tests cover default and RejectPolicy configurations.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DjangoSettings
  participant SSHConnector
  participant ParamikoSSHClient
  DjangoSettings->>SSHConnector: provide configured host key policy
  SSHConnector->>SSHConnector: resolve policy class
  alt policy is not AutoAddPolicy
    SSHConnector->>ParamikoSSHClient: load system host keys
  end
  SSHConnector->>ParamikoSSHClient: apply policy instance

Loading

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Features	⚠️ Warning	Docs and tests were added, but I found no evidence of an existing issue or org-member validation/acceptance for this feature.	Link the feature to an existing issue and show it was validated/accepted by an org member (e.g., issue link, label, or project-board evidence).
General Rules	❓ Inconclusive	I can't verify the checklist’s external issue-label/project assignment from repo contents alone, though the code/doc/test changes look fine.	Provide the PR/issue link or GitHub metadata showing at least one valid label and assignment to project 37 or 42.

✅ Passed checks (6 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is descriptive, matches the SSH host key policy change, and uses the required [fix] prefix.
Description check	✅ Passed	It clearly covers the problem, fix, and testing, but omits the template checklist, issue reference, and screenshot section.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Bug Fixes	✅ Passed	PASS: The connector now uses the configured policy and loads system host keys for non-AutoAdd policies; the RejectPolicy unit test would fail without the patch and is deterministic.
Changes	✅ Passed	Docs were updated for the new SSH host-key setting, tests cover default and RejectPolicy behavior, and there are no UI changes or API breaks needing screenshots or migration.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Nice work on this security hardening. The change makes the SSH missing host key policy configurable while preserving the existing AutoAddPolicy default, so it's backwards compatible. System host keys are correctly loaded only when a stricter (non-AutoAdd) policy is configured, the new setting has a sensible default, the docs are updated, and both the default and the configurable (Reject) code paths are covered by regression tests.

Files Reviewed (4 files)

• docs/user/settings.rst
• openwisp_controller/connection/connectors/ssh.py
• openwisp_controller/connection/settings.py
• openwisp_controller/connection/tests/test_ssh.py

---

_{Reviewed by hy3:free · Input: 43.5K · Output: 3.1K · Cached: 131.8K}

[coveralls]

Coverage is 98.478% — MichaelUray:fix/ssh-configurable-host-key-policy into openwisp:master. No base build found for openwisp:master.

[CodingWithSaksham]

Hi @MichaelUray. Thanks for tackling this, its much appreciated. Before we proceed I have a few questions.

1. When someone turns on the stricter policy, it doesn't check trusted keys against anything OpenWISP tracks about its devices, it falls back to reading whatever host-key file already exists on the machine running the background workers. As far as I can tell, OpenWISP has no way to add a device's key to that file itself: no field for it on the connection/credential records, no admin screen, nothing. So in practice: an operator turns the setting on, and every device connection fails immediately, for every device, until someone manually edits a file on the server outside of OpenWISP entirely, and that'd have to be redone by hand every time a device is added or the server is rebuilt. That doesn't feel like real host-key verification yet, more like a way to accidentally break all SSH connections.
2. Would it make sense to store each device's key alongside its existing connection details in OpenWISP, so trust is tied to the device record it already manages, rather than relying on server state OpenWISP has no control over?
3. Is there an issue tracking this? Couldn't find one linked might be worth getting a maintainer's take on the approach, since this touches how device auth works and also adds an entirely new feature for maintainers to support. Due to this the maintainers can also flag this PR as "AI Slop" (see AI Spam policy here)