Skip to content

Commit

Permalink
improve debug output (node-saml#279)
Browse files Browse the repository at this point in the history
This commit improves the debug output by adding the SAML response ID to
the debug message and by exposing the validation errors from xml-crypto
in case signature validation fails.

Here are examples of the improve output:

passport-saml checkSignature failed for pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa. Validation errors: invalid signature: for uri #pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa calculated digest is 0AbCoTZl3NxNBiPUyucHk/7gay8= but the xml to validate supplies digest DCnPTQYBb1hKspbe6fg1U3q8xn4= +-2017d

passport-saml validatePostResponse for id "_6a377272c8662561acf1056274ef3f81" resulted in an error: Error: SAML provider returned Responder error: InvalidNameIDPolicy +1ms

Testing done:
ran 'DEBUG=passport-saml npm run-script test' successfully,
saw the expected output.
  • Loading branch information
giladwolff committed Dec 5, 2019
1 parent c82149d commit d250c14
Showing 1 changed file with 18 additions and 5 deletions.
23 changes: 18 additions & 5 deletions lib/passport-saml/saml.js
Expand Up @@ -303,7 +303,7 @@ SAML.prototype.generateLogoutResponse = function (req, logoutRequest) {
};

SAML.prototype.requestToUrl = function (request, response, operation, additionalParameters, callback) {

const requestToUrlHelper = (err, buffer) => {
if (err) {
return callback(err);
Expand Down Expand Up @@ -537,7 +537,7 @@ SAML.prototype.validateSignature = function (fullXml, currentNode, certs) {
if (signatures.length != 1) {
return false;
}

const signature = signatures[0];
return certs.some(certToCheck => {
return this.validateSignatureForCert(signature, certToCheck, fullXml, currentNode);
Expand Down Expand Up @@ -570,11 +570,19 @@ SAML.prototype.validateSignatureForCert = function (signature, cert, fullXml, cu
if (totalReferencedNodes.length > 1) {
return false;
}
return sig.checkSignature(fullXml);

if (!sig.checkSignature(fullXml)) {
debug('checkSignature failed for refId %s. Validation errors: %s',
refId,
sig.validationErrors);
return false;
}
return true;
};

SAML.prototype.validatePostResponse = function (container, callback) {
var xml, doc, inResponseTo;
var responseId = 'unknown';

Q.fcall(() => {
xml = Buffer.from(container.SAMLResponse, 'base64').toString('utf8');
Expand All @@ -584,6 +592,11 @@ SAML.prototype.validatePostResponse = function (container, callback) {
if (!Object.prototype.hasOwnProperty.call(doc, 'documentElement'))
throw new Error('SAMLResponse is not valid base64-encoded XML');

var id = xpath(doc, "/*[local-name()='Response']/@ID");
if (id && id.length) {
responseId = id[0].nodeValue;
}

inResponseTo = xpath(doc, "/*[local-name()='Response']/@InResponseTo");

if (inResponseTo) {
Expand Down Expand Up @@ -708,7 +721,7 @@ SAML.prototype.validatePostResponse = function (container, callback) {
});
})
.fail(err => {
debug('validatePostResponse resulted in an error: %s', err);
debug('validatePostResponse for id %s resulted in an error: %s', responseId, err);
if (this.options.validateInResponseTo) {
Q.ninvoke(this.cacheProvider, 'remove', inResponseTo)
.then(function() {
Expand Down Expand Up @@ -904,7 +917,7 @@ SAML.prototype.processValidlySignedAssertion = function(xml, samlResponseXml, in
if (inResponseTo) {
profile.inResponseTo = inResponseTo;
}

var authnStatement = assertion.AuthnStatement;
if (authnStatement) {
if (authnStatement[0].$ && authnStatement[0].$.SessionIndex) {
Expand Down

1 comment on commit d250c14

@cjbarth
Copy link

Choose a reason for hiding this comment

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

Is there any desire to make a PR for this against the node-saml project?

Please sign in to comment.