Skip to content

feat(keyring): credential-keyring module (split from #181) - #188

Closed
raymondginger2018-sudo wants to merge 2 commits into
HKUDS:mainfrom
raymondginger2018-sudo:pr181/keyring
Closed

feat(keyring): credential-keyring module (split from #181)#188
raymondginger2018-sudo wants to merge 2 commits into
HKUDS:mainfrom
raymondginger2018-sudo:pr181/keyring

Conversation

@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor

Summary

Splits the keyring module out of #181 (maintainer suggestion: split the +3031-line PR so each module can merge independently). This PR is new files only — no existing behavior is modified.

GenAI course lesson 09: secrets must not live in plaintext config. core/keyring.py provides:

  • Keyring stores secrets out-of-band (env knob DEEPCODE_KEYRING, default off)
  • get / set / delete / has, secret-masked repr (never prints values)
  • Tests never touch the real keychain (DEEPCODE_KEYRING=mem in tests)

Tests

tests/test_keyring.py — 13 passed locally (also covered by CI).

@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor Author

@Zongwei9888 Per your 08-17 suggestion on #181, this is the keyring module split out as an independent PR: new files only (core/keyring.py + tests/test_keyring.py, 13 tests), zero changes to existing behavior while DEEPCODE_KEYRING is unset, and merge-order independent of the sibling splits (#189 classifier, #190 wiring).

CI status: Python CI / Desktop CI / Linting all green. The Security CI failure is repo-wide and unrelated to this PR — the sidecar lock pins pip==26.1.2, newly flagged by PYSEC-2026-3721; one-line fix proposed in #191 (verified locally with pip-audit 2.10.1).

Ready for review whenever you have a moment — this one and #189 are the two that can merge in any order.

GenAI course lesson 09: never put secrets in plaintext config. Keyring
stores secrets out-of-band (DEEPCODE_KEYRING env knob), get/set/delete,
secret-masked repr. All new files; no existing behavior touched.
@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor Author

Hi Zongwei, this is the keyring module split from #181. Pure new files, 13 tests, no behavior changes to existing code. Rebased onto latest main (e0767d0), all 14 CI checks green. Merge-order independent. Ready for review when you have a moment. Thanks!

@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor Author

Closing as cleanup after #200 — thanks for your patience. For the record: this is a clean new-module PR (no duplicate on upstream/main), but as it stands it has no runtime call site wired in (the module is not yet invoked by config resolution), which per the #200 feedback is the shape that will not be accepted on its own. Parking it rather than leaving it stale: the branch stays on my fork, and I can reopen with the call-site wiring once the current narrow PR (#204) lands. Happy to re-scope if you prefer.

@Zongwei9888

Copy link
Copy Markdown
Collaborator

Apologies for the late reply. We did not take this because DeepCode already has a hardened credential store — core/providers/credentials.py (0600, atomic replace, file locks, Windows ACL) — used by the profile resolver. A second, parallel keyring.json written with plain write_text (default umask, no lock) would be the less safe of the two, and since the keyring package is not a declared dependency, the system-keychain path never activates in a default install.

The shape that would be welcome: an optional keyring backend for CredentialStore (roughly twenty lines), so the existing resolution order (env → credential store → legacy config) gains keychain support without a second store.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants