[TexasCoding]

Summary

KalshiAuth.from_pem now detects OpenSSH-format private keys (-----BEGIN OPENSSH PRIVATE KEY-----, the default ssh-keygen output since OpenSSH 7.8) and raises a targeted error pointing at the exact conversion command, instead of the generic "Invalid PEM" message that sent users hunting for PKCS8 issues in a non-PKCS8 file.

Issues closed

• Closes from_pem error for an OpenSSH-format key mentions only PKCS8 — wrong remediation #335 — from_pem error for an OpenSSH-format key mentions only PKCS8 — wrong remediation

Behavioral changes

• OpenSSH-format input to KalshiAuth.from_pem / from_key_path / from_env now raises KalshiAuthError with an OpenSSH-specific message and conversion command, before the generic load_pem_private_key path. PKCS8/PKCS1 handling and the existing passphrase paths are unchanged.

Tests

• tests/test_auth.py::TestFromPem::test_issue_335_from_pem_openssh_format_error_message — feeds an OpenSSH-format key and asserts the targeted message and ssh-keygen -p -m PKCS8 hint.

Source

Round-3 independent audit closure plan, wave W0.

[claude]

Code Review

Summary: This PR adds a targeted pre-flight check in KalshiAuth.from_pem to detect OpenSSH-format private keys and surface a specific, actionable error message instead of the generic "Invalid PEM" path. The change is small, surgical, and directly addresses issue #335.

---

Code Quality

Placement is correct. The new check sits after pem_data is normalized to bytes (the str branch runs first) and before load_pem_private_key, so the early return fires in exactly the right spot without touching any other path.

lstrip() is appropriate for handling leading whitespace/newlines that some tools emit before the PEM header.

startswith over in — using startswith on the stripped bytes is the right call here; in would be more susceptible to false positives if the header string appeared elsewhere in the payload.

---

Potential Issue: -N '' in the conversion command

The suggested command:

ssh-keygen -p -m PKCS8 -f <path> -N ''

-N '' sets an empty new passphrase, which silently strips the passphrase from an encrypted OpenSSH key. A user with a passphrase-protected key running this command verbatim will end up with an unencrypted key on disk — potentially a security surprise.

Consider either:

1. Dropping -N '' so ssh-keygen interactively prompts for a new passphrase (safest and handles both encrypted and unencrypted source keys), or
2. Adding a note like (omit -N '' to set a new passphrase instead of removing it).

# Suggested tweak
"ssh-keygen -p -m PKCS8 -f <path> "
"(you will be prompted for the current passphrase, if any, and a new one)"

---

Tests

• The test correctly uses a syntactically valid OpenSSH PEM header/footer with realistic base64 payload content.
• Both the error type (KalshiAuthError) and the message content ("OpenSSH private-key format", "ssh-keygen -p -m PKCS8") are asserted — good coverage for the contract.
• One minor suggestion: add a check that from_key_path and from_env also surface the same targeted error (they delegate to from_pem, so they would, but an explicit regression guard would make that explicit).

---

No Issues Found

• No performance concern: .lstrip() + .startswith() on a ~1–4 KB key file is negligible.
• No security concern: the check is read-only; it does not alter the key or expose any data.
• PKCS8/PKCS1 and passphrase paths are completely unaffected.

---

Verdict

Approve with one blocking-ish nit: the -N '' in the hint can silently strip a passphrase on encrypted OpenSSH keys. Everything else is correct and well-tested. Fixing the hint copy (or adding a clarifying note) would make this ready to merge.

[TexasCoding]

Round-2 review addressed in af06a0f:

• Dropped -N '' from the OpenSSH conversion hint so ssh-keygen prompts interactively for the new passphrase, avoiding a silent passphrase strip on encrypted source keys. Hint now reads ssh-keygen -p -m PKCS8 -f <path> followed by a parenthetical noting the interactive prompt. Regression test updated to assert the new substring and that -N '' is absent.

Deferred:

• Explicit regression guards for from_key_path / from_env against OpenSSH input — they delegate to from_pem, so the contract is already covered by the existing test. Happy to add thin wrappers in a follow-up if you'd like belt-and-suspenders coverage.

[claude]

Code Review

Overview

