Skip to content

saml2-web-sso: stop failing signature verification on stdout/stderr noise #5311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
blackheaven wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

saml2-web-sso: stop failing signature verification on stdout/stderr noise #5311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
88209f3
saml2-web-sso: stop failing signature verification on stdout/stderr n…
blackheaven Jul 2, 2026
d8c39e3
Hello CI
blackheaven Jul 2, 2026
9d28dc5
Hello CI
blackheaven Jul 3, 2026
15729fa
Hello CI
blackheaven Jul 3, 2026
e6bfa58
Hello CI
blackheaven Jul 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions libs/saml2-web-sso/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
, servant-multipart
, servant-server
, shelly
, silently
, string-conversions
, temporary
, text
Expand Down Expand Up @@ -137,7 +136,6 @@ mkDerivation {
servant-multipart
servant-server
shelly
silently
string-conversions
temporary
text
Expand Down Expand Up @@ -211,7 +209,6 @@ mkDerivation {
servant-multipart
servant-server
shelly
silently
string-conversions
temporary
text
Expand Down
2 changes: 0 additions & 2 deletions libs/saml2-web-sso/saml2-web-sso.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ library
, servant-multipart >=0.12
, servant-server >=0.16.2
, shelly >=1.8.1
, silently >=1.2.5.1
, string-conversions >=0.4.0.1
, temporary >=1.3
, text >=1.2.3.1
Expand Down Expand Up @@ -259,7 +258,6 @@ test-suite saml2-web-sso-tests
, servant-multipart >=0.12
, servant-server >=0.16.2
, shelly >=1.8.1
, silently >=1.2.5.1
, string-conversions >=0.4.0.1
, temporary >=1.3
, text >=1.2.3.1
Expand Down
19 changes: 5 additions & 14 deletions libs/saml2-web-sso/src/Text/XML/DSig.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,6 @@ import Network.Wai.Utilities.Exception
import SAML2.XML qualified as HS hiding (Node, URI)
import SAML2.XML.Canonical qualified as HS
import SAML2.XML.Signature qualified as HS
import System.IO (stderr, stdout)
import System.IO.Silently (hCapture)
import System.IO.Unsafe (unsafePerformIO)
import System.Random (mkStdGen, random)
import Text.XML as XML
Expand Down Expand Up @@ -236,11 +234,10 @@ mkSignCredsWithCertWithLifespan validSinceRaw validUntilRaw size = do
--
-- NB: The call to 'unsafePerformIO' in this function is sound under the assumption that
-- 'verifyIO' has no effects in 'IO' other than throwing 'SomeException' (which are captured
-- by 'try'. Technically, it does have other effects, like opening temp files for capturing
-- stderr (if any), but we do not care about those. The only thing we care about is that the
-- conceptually pure function of validating a signature will either be called twice with the
-- same arguments and return the same result value, or not be called a second time with the
-- same arguments, in which case that same value will be used.
-- by 'try'). The only thing we care about is that the conceptually pure function of
-- validating a signature will either be called twice with the same arguments and return the
-- same result value, or not be called a second time with the same arguments, in which case
-- that same value will be used.
{-# NOINLINE verify #-}
verify :: forall m. (MonadError String m) => NonEmpty SignCreds -> LBS -> String -> m HXTC.XmlTree
verify creds el sid = case unsafePerformIO (try @SomeException $ verifyIO creds el sid) of
Expand All @@ -266,17 +263,11 @@ verifyRoot creds el = do
-- | Try a list of creds against a document. If all fail, return a list of errors for each cert; if
-- *any* succeed, return the empty list.
verifyIO :: NonEmpty SignCreds -> LBS -> String -> IO (SignCreds, Either HS.SignatureError HXTC.XmlTree)
verifyIO creds el sid = capture' $ do
verifyIO creds el sid = do
results <- NL.zip creds <$> forM creds (\cred -> verifyIO' cred el sid)
case NL.filter (isRight . snd) results of
[result] -> pure result
_ -> throwIO . ErrorCall $ "all credentials failed to verify signature"
where
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
Comment on lines -275 to -279

@akshaymankar akshaymankar Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@blackheaven blackheaven Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 sets channel = 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.


verifyIO' :: SignCreds -> LBS -> String -> IO (Either HS.SignatureError HXTC.XmlTree)
verifyIO' (SignCreds SignDigestSha256 (SignKeyRSA key)) el sid = runExceptT $ do
Expand Down