saml2-web-sso: stop failing signature verification on stdout/stderr noise#5311
saml2-web-sso: stop failing signature verification on stdout/stderr noise#5311blackheaven wants to merge 5 commits into
Conversation
…oise
verifyIO wrapped signature verification in hCapture [stdout, stderr] and threw
BadSamlResponseInvalidSignature ("noise on stdout/stderr from hsaml2
package") whenever any output appeared during the window. The capture is
process-global, so spar's own concurrent Debug logging in the same SSO request
(SCIM user creation, IdP validation, verdict handling) was swept in, causing
flaky false signature failures -- e.g. testSparExpiredIdpCertStillWorks.
Drop the capture' wrapper and the now-unused 'silently' dependency so verifyIO
relies solely on the Either SignatureError result. The signer already
self-verifies its own signature (DSig.hs signRootAt), so this does not weaken
actual signature checking.
zebot
added
the
ok-to-test
| capture' :: IO a -> IO a | ||
| capture' action = | ||
| hCapture [stdout, stderr] action >>= \case | ||
| ("", out) -> pure out | ||
| (noise, _) -> throwIO . ErrorCall $ "noise on stdout/stderr from hsaml2 package: " <> noise |
There was a problem hiding this comment.
It makes sense that this causes flakiness, because hCapture actually captures stdout/stderr from other threads too.
Example failure due to some logging which definitely prints in another thread:
(BadSamlResponseInvalidSignature "noise on stdout/stderr from hsaml2 package: {\"level\":\"Debug\",\"msgs\":[\"leaving verdictHandler: ResponseVerdict {unResponseVerdict = ServerError {errHTTPCode = 200, errReasonPhrase = \\\"not-found\\\", errBody = \\\"<?xml version=\\\\\\\"1.0\\\\\\\" encoding=\\\\\\\"UTF-8\\\\\\\"?><!DOCTYPE html PUBLIC \\\\\\\"-//W3C//DTD XHTML 1.1//EN\\\\\\\" \\\\\\\"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd\\\\\\\"><html xml:lang=\\\\\\\"en\\\\\\\" xmlns=\\\\\\\"http://www.w3.org/1999/xhtml\\\\\\\"><head> <title>wire:sso:error:not-found</title> <script type=\\\\\\\"text/javascript\\\\\\\"> const receiverOrigin = '*'; window.opener.postMessage({\\\\\\\"payload\\\\\\\":{\\\\\\\"errors\\\\\\\":[\\\\\\\"Could not find SAML credentials, and auto-provisioning is disabled.\\\\\\\"],\\\\\\\"label\\\\\\\":\\\\\\\"forbidden\\\\\\\"},\\\\\\\"type\\\\\\\":\\\\\\\"AUTH_ERROR\\\\\\\"}, receiverOrigin); </script></head></html>\\\", errHeaders = [(\\\"Content-Type\\\",\\\"text/html;charset=utf-8\\\")]}}\"]}\n{\"code\":\"200\",\"label\":\"unknown-error\",\"level\":\"Debug\",\"msgs\":[\"\\\"<?xml version=\\\"1.0\\\" encoding=\\\"UTF-8\\\"?><!DOCTYPE html PUBLIC \\\"-//W3C//DTD XHTML 1.1//EN\\\" \\\"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd\\\"><html xml:lang=\\\"en\\\" xmlns=\\\"http://www.w3.org/1999/xhtml\\\"><head> <title>wire:sso:error:not-found</title> <script type=\\\"text/javascript\\\"> const receiverOrigin = '*'; window.opener.postMessage({\\\"payload\\\":{\\\"errors\\\":[\\\"Could not find SAML credentials, and auto-provisioning is disabled.\\\"],\\\"label\\\":\\\"forbidden\\\"},\\\"type\\\":\\\"AUTH_ERROR\\\"}, receiverOrigin); </script></head></html>\\\"\"],\"request\":\"N/A\"}\n{\"level\":\"Info\",\"method\":\"POST\",\"msgs\":[\"generated a new request id for local request\"],\"path\":\"identity-providers\",\"request\":\"6c67c5e7-09ad-4e99-8751-2f9ac93af2a9\"}\n{\"level\":\"Debug\",\"msgs\":[\"granting sso login for cc79fbfa-0acd-4b5d-a085-a237261dd5d3\"]}\n{\"level\":\"Debug\",\"msgs\":[\"entering validateNewIdP\"]}",[Input {iName = "SAMLResponse", iValue = ".."}])
But removing it seems to remove capturing of errors from libxml, we should somehow limit this capture to only capture stdout/stderr limited to the IO action.
There was a problem hiding this comment.
@akshaymankar Thanks for flagging this — you're right that the capture wasn't entirely vacuous. I traced where libxml2 actually writes, and the writes that affect verification are already surfaced; the rest is non-fatal diagnostics. Details:
Where libxml2 writes to stderr (library core). Exactly one writer: xmlGenericErrorDefaultFunc does vfprintf((FILE*) xmlGenericErrorContext, …), with xmlGenericErrorContext defaulting to stderr (same function, error.c:236), wired as the global default in globals.c:233. Everything else (runsuite.c, runxmlconf.c, shell.c) is test/CLI code, not in our link path.
Which of our calls reach it.
xmlReadMemory(parser.c:13449) — yes; recoverable parse diagnostics go to stderr.xmlXPathEval(xpath.c:754, where the error path setschannel = xmlGenericError) — yes; invalid-expression diagnostics go to stderr.xmlC14NDocDumpMemory(c14n.c) — no stderr writes at all; it only signals failure by a negative return value.
Already covered. The hard failures from those same calls are caught by hsaml2's throwErrnoIf* wrappers in SAML2.XML.LibXML2 — throwErrnoIfNull "xmlReadMemory", throwErrnoIfNull "xmlXPathEval", throwErrnoIf (< 0) "xmlC14NDocDumpMemory". Those throw IO exceptions, caught by try in verify and catchAll in verifySignatureUnenvelopedSigs. So verification-affecting failures still propagate; the only thing the old hCapture added was catching non-fatal diagnostic output — and since hCapture redirects the process-global handles, it mostly swept up unrelated concurrent application logging, which is the flakiness this PR fixes.
If you'd prefer a real per-action capture (your "limit to the IO action" suggestion): libxml2 exposes exactly that via xmlSetGenericErrorFunc(void *ctx, xmlGenericErrorFunc handler) (error.c:272). The ctx is a void* that the default handler casts to a FILE* and vfprintfs to, so xmlSetGenericErrorFunc((void*) someHandle, NULL) redirects libxml2's output to that handle with no stderr involvement at all. The setting is thread-local, so under GHC we'd set it on a bound thread (forkOS) wrapping verifyIO, call into libxml, read the handle, restore. That yields true per-action isolation with zero process-global redirection (no cross-thread contamination). Happy to implement that if you'd rather have it than "remove + document"; otherwise I'll keep the removal with the explanatory note now in DSig.hs.
3 participants
Checklist
changelog.d