Targeted, well-scoped fix for #335. Adds early OpenSSH-format detection in from_pem, surfacing an actionable conversion command instead of the generic "Invalid PEM" message. The change is small, low-risk, and solves a real user-pain point.

---

Correctness

Logic is sound. After the str → bytes conversion at line 120–121, pem_data is always bytes, so the b"..." byte-literal comparison is correct. .lstrip() correctly handles leading whitespace before the PEM header.

One ordering issue worth fixing.

The OpenSSH check sits after the password handling block:

pw_bytes: bytes | None = None
if password is not None:
    raw = password() if callable(password) else password   # ← callable is invoked here
    pw_bytes = raw.encode("utf-8") if isinstance(raw, str) else raw
if pem_data.lstrip().startswith(b"-----BEGIN OPENSSH PRIVATE KEY-----"):  # ← raises here
    raise KalshiAuthError(...)

If password is a callable with side effects (e.g. prompting the user, decrypting a vault), it will be invoked before the OpenSSH error is raised. Moving the check to before the password block avoids the unnecessary invocation:

if pem_data.lstrip().startswith(b"-----BEGIN OPENSSH PRIVATE KEY-----"):
    raise KalshiAuthError(...)
pw_bytes: bytes | None = None
if password is not None:
    ...

This is a minor ergonomics issue, not a correctness bug—the error is still raised correctly—but worth fixing given the overall quality bar of the file.

---

Error Message

Clear and actionable. Two concrete remediation paths are provided (in-place conversion and fresh key generation), which is the right balance. The ssh-keygen -p -m PKCS8 -f <path> invocation is correct.

---

Test Coverage

Solid. The test covers:

• The correct exception type (KalshiAuthError)
• The key phrase in the message ("OpenSSH private-key format")
• The exact command string ("ssh-keygen -p -m PKCS8 -f <path>")
• A negative assertion ("-N ''" not present)

One small nit: The assert "-N ''" not in msg assertion has no obvious context for a future reader—it looks like it's guarding against a regression from an earlier draft of the message. A short inline comment would make the intent clear:

assert "-N ''" not in msg  # dropped from earlier draft; empty-passphrase hint confused users

Coverage gap (non-blocking): from_key_path and from_env both call from_pem internally, so the error propagates correctly—but there's no explicit test for the file-path entry point with an OpenSSH key on disk. Not required given the call chain is trivial, but worth noting if test coverage metrics are tracked.

---

Summary

Correctness	✅ (minor: move OpenSSH check before password block)
Error message	✅ clear, actionable, two remediation paths
Tests	✅ (minor: add comment to -N '' assertion)
Scope	✅ surgical, no unrelated changes
Conventions	✅ matches existing error-raising patterns in the file

The callable-password ordering issue is the only thing worth addressing before merge. Everything else is minor polish.

[TexasCoding]

Round-3 review addressed in 07ef490:

• Hoisted OpenSSH-format check before password-callable invocation — avoids triggering vault/prompt side-effects when the key is OpenSSH-format.
• Added inline comment on the test negative assertion documenting the regression-guard intent.
  Deferring as non-blocking: explicit from_key_path/from_env regression guards — acknowledged covered by from_pem delegation contract.

[claude]

Code Review

Overview

This is a small, well-scoped UX fix: when from_pem receives an OpenSSH-format key (the ssh-keygen default since OpenSSH 7.8), it now raises a targeted error with the exact conversion command instead of falling through to the generic load_pem_private_key path and emitting a confusing "Invalid PEM" message.

---

Code Quality

kalshi/auth.py

The check is correctly placed — after str → bytes normalization but before load_pem_private_key, so both str and bytes callers hit it. lstrip() is the right choice here; PEM files occasionally have leading newlines/whitespace. The check is a simple prefix scan and raises immediately, which is exactly the right shape for a guard clause.

The error message is thorough and actionable. Two commands are offered (convert-in-place and generate-fresh), which covers the two realistic user situations.

No issues with the implementation itself.

---

Test Coverage

tests/test_auth.py

