-
Notifications
You must be signed in to change notification settings - Fork 91
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.
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
fix!: handle poorly formatted PEM files #85
Conversation
Added seamless handling of poorly formatted PEM files for signing and encryption
lib/xml/sign.js
Outdated
} catch (err) { | ||
console.warn(`Failed to sign xml. Attempting to fix errors and retrying. Error=${err.message}`) | ||
key = utils.fixPemFormatting(key) | ||
signed = sign() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's an exception in the second invocation of sign
, what happens? I imagine we should call the callback with the error.
This is interesting because even in master
, exceptions in sig.getSignedXml()
are not passed back in the callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original thinking was to address the PEM handling issue only and leave the exception handling behaviour mostly unchanged but I think you are correct and it should return the error in the callback. I have made the changes
lib/xml/encrypt.js
Outdated
}, | ||
function (retryErr, retryEncrypted) { | ||
err = retryErr; | ||
encrypted = retryEncrypted; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The callback style of xmlenc.encrypt
implies the operation is asynchronous. If so, callback
on line 48 is going to be called this inner callback is invoked.
Even if xmlenc.encrypt is synchronous today, it's not guaranteed to be that way and could break us in the future.
A more robust solution would have each of the callbacks to xmlenc.encrypt
make it's own call to callback
, something like:
xmlenc.encrypt(xml, encryptOptions, function (err, encrypted) {
if (!err) {
callback(null, utils.removeWhitespace(encrypted));
}
// Attempt to fix errors and retry
console.warn(`Failed to encrypt xml. Attempting to fix errors and retrying. Error=${err.message}`);
xmlenc.encrypt(xml, {
...encryptOptions,
rsa_pub: utils.fixPemFormatting(encryptOptions.rsa_pub),
pem: utils.fixPemFormatting(options.encryptionCert)
}, function (retryErr, retryEncrypted) {
if (retryErr) return callback(retryErr);
callback(null, utils.removeWhitespace(retryEncrypted));
});
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Updated
lib/xml/sign.js
Outdated
signed = sign() | ||
} catch (err) { | ||
console.warn(`Failed to sign xml. Attempting to fix errors and retrying. Error=${err.message}`) | ||
key = utils.fixPemFormatting(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of re-assigning key
, it might be clearer to pass the key into sign
:
signed = sign(key);
...
signed = sign(utils.fixPemFormatting(key));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Done
var signedAssertion = saml11[createAssertion]({...options, key: Buffer.from(options.key.toString().replaceAll(/[\r\n]/g, ''))}); | ||
assertSignature(signedAssertion, options); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at RFC 7468, data is allowed to appear before and after the encapsulation boundary, worthwhile to see if that works?
I imagine one of these certs would look something like:
some data that should
be ignored
-----BEGIN CERTIFICATE-----
BIGOLDCERTIFICATE
-----END CERTIFICATE-----
more data to ignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I've added some test cases for this.
Addressing review comments
5c7db0e
to
3807388
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all that's need is a return
and this should be good to go!
lib/xml/encrypt.js
Outdated
err = retryErr; | ||
encrypted = retryEncrypted; | ||
if (retryErr) return callback(retryErr) | ||
return callback(null, utils.removeWhitespace(retryEncrypted)); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll want to add a return
after this call to xmlenc.encrypt
to avoid the callback being called on both lines 43 and 48.
I also believe that it should be possible to remove line 47 since this new branch will handle the error case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-worked it a bit because it got messy fast trying to deal with the callbacks.
Addressing review comments
lib/xml/encrypt.js
Outdated
if (err) return callback(err) | ||
return callback(null, utils.removeWhitespace(encrypted)); | ||
}); | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try/catch only works for code that is synchronous or uses async/await. If xmlenc.encrypt is synchronous, this will work, but similar to a comment last week, the callback continuation style implies an asynchronous operation. If it's synchronous in reality, that's an implementation detail we shouldn't rely on since the API contract states otherwise.
I think what we want here is something like:
xmlenc.encrypt(xml, encryptOptions, function (err, encrypted) {
if (err) {
// Attempt to fix errors and retry
console.warn(
`Failed to encrypt xml. Attempting to fix errors and retrying. Error=${err.message}`
);
xmlenc.encrypt(
xml,
{
...encryptOptions,
rsa_pub: utils.fixPemFormatting(encryptOptions.rsa_pub),
pem: utils.fixPemFormatting(options.encryptionCert),
},
function (retryErr, retryEncrypted) {
if (retryErr) {
return callback(retryErr);
}
callback(null, utils.removeWhitespace(retryEncrypted));
}
);
} else {
callback(null, utils.removeWhitespace(encrypted));
}
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow I forgot about that quirk of async calls. I borrowed your code.
Addressing review comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 shippit!
lib/xml/encrypt.js
Outdated
{ | ||
...encryptOptions, | ||
rsa_pub: utils.fixPemFormatting(encryptOptions.rsa_pub), | ||
pem: utils.fixPemFormatting(options.encryptionCert), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options
doesn't exist (I only noticed this because I ran eslint on this file). This leads me to believe there might not be a test for this case!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I added the missing tests for encryption
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that options
does not exist in encrypt.js
.
Addressing review comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me!
lib/xml/encrypt.js
Outdated
callback(null, utils.removeWhitespace(encrypted)); | ||
if (err) { | ||
// Attempt to fix errors and retry | ||
console.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-renaud-okta If I understand correctly we are adding a few of "console" calls, that haven't been used before.
I'm a bit worried about the consumers of this library and control they will have (or lack of it) to disable these logs, given that in some cases these PEMs might be coming from users, they might be spamming consumer systems with tons of warnings. Unless we provide some more sophisticated solution of signing for log events/injecting logger, maybe it makes sense to remove these logs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind the warning was to discourage using malformed PEMs but I can how that would be problematic when they come from users. It may be difficult to completely eliminate this kind of error which could create a lot of noise for something that doesn't impact the overall functionality.
I've removed the log
lib/utils.js
Outdated
fixedPem = fixedPem.concat(`${pemParts[1]}\n${pemParts[2].replaceAll(/[\r\n]/g, '')}\n${pemParts[3]}\n`) | ||
} | ||
if (fixedPem.length === 0) { | ||
console.debug('fixPemFormatting: Content does not appear to be PEM. Passing through unmodified.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we lose the console logging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
return function () { | ||
return prototypeDoc.cloneNode(true); | ||
}; | ||
}; | ||
|
||
/** | ||
* Standardizes PEM content to match the spec (best effort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add the limits of this standardization to the function description - namely that it only adds and updates newlines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it vague so that we could extend this in the future if other easily correctable errors come up. It's meant to be a catch-all reformatter for which we currently only have 1 case covered.
@@ -29,8 +29,29 @@ exports.unencrypted = function (xml, callback) { | |||
exports.encrypted = function (encryptOptions) { | |||
return function encrypt(xml, callback) { | |||
xmlenc.encrypt(xml, encryptOptions, function (err, encrypted) { | |||
if (err) return callback(err); | |||
callback(null, utils.removeWhitespace(encrypted)); | |||
if (err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific error we can watch for here, rather than attempting re-encryption for any error type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We'd be trying to match on error text which is unreliable since there is no guarantee it won't change between releases.
Addressing review comments
Description
After upgrading to a newer version of the xml-encryption library some poorly formatted PEM files which were previously usable started producing errors for signing and encryption. This changes re-enables support for those files to maintain backwards compatibility.
References
https://auth0team.atlassian.net/browse/IPS-2457
Testing
All automated test suites pass.
Checklist
master