fix(credentials): remove inline SSH credential read-fallback (#748, ARC-4)#873
Merged
Conversation
…RC-4) Node/group SSH secrets historically lived in inline ssh_* columns. The credential-consolidation epic (#704) moved them into the first-class Credential store (credential_id); migration 055 copied them and 058 NULLed them, but the service layer still READ the inline columns as a fallback, leaving two secret-resolution paths. This closes the ARC-4 security goal by eliminating the inline read path: - credential_resolver: drop the inline node/group branches and the inline helper functions. Resolution now flows solely through the Credential store plus the controller/global platform tiers — there is exactly one place a per-row secret can come from. Add NoUsableCredentialError and require_usable_node_credentials(/_sync) so credential-required callers fail loudly instead of silently degrading (the old inline-fallback behaviour). - ssh_credential_link.owner_secret_flags: no longer consults the inline columns; reports secret presence from the linked Credential only. The inline_* kwargs are retained (accepted-but-ignored, deprecated) so sibling- owned api/routes callers keep type-checking during the staged removal. node_import and bootstrap_svc already accept inline creds on import and immediately convert them into a Credential link, so import ergonomics are preserved; only the persisted dual-path READ is removed. tool_calling does not touch SSH credentials. Adds tests/unit/test_748_no_inline_ssh_fallback.py and updates the resolver tests to the credential-store-only contract. Deferred (precondition "once no code reads them" not yet met): the physical DROP of the inline columns (model removal + migration 061) is intentionally NOT included. workers/ansible_tasks.py and api/routes/{groups,nodes}.py — owned by sibling PRs — still read the inline columns, so dropping them now would break those modules and the repo-wide type gate. The columns stay on the model (unread by the service layer) until those readers are removed; the drop is the natural follow-up migration once they land.
Coverage Report✅ 86.1% on Gate passed |
…ssh-cred-columns # Conflicts: # VERSION # frontend/package.json
Coverage Report✅ 86.0% on Gate passed |
Coverage Report✅ 86.0% on Gate passed |
Coverage Report✅ ✅ Gate passed |
Coverage Report✅ ✅ Gate passed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #748
Summary
ARC-4: eliminate the second SSH-secret resolution path. Node/group SSH secrets historically lived in inline
ssh_*columns; the credential-consolidation epic (#704) moved them into the first-classCredentialstore (credential_id), and migrations 055/058 copied + NULLed the inline columns. But the service layer still read the inline columns as a fallback, leaving two secret-resolution paths. This PR removes that read path.services/credential_resolver.py— drop the inline node/group branches and the_inline_node_creds/_inline_group_credshelpers. Resolution now flows solely through the Credential store (node/groupcredential_id) plus the controller/global platform tiers._primary_group_stmtis eligible bycredential_idonly. AddedNoUsableCredentialError+require_usable_node_credentials(/_sync)so credential-required callers (WebSSH, VNC, password-mode bootstrap) fail loudly instead of silently degrading — the explicit replacement for the old inline fallback.services/ssh_credential_link.py—owner_secret_flagsno longer consults the inline columns; secret presence comes from the linkedCredentialonly.services/node_import.py,tool_calling.py,bootstrap_svc.py— audited:node_importonly parses import input (andbootstrap_svcconverts inline input into aCredentiallink viaupsert_owner_ssh_credential), so import ergonomics are preserved and nothing persists/reads an inline dual path.tool_callingdoes not touch SSH creds. No changes required.New failure mode (security-relevant)
Previously, a node with no
credential_idbut legacy inline data resolved via the inline columns. Now the inline columns are never read, so such a node resolves to no usable secret. Call sites that require a secret should userequire_usable_node_credentials(/_sync), which raisesNoUsableCredentialErrorwith a clear, actionable message rather than attempting a connection with the wrong/empty secret.Migration note (head 060 → 061): DEFERRED
The task asked for migration
061to DROP the inline columns "once no code reads them". That precondition is not yet met:workers/ansible_tasks.pyandapi/routes/{groups,nodes}.py(owned by sibling PRs, out of this PR's scope) still read the inline columns. Dropping them now would break those modules at runtime and fail the repo-widemypygate on files this PR must not touch.So this PR removes the service-layer read path (the ARC-4 security goal) and defers the physical column drop:
models/node.py/models/group.py(now unread by the service layer, annotated as deprecated).owner_secret_flagskeepsinline_password_enc/inline_key_enckwargs (accepted-but-ignored) so sibling-owned route callers keep type-checking during the staged removal.nodes/groupsin migration061(down_revision = "060", current verified head), remove the SQLAlchemy column defs, and delete the deprecatedowner_secret_flagskwargs.Backward-compat caveats
require_usable_node_credentialsis additive; existing callers ofresolve_node_credentials(/_sync)are unaffected (signatures unchanged, dict shape unchanged,credential_sourceunchanged).credential_id) will now resolve to the group/controller/global tier instead of inline — expected, since the store is authoritative after 055/058.Test plan
uv run pytest tests/unit/test_748_no_inline_ssh_fallback.py -q— new, 12 tests, all pass.test_credential_resolver.py,test_credential_resolver_sync.py,test_playbook_credentials_b279.pyto the credential-store-only contract — pass.uv run python -c "import fleet_platform.api.main"— smoke import OK.tests/unit/test_migration_chain_guard_571.py— pass (chain unchanged).Note: a handful of unrelated unit tests fail in the sandbox due to missing optional deps (
asyncssh,ansible_runner) — verified identical on baseorigin/master, so not introduced here.Made with Cursor