CXP-963: accept literal \n escapes in GitHub App private key PEM - #185
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: CXP-963: accept literal \n escapes in GitHub App private key PEMBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryScanned the full PR diff ( Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
2ad0352 to
c58c68a
Compare
luisina-santos
changed the title
A PEM escaped from a CRLF-origin (Windows) file into a single line produces \r\n rather than \n between lines. loadPrivateKeyFromString only unescaped \n, so the leftover \r broke pem.Decode. Use a strings.Replacer that maps \r\n and \n to a real newline and drops any remaining \r. Addresses a bot review suggestion on PR #185.
| } | ||
|
|
||
| func loadPrivateKeyFromString(p string) (*rsa.PrivateKey, error) { | ||
| p = strings.ReplaceAll(p, `\n`, "\n") |
There was a problem hiding this comment.
[Connector] Two more paste shapes still fail, including the one the PR body targets
Measured with this function copied verbatim against a real 2048-bit RSA key. Beyond the \r\n case already raised above, these still return invalid private key PEM format:
| shape | result |
|---|---|
\n-escaped, as the docs instruct |
passes |
\n-escaped, one trailing space |
passes |
\n-escaped, one leading space |
fails |
| newlines stripped, all one line | fails |
| newlines replaced by spaces | fails |
A trailing space is harmless but a leading one is fatal, because pem.Decode only accepts the BEGIN armor at offset 0 or immediately after a newline. The last two are what a browser produces when a multi-line PEM is pasted straight into a single-line <input> — which is the case the PR body describes as fixed ("a pasted PEM always failed ... now parses").
Folding these into the \r handling already suggested, one normalization covers all of them:
var pemArmorHead = regexp.MustCompile(`(-----BEGIN [A-Z0-9 ]+-----)[ \t]*`)
var pemArmorTail = regexp.MustCompile(`[ \t]*(-----END [A-Z0-9 ]+-----)`)
func loadPrivateKeyFromString(p string) (*rsa.PrivateKey, error) {
// The C1 config form submits this field on a single line, so the PEM arrives with
// its line breaks escaped, flattened to spaces, or stripped out entirely.
p = strings.NewReplacer(`\r\n`, "\n", `\n`, "\n", `\r`, "\n").Replace(p)
p = strings.TrimSpace(p)
p = pemArmorHead.ReplaceAllString(p, "$1\n")
p = pemArmorTail.ReplaceAllString(p, "\n$1")
block, _ := pem.Decode([]byte(p))This passes all eight shapes with no regression on real newlines, real CRLF, or the file-upload path — app-privatekey-path is a FileUploadField carrying real PEM bytes, and since neither base64 nor the armor can contain a backslash the replacer is a no-op there.
One consequence worth deciding rather than inheriting: with this in place, the instruction at docs/connector.mdx:297 to replace every line break with a literal \n becomes optional instead of required, since pasting the PEM as-is would work.
Non-blocking.
There was a problem hiding this comment.
Re-measured against 38a48296, which lands the \r handling. Three shapes still fail:
| shape | result |
|---|---|
real newlines · real CRLF · \n-escaped |
pass |
\r\n-escaped |
pass — fixed by this commit |
\n-escaped, one leading space |
fails |
| newlines stripped, all one line | fails |
| newlines replaced by spaces | fails |
A trailing space is harmless; a leading one is fatal, because pem.Decode only accepts the BEGIN armor at offset 0 or immediately after a newline. The last two are what a browser produces when a multi-line PEM is pasted into a single-line <input> — the case the PR body describes as fixed.
Extending what you already have covers all three:
var pemArmorHead = regexp.MustCompile(`(-----BEGIN [A-Z0-9 ]+-----)[ \t]*`)
var pemArmorTail = regexp.MustCompile(`[ \t]*(-----END [A-Z0-9 ]+-----)`)
p = escapedLineBreaks.Replace(p)
p = strings.TrimSpace(p)
p = pemArmorHead.ReplaceAllString(p, "$1\n")
p = pemArmorTail.ReplaceAllString(p, "\n$1")Measured: that passes every shape above, with no regression on real newlines, real CRLF, or the file-upload path — app-privatekey-path carries real PEM bytes, and neither base64 nor the armor can contain a backslash, so the replacer is a no-op there.
One consequence worth deciding rather than inheriting: with the armor handling in, the instruction at docs/connector.mdx:297 to replace every line break with a literal \n becomes optional instead of required, since pasting the PEM as-is would work.
Still non-blocking — fine to ship the \r fix now and take this as a follow-up.
There was a problem hiding this comment.
Landed the full normalization from your later re-measured suggestion in aaf07ab: escapedLineBreaks + TrimSpace + pemArmorHead/pemArmorTail regexes. All 9 shapes (including leading-space and newlines-stripped/space-joined) now covered by a table-driven test in app_privatekey_test.go.
There was a problem hiding this comment.
Scoped this back down — see 2deac91. Kept the \r\n/\r unescaping (same documented -escaping contract, just covering the CRLF/CR-origin case), but dropped the armor-repositioning regex (TrimSpace + forcing BEGIN/END onto their own line). That piece was recovering from not following the documented escaping instructions at all — unverified browser paste behavior rather than a line-break-convention gap — so leaving it as a follow-up if it turns out to matter in practice, per your note that it's fine to defer. Reopening this thread since the code no longer matches what I said was fixed.
There was a problem hiding this comment.
Verified the paste behavior, since that was the open question.
Chromium 150, pasting a multi-line PEM into the live app-privatekey field with Ctrl+V from the system clipboard: the value arrives on one line with each newline replaced by a space. Character count is preserved (155 in, 155 out), and the joins read ----- AAAAB3Nza… after the BEGIN armor and …1111 -----END before the END. So the space-joined shape is what an operator gets from copy-pasting the .pem — not from ignoring the escaping instructions.
Correcting one thing in my earlier comment: "newlines stripped, all one line" is what a programmatic value set produces (the input's value-sanitization path, 155 → 152 chars), not a real paste. The space-joined shape is the real one.
Why the armor regexes fixed it: Go's base64 decoder tolerates spaces inside the body, so the only thing the pasted shape is actually missing is a newline after BEGIN and before END. TrimSpace is independent of that — a single leading space is an ordinary paste artifact and is fatal on its own, because pem.Decode only accepts the BEGIN armor at offset 0 or immediately after a newline.
Scope note: measured on Chromium only; other engines may sanitize pasted line breaks differently, and I have not checked them.
Still your call whether this clears the bar for this PR or stays a follow-up — the deferral was reasonable on the evidence available when you made it.
Beyond \n and \r\n escapes, a browser can flatten a pasted multi-line PEM by stripping newlines entirely or replacing them with spaces, and a leading space before the BEGIN armor breaks pem.Decode (it only recognizes the armor at offset 0 or right after a newline). Normalize all escaped line-break forms to a real newline, trim surrounding whitespace, and force the BEGIN/END armor onto its own line before decoding. Table-driven test now covers all nine input shapes. Addresses carolinaroncaglia's review comment on PR #185.
The app-privatekey field renders as a single-line input in the C1 config form, which cannot hold real newlines, so a pasted PEM always failed to parse and the connector could not be configured for GitHub App auth from the form. Unescape literal \n sequences before decoding, mirroring baton-okta's parseRSAPrivateKey. Fixes CXP-963.
The C1 config form's GitHub App private key (PEM) field only accepts a single line, so operators need to know to substitute literal \n for line breaks. README.md's flag help text and the Cloud-hosted setup steps in docs/connector.mdx now cover this alongside the existing file-upload option.
…ine limit golangci-lint's revive line-length-limit flagged the description added in the previous commit. Reworded and wrapped it across two source lines; regenerated config_schema.json and README.md to match.
A PEM escaped from a CRLF-origin (Windows) file into a single line produces \r\n rather than \n between lines. loadPrivateKeyFromString only unescaped \n, so the leftover \r broke pem.Decode. Use a strings.Replacer that maps \r\n and \n to a real newline and drops any remaining \r. Addresses a bot review suggestion on PR #185.
Beyond \n and \r\n escapes, a browser can flatten a pasted multi-line PEM by stripping newlines entirely or replacing them with spaces, and a leading space before the BEGIN armor breaks pem.Decode (it only recognizes the armor at offset 0 or right after a newline). Normalize all escaped line-break forms to a real newline, trim surrounding whitespace, and force the BEGIN/END armor onto its own line before decoding. Table-driven test now covers all nine input shapes. Addresses carolinaroncaglia's review comment on PR #185.
aaf07ab to
62eb2f4
Compare
| realNewlines := string(pemBytes) | ||
|
|
||
| shapes := map[string]string{ | ||
| "real newlines": realNewlines, |
There was a problem hiding this comment.
🟡 Suggestion: This map entry is one space short of gofmt alignment — its value starts at column 46 while every other entry (including the longest key on line 61) aligns at column 47. gofmt -w fixes it; worth doing so the file doesn't trip a formatting gate in verify.
| "real newlines": realNewlines, | |
| "real newlines": realNewlines, |
The armor-repositioning regexes (TrimSpace + BEGIN/END on their own line) went beyond the ticket's scope: they recover from a user not following the documented \n-escaping instructions at all (raw paste mangled by the browser), rather than from a different line-break convention. That's speculative browser behavior, not verified against the actual C1 form, and adds meaningful parsing surface for an unconfirmed edge case. Keep the proportionate fix: unescape \r\n, \n, and \r to a real newline, matching the documented "replace line breaks with \n" contract across LF- and CRLF-origin keys.
| shapes := map[string]string{ | ||
| "real newlines": realNewlines, | ||
| "real CRLF": strings.ReplaceAll(realNewlines, "\n", "\r\n"), | ||
| "backslash-n escaped, as the docs instruct": strings.ReplaceAll(realNewlines, "\n", `\n`), | ||
| "backslash-n escaped, trailing space": strings.ReplaceAll(realNewlines, "\n", `\n`) + " ", | ||
| "backslash-r-backslash-n escaped (CRLF file)": strings.ReplaceAll(realNewlines, "\n", `\r\n`), | ||
| "backslash-r escaped (CR-only file)": strings.ReplaceAll(realNewlines, "\n", `\r`), | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: every shape here is built from a PKCS#1 RSA PRIVATE KEY block, so loadPrivateKeyFromString's PKCS#8 branch — including the key.(*rsa.PrivateKey) type assertion and the "not an RSA private key" path — stays untested even though the existing TestAppPrivateKeyPEM fixtures use PRIVATE KEY armor. Adding one pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: x509.MarshalPKCS8PrivateKey(key)}) shape would cover both armor types the function accepts. Low confidence that this matters in practice (GitHub App keys are PKCS#1), so purely a coverage nit.
3 participants
Summary
app-privatekeyfield renders as a single-line input in the C1 config form, which cannot hold real newlines, so a PEM pasted there always failed withinvalid private key PEM formatbefore any request to GitHub.loadPrivateKeyFromStringnow unescapes literal\nsequences before callingpem.Decode, mirroringbaton-okta'sparseRSAPrivateKey(pkg/oktaauth/oktaauth.go), so a PEM with\nin place of real newlines — the shape a single-line form field can actually submit — now parses.\n-escaped shape, and regeneratedconfig_schema.json.Fixes CXP-963.
Test plan
go build ./cmd/baton-*go test ./...(66 passed)TestLoadPrivateKeyFromString: parses a PEM with real newlines, parses the same PEM with\nescapes substituted (the config-form shape), errors on garbage inputTestAppPrivateKeyPEMstill passes unchanged🤖 Generated with Claude Code