fix!: handle poorly formatted PEM files #85

david-renaud-okta · 2022-04-12T04:44:05Z

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.

This change adds test coverage for new/changed/fixed functionality

Checklist

I have added documentation for new/changed functionality in this PR or in auth0.com/docs
All active GitHub checks for tests, formatting, and security are passing
The correct base branch is being used, if not master

Added seamless handling of poorly formatted PEM files for signing and encryption

shane-tomlinson · 2022-04-12T13:13:18Z

lib/xml/sign.js

+ } catch (err) {
+ console.warn(`Failed to sign xml. Attempting to fix errors and retrying. Error=${err.message}`)
+ key = utils.fixPemFormatting(key)
+ signed = sign()


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.

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

shane-tomlinson · 2022-04-12T13:22:38Z

lib/xml/encrypt.js

+ },
+ function (retryErr, retryEncrypted) {
+ err = retryErr;
+ encrypted = retryEncrypted;


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)); }); });

Makes sense. Updated

shane-tomlinson · 2022-04-12T13:37:36Z

lib/xml/sign.js

+ signed = sign()
+ } catch (err) {
+ console.warn(`Failed to sign xml. Attempting to fix errors and retrying. Error=${err.message}`)
+ key = utils.fixPemFormatting(key)


Instead of re-assigning key, it might be clearer to pass the key into sign:

signed = sign(key); ... signed = sign(utils.fixPemFormatting(key));

Good idea. Done

shane-tomlinson · 2022-04-12T13:45:24Z

test/saml11.tests.js

+ var signedAssertion = saml11[createAssertion]({...options, key: Buffer.from(options.key.toString().replaceAll(/[\r\n]/g, ''))});
+ assertSignature(signedAssertion, options);
+ });
+


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

Good catch. I've added some test cases for this.

Addressing review comments

shane-tomlinson

I think all that's need is a return and this should be good to go!

shane-tomlinson · 2022-04-13T12:39:37Z

lib/xml/encrypt.js

- err = retryErr;
- encrypted = retryEncrypted;
+  if (retryErr) return callback(retryErr)
+  return callback(null, utils.removeWhitespace(retryEncrypted));
 })


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.

I re-worked it a bit because it got messy fast trying to deal with the callbacks.

Addressing review comments

shane-tomlinson · 2022-04-19T09:21:05Z

lib/xml/encrypt.js

- if (err) return callback(err)
- return callback(null, utils.removeWhitespace(encrypted));
- });
+ try {


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)); } });

Somehow I forgot about that quirk of async calls. I borrowed your code.

Addressing review comments

shane-tomlinson

👍 shippit!

shane-tomlinson · 2022-04-19T18:55:56Z

lib/xml/encrypt.js

+ {
+ ...encryptOptions,
+ rsa_pub: utils.fixPemFormatting(encryptOptions.rsa_pub),
+ pem: utils.fixPemFormatting(options.encryptionCert),


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!

Fixed. I added the missing tests for encryption

shane-tomlinson

I just noticed that options does not exist in encrypt.js.

Addressing review comments

shane-tomlinson

Looks great to me!

forrest-ua · 2022-04-25T13:54:03Z

lib/xml/encrypt.js

- callback(null, utils.removeWhitespace(encrypted));
+ if (err) {
+ // Attempt to fix errors and retry
+ console.warn(


@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?

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

tomauth0 · 2022-04-25T10:14:57Z

lib/utils.js

+ 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.');


Can we lose the console logging?

tomauth0 · 2022-04-25T10:23:24Z

lib/utils.js


 return function () {
 return prototypeDoc.cloneNode(true);
 };
 };
+
+/**
+ * Standardizes PEM content to match the spec (best effort)


It would be good to add the limits of this standardization to the function description - namely that it only adds and updates newlines

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.

tomauth0 · 2022-04-25T10:27:28Z

lib/xml/encrypt.js

@@ -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) {


Is there a specific error we can watch for here, rather than attempting re-encryption for any error type?

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

fix!: handle poorly formatted PEM files

597b4c1

Added seamless handling of poorly formatted PEM files for signing and encryption

shane-tomlinson reviewed Apr 12, 2022

View reviewed changes

fix!: handle poorly formatted PEM files

3807388

Addressing review comments

david-renaud-okta force-pushed the IPS-2457 branch from 5c7db0e to 3807388 Compare April 13, 2022 06:25

shane-tomlinson suggested changes Apr 13, 2022

View reviewed changes

fix!: handle poorly formatted PEM files

3af7211

Addressing review comments

david-renaud-okta requested a review from shane-tomlinson April 13, 2022 19:15

shane-tomlinson reviewed Apr 19, 2022

View reviewed changes

fix!: handle poorly formatted PEM files

2d295bf

Addressing review comments

shane-tomlinson approved these changes Apr 19, 2022

View reviewed changes

shane-tomlinson reviewed Apr 19, 2022

View reviewed changes

shane-tomlinson suggested changes Apr 19, 2022

View reviewed changes

fix!: handle poorly formatted PEM files

3eb1a74

Addressing review comments

shane-tomlinson approved these changes Apr 21, 2022

View reviewed changes

forrest-ua reviewed Apr 25, 2022

View reviewed changes

tomauth0 reviewed Apr 25, 2022

View reviewed changes

fix!: handle poorly formatted PEM files

296b619

Addressing review comments

forrest-ua approved these changes Apr 26, 2022

View reviewed changes

tomauth0 approved these changes Apr 28, 2022

View reviewed changes

david-renaud-okta merged commit 8830a23 into master Apr 28, 2022

david-renaud-okta deleted the IPS-2457 branch April 28, 2022 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: handle poorly formatted PEM files #85

fix!: handle poorly formatted PEM files #85

david-renaud-okta commented Apr 12, 2022 •

edited

Loading

shane-tomlinson Apr 12, 2022

david-renaud-okta Apr 13, 2022

shane-tomlinson Apr 12, 2022

david-renaud-okta Apr 13, 2022

shane-tomlinson Apr 12, 2022

david-renaud-okta Apr 13, 2022

shane-tomlinson Apr 12, 2022

david-renaud-okta Apr 13, 2022

shane-tomlinson left a comment

shane-tomlinson Apr 13, 2022

david-renaud-okta Apr 13, 2022

shane-tomlinson Apr 19, 2022

david-renaud-okta Apr 19, 2022

shane-tomlinson left a comment

shane-tomlinson Apr 19, 2022

david-renaud-okta Apr 20, 2022

shane-tomlinson left a comment

shane-tomlinson left a comment

forrest-ua Apr 25, 2022

david-renaud-okta Apr 25, 2022

tomauth0 Apr 25, 2022

david-renaud-okta Apr 25, 2022

tomauth0 Apr 25, 2022

david-renaud-okta Apr 25, 2022

tomauth0 Apr 25, 2022

david-renaud-okta Apr 25, 2022

fix!: handle poorly formatted PEM files #85

fix!: handle poorly formatted PEM files #85

Conversation

david-renaud-okta commented Apr 12, 2022 • edited Loading

Description

References

Testing

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shane-tomlinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shane-tomlinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shane-tomlinson left a comment

Choose a reason for hiding this comment

shane-tomlinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-renaud-okta commented Apr 12, 2022 •

edited

Loading