Allow to configure optional logger function to log SAML requests/resp…

[RajuTanchak]

Allow to configure optional logger function to log SAML operations

We have migrated our application's Login component away from Java/Spring to Typescript with passport-saml.

One facet of the Spring SAML libraries which we found very useful when debugging SAML config issues in production was the ability to configure the library to ouput logs for key SAML operations. This was introduced in with a comment in the Spring code that said "Implementations are supposed to log significant SAML operations."

We have been unable to find a reference to this "supposed to" in the formal SAML specifications, but as mentioned it has been a very useful capability in the past, so we thought to propose this approach as a way to address the issue described in node-saml/passport-saml#279.

Checklist:

• Issue Addressed: [Wish: Improve debug logging and troubleshooting documentation.  passport-saml#279]
• Link to SAML spec: [ ]
• Tests included? [Yes]
• Documentation updated? [Yes]

[cjbarth]

I'm not against this, but I worry that it could be left on in production and then create some issues with log volume and security. Should it also be behind a debug flag? Can be leverage the debug() function that we already have, or perhaps migrate our one usage of debug() to a system unified with what you have here?

[srd90]

---

PR description says:

One facet of the Spring SAML libraries which we found very useful when debugging...

Implementation is using/implicating info level. Shouldn't it be log function's responsibility to solve proper logging level.

node-saml/src/saml.ts

Lines 695 to 697 in 96fc62d

	if (this.options.log) {
	this.options.log.info("SAML authorization response: '%s'", xml);
	}

---

Did I miss something or is this code path missing logging?

node-saml/src/saml.ts

Lines 860 to 876 in 96fc62d

	async validateRedirectAsync(
	container: ParsedQs,
	originalQuery: string
	): Promise<{ profile: Profile | null; loggedOut: boolean }> {
	const samlMessageType = container.SAMLRequest ? "SAMLRequest" : "SAMLResponse";
	const data = Buffer.from(container[samlMessageType] as string, "base64");
	const inflated = await inflateRawAsync(data);
	const dom = await parseDomFromString(inflated.toString());
	const doc: XMLOutput = await parseXml2JsFromString(inflated);
	samlMessageType === "SAMLResponse"
	? await this.verifyLogoutResponse(doc)
	: this.verifyLogoutRequest(doc);
	await this.hasValidSignatureForRedirect(container, originalQuery);
	return await this.processValidlySignedSamlLogoutAsync(doc, dom);
	}

It is called e.g. from https://github.com/node-saml/passport-saml/blob/91b1ba6df23fd5d30c36a3c52d4e6885cb075284/src/strategy.ts#L172

Same question about this:

node-saml/src/saml.ts

Lines 1245 to 1256 in 96fc62d

	async validatePostRequestAsync(
	container: Record<string, string>
	): Promise<{ profile: Profile; loggedOut: boolean }> {
	const xml = Buffer.from(container.SAMLRequest, "base64").toString("utf8");
	const dom = await parseDomFromString(xml);
	const doc = await parseXml2JsFromString(xml);
	const certs = await this.certsToCheck();
	if (!validateSignature(xml, dom.documentElement, certs)) {
	throw new Error("Invalid signature on documentElement");
	}
	return await this.processValidlySignedPostRequestAsync(doc, dom);
	}

---

@cjbarth commented:

...I worry that it could be left on in production and then create some issues with log volume and security.

This PR seems to introduce mechanism to log all HTTP(S) payloads of incoming SAML protocol messages and payloads of outgoing SAML protocol messages (whether those messages are SAML Requests or SAML Responses).

Furthermore these payloads are logged in this PR's implementation:

• in case of incoming messages: as early as possible
• in case of outgoing messages: as late as possible

maybe such logging could be achieved with some express or similar framework's capabilities to direct HTTP traffic from/to certain endpoints to log subsystem?

About this PR's "log incoming messages as early as possible" approach:

If incoming message is SAML authnresponse and if it has unencrypted assertions then (based on content of assertions) it could contain highly sensitive information (e.g. social security numbers, addresses, etc etc) and it would not be wise move to store those to any log system (if such log system is not hardened enough). Of course same problem would exists if payloads are directed from express or similars to log subsystem.

PR references to spring-attic/spring-security-saml#63 which states that significant events should be stored for auditing purposes. IMHO auditing purpose is fulfilled by logging only user ids or similar (but not whole assertion). Of course implementation of log function could extract only some parts of e.g. SAML authnresponse to be logged....

...which brings us to next problem: In case of "log incoming messages as early as possible" combined with encrypted assertions this PR's current implementation feeds incoming SAML authnresponse to log function before encrypted assertion is decrypted.

node-saml/src/saml.ts

Lines 685 to 697 in 96fc62d

	async validatePostResponseAsync(
	container: Record<string, string>
	): Promise<{ profile: Profile | null; loggedOut: boolean }> {
	let xml: string;
	let doc: Document;
	let inResponseTo: string | null = null;
	try {
	xml = Buffer.from(container.SAMLResponse, "base64").toString("utf8");
	if (this.options.log) {
	this.options.log.info("SAML authorization response: '%s'", xml);
	}

I.e. log function could not extract any meaningful information (from audit logging point of view) to be stored to (audit) log subsystem....

....so big picture seems to be that this PR contains mechanism for logging raw HTTP payloads to/from SAML endpoints and such (debug) logging could/might be achieved with some express or similar frameworks capabilities and same information masking that should be implemented to log function used with this PR's implementation should/could be used with some express or similar's "log SAML endpoint traffic interceptor implementation".

[cjbarth]

@RajuTanchak , what do you think of @srd90 's comments?

[cjbarth]

I'm going close this as there has been no response from the author, there are significant arguments against this, and there are merge conflicts.