Skip to content

fix(tesla): create private keys atomically with owner-only permissions#74

Merged
Bre77 merged 7 commits into
mainfrom
fm/tfa-key-atomic-0600
Jul 12, 2026
Merged

fix(tesla): create private keys atomically with owner-only permissions#74
Bre77 merged 7 commits into
mainfrom
fm/tfa-key-atomic-0600

Conversation

@Bre77

@Bre77 Bre77 commented Jul 12, 2026

Copy link
Copy Markdown
Member

Intent

Harden private-key file creation so the PEM is born 0600 instead of written world-readable then chmod'd. get_private_key and get_rsa_private_key (tesla_fleet_api/tesla/tesla.py) previously wrote the PEM via aiofiles with the process default mode, then called chmod(path, 0o600) wrapped in except OSError: pass - leaving (a) a brief world-readable window on every fresh key and (b) a failed chmod silently leaving the key 0644 forever. Fix: both sites now create the file atomically with owner-only permissions via a custom aiofiles opener that calls os.open(path, O_WRONLY|O_CREAT|O_EXCL, 0o600) - no post-hoc chmod, no window, no swallowed failure. O_EXCL also closes the pre-existing TOCTOU race between the exists() check and the write: if a concurrent caller wins the create race (FileExistsError), the code deliberately falls back to reading that file instead of raising, matching the existing 'get or create' contract of these methods. This is a narrowly scoped security-hardening fix - no other behavior of get_private_key/get_rsa_private_key was changed (encoding, format, encryption_algorithm, and the load/read path for pre-existing keys are all unchanged). Added 6 new/updated tests in tests/test_tesla_private_key.py covering: fresh key file is 0600 immediately after creation (for both the EC and RSA key), existing-key read path is unchanged, and the concurrent-create race falls back to a read instead of raising (simulated by mocking exists() to return False while the target file already exists). This is a companion PR to a sibling fix (_log_request_result None-guard) on a different branch; both are intended to ship together in release 1.7.2, but this PR stands alone and does not depend on the other.

What Changed

  • Hardened EC and RSA private-key creation so new PEM files are opened atomically with owner-only permissions instead of being written first and chmodded afterward.
  • Added bounded retries for raced reads so concurrent callers can load the winning key once the creator finishes writing it.
  • Documented the 0600 private-key file requirement across the README and related docs, and expanded private-key tests around permissions and creation races.

Risk Assessment

✅ Low: The change is narrowly scoped to private-key file creation and raced reads, with the previously identified permission, portability, and partial-read concerns now addressed and covered by focused tests.

Testing

Exercised the key creation/read/race paths directly through Tesla.get_private_key and Tesla.get_rsa_private_key, captured mode/key-material evidence under /tmp/no-mistakes-evidence, then ran the focused private-key tests and the full test suite; all passed and the worktree remained clean.

Evidence: API-level private-key permission evidence
{
  "checks": [
    {
      "api": "Tesla.get_private_key",
      "operation": "fresh create",
      "path": "/tmp/tmpzbdjsgu3/private_key.pem",
      "mode": "0o600",
      "pem_header": "-----BEGIN EC PRIVATE KEY-----"
    },
    {
      "api": "Tesla.get_private_key",
      "operation": "existing read",
      "same_key_material": true,
      "mode_after_read": "0o600"
    },
    {
      "api": "Tesla.get_rsa_private_key",
      "operation": "fresh create",
      "path": "/tmp/tmpzbdjsgu3/tedapi_rsa_private.pem",
      "mode": "0o600",
      "pem_header": "-----BEGIN RSA PRIVATE KEY-----"
    },
    {
      "api": "Tesla.get_rsa_private_key",
      "operation": "existing read",
      "same_key_material": true,
      "mode_after_read": "0o600"
    },
    {
      "api": "Tesla.get_private_key",
      "operation": "simulated concurrent create loser fallback",
      "same_key_material": true,
      "mode_after_race": "0o600"
    },
    {
      "api": "Tesla.get_rsa_private_key",
      "operation": "simulated concurrent create loser fallback",
      "same_key_material": true,
      "mode_after_race": "0o600"
    }
  ],
  "result": "all API-level private key checks passed"
}

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 1 issue found → auto-fixed (4) ✅
  • ⚠️ tesla_fleet_api/tesla/tesla.py:22 - os.open(..., 0o600) is still filtered by the process umask, so a restrictive umask can create the key as 0400 or even 0000 instead of the previous final 0600. That can make a freshly created key unreadable on the next process run and breaks the documented exact-permission contract; create the fd with owner-only bits, then fchmod(fd, 0o600) before returning it so there is still no wider-permission window.

🔧 Fix: Force key permissions after atomic create
1 warning still open:

  • ⚠️ tesla_fleet_api/tesla/tesla.py:24 - Fresh key creation now calls os.fchmod() unconditionally, but this package is advertised as OS-independent and os.fchmod is Unix-only. On Windows this raises after os.open() has already created the file, leaks the fd, and leaves an empty PEM path that later reads fail to deserialize; guard the fd chmod with a portable fallback such as hasattr(os, "fchmod") plus path chmod where needed.

🔧 Fix: Make key opener portable without fchmod
1 warning still open:

  • ⚠️ tesla_fleet_api/tesla/tesla.py:71 - The FileExistsError fallback can still lose the real concurrent-create race: the winning process creates the file before it writes and closes the PEM, so a loser can hit this branch, immediately read an empty or partial file, and raise deserialization errors instead of satisfying the get-or-create contract. On this raced path, retry the read/deserialize for a short bounded window or otherwise wait until the creator has finished writing before loading the key.

🔧 Fix: Retry raced private-key reads
1 warning still open:

  • ⚠️ tesla_fleet_api/tesla/tesla.py:100 - A concurrent caller can still read the file while the winner has created it but before the PEM write has completed: in that timing exists(path) is already true, so this normal read path calls _load_pem_private_key(path) without retry_invalid=True and can raise a deserialization ValueError instead of satisfying the get-or-create contract. Apply the same bounded partial-write retry on the existing-file read path, or otherwise publish completed PEM contents atomically.

🔧 Fix: Retry existing key reads during creation race
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • uv run pytest tests/test_tesla_private_key.py
  • uv run python - <<'PY' > /tmp/no-mistakes-evidence/01KXAXPMK9CVW1QA0YKFJNAG0F/private_key_atomic_0600_evidence.json ...
  • uv run pytest tests
  • git status --short
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

firstmate crewmate added 7 commits July 12, 2026 20:23
get_private_key and get_rsa_private_key wrote the PEM with the process
default mode then chmod'd to 0o600 afterward, leaving a brief
world-readable window on every fresh key and silently leaving the key
0644 forever if the chmod failed. Both now open with O_EXCL at 0o600
via a custom aiofiles opener, so the file is born owner-only with no
window and no swallowed failure. O_EXCL also closes the TOCTOU race
between the exists() check and the write - a caller that loses the
create race falls back to reading the winner's file instead of raising.
@Bre77 Bre77 merged commit c618141 into main Jul 12, 2026
5 checks passed
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.

1 participant