• Happy-path trigger, message substring, and the -N '' regression guard are all present. Good.
• The test uses real-looking (but truncated) base64 content, which avoids a fixture that only works if you look closely. Fine.
• The str-input path (from_pem("key", openssh_pem.decode())) is not tested. Since str→bytes happens one line above the new guard, it would be covered by the existing conversion logic, but a one-liner parametrize would make the contract explicit.

@pytest.mark.parametrize("pem", [openssh_pem, openssh_pem.decode()])
def test_issue_335_from_pem_openssh_format_error_message(self, pem):
    ...

Not a blocker, but worth considering.

---

Minor Issues

Inline comment references PR-internal history

# regression: dropped from earlier draft (would silently strip passphrase
# on encrypted OpenSSH keys)

Per CLAUDE.md: "Don't reference the current task, fix, or callers... since those belong in the PR description and rot as the codebase evolves." The assert itself is self-documenting. If you want to keep a hint for future readers, something like # -N '' would silently strip the passphrase — must not appear in the message states the invariant without referencing the PR history.

---

Summary

Area	Status
Correctness	✅
Conventions	✅ (one comment to clean up)
Test coverage	✅ (str-path parametrize optional)
Security	✅ (no passphrase-stripping flag in message)
Performance	✅ (one-time key load; negligible)

Solid fix. The two items above are polish-level — neither is a blocker. The implementation is correct and minimal. Good to merge once the comment is cleaned up (or left as a known deviation).

---

Reviewed by Claude (claude-sonnet-4-6) — kalshi-python-sdk PR #359

[TexasCoding]

Round-4 review addressed in 0b93cca:

• Rephrased -N '' assertion comment to state the invariant (no PR-internal history reference per CLAUDE.md).
  Deferring as non-blocking: str-input parametrize — bot said "Not a blocker"; existing str→bytes normalization above the guard covers the contract.

[claude]

Code Review

Overview

This PR adds a targeted early-exit check in KalshiAuth.from_pem for OpenSSH-format private keys (-----BEGIN OPENSSH PRIVATE KEY-----), replacing the confusing generic "Invalid PEM" error with an actionable message and a concrete conversion command. Fixes #335.

---

Code Quality & Style

kalshi/auth.py

• Placement is correct. The OpenSSH check fires before load_pem_private_key, which is the right spot — the cryptography library can't parse OpenSSH format at all, so failing fast with a clear message is better than letting it fall into a generic ValueError.
• pem_data.lstrip().startswith(...) correctly handles leading whitespace. No issues here; for PEM files this is negligible overhead.
• The error message is appropriately detailed — it names the bad format, explains what's required, and gives two concrete remediation paths. Consistent with the style of existing error messages in this file.

Minor nit — lstrip specificity: bytes.lstrip() with no argument strips all ASCII whitespace. Being explicit with pem_data.lstrip(b' \t\r\n') would make the intent clearer without changing behavior for any realistic PEM file, but this is stylistic — not a bug.

---

Tests

tests/test_auth.py::TestFromPem::test_issue_335_from_pem_openssh_format_error_message

• Feeds a syntactically valid-looking (but non-functional) OpenSSH PEM blob — good choice, avoids depending on a real key.
• Asserts the exception type, the "OpenSSH private-key format" string in the message, and the exact ssh-keygen -p -m PKCS8 -f <path> hint.
• The negative assertion assert "-N ''" not in msg is a smart guard: -N '' would silently strip passphrases on encrypted OpenSSH keys, which would be wrong advice. Worth keeping.

One style note: The test name encodes the issue number (test_issue_335_...). Per the project's CLAUDE.md conventions, names shouldn't reference issue numbers or task context since that context rots. Consider renaming to test_from_pem_openssh_format_error_message. Not a blocker.

---

Behavioral Coverage

The fix propagates naturally to from_key_path and from_env since both ultimately call from_pem. No additional changes are needed there.

Existing PKCS8/PKCS1 key handling, passphrase paths, and the RSA-type check are all untouched — confirmed by the diff.

---

Security

No concerns. The suggested ssh-keygen -p -m PKCS8 -f <path> command is correct and appropriate — it converts the key format in-place without silently discarding passphrases.

---

Summary

Clean, minimal, well-tested fix. The only actionable suggestion is renaming the test method to drop the issue number from its name. Everything else is solid.

Verdict: Approve with minor nit on the test name.