Add public key support #225

RopoMen · 2022-11-18T08:00:43Z

Description

re-factored "crypto.ts" to handle PEM files in more general way.
Added unit tests for new functions.
Re-factored internally implementation to use "key info" (or similar) terminology instead of talking only about certificates. Certificate term is ok, if we known that we are handling certificate.

These changes are not making any changes into used interface, only internal re-factoring.

I will make changes to README.md before merge, because it is not currently talking about public key support. Public key support comes through cert property, if PEM file is well formatted then it is accepted.

RopoMen · 2022-11-18T08:01:48Z

Resolves #206

src/crypto.ts

codecov · 2022-11-18T08:04:34Z

Codecov Report

Merging #225 (fc60c79) into master (96ac785) will increase coverage by 0.14%.
The diff coverage is 98.03%.

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
+ Coverage   80.54%   80.68%   +0.14%     
==========================================
  Files          11       11              
  Lines         812      813       +1     
  Branches      247      249       +2     
==========================================
+ Hits          654      656       +2     
+ Misses         68       67       -1     
  Partials       90       90

Impacted Files	Coverage Δ
src/types.ts	`100.00% <ø> (ø)`
src/crypto.ts	`95.00% <93.75%> (+3.00%)`	⬆️
src/metadata.ts	`90.47% <100.00%> (ø)`
src/saml.ts	`78.12% <100.00%> (+0.28%)`	⬆️
src/xml.ts	`79.13% <100.00%> (-0.18%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cjbarth · 2022-11-18T13:42:52Z

Thank you for this, but all these code scanning alerts will have to be addressed. If you need help figuring out how to make the regex less catastrophic, please post here and we'll see what help we can provide.

It looks like the new code you added isn't 100% covered by tests. Please have a look and see what can be done about that.

I look forward to seeing your README updates; our README definitely needs a little TLC.

Also, if it is easy to add a test to add coverage to another part of the code that is currently untested, perhaps a branch, please consider that; sometimes I find that when I'm writing a test trying to get complete coverage for my new code, I see that I can make one tweak to a new test and cover a previously untested branch in some other code.

RopoMen · 2022-11-21T06:37:52Z

@cjbarth

..but all these code scanning alerts will have to be addressed. ofc. I'll check what is the best way to test potential fix before pushing changes. (I have not used Github code scanning tools before)
It looks like the new code you added isn't 100% covered by tests. , I'll check what improvements there can be made. Those PEM handling functions should be covered as much as possible. My guess is that assertions should be tested better.
README.md updates
Add at least one unit test for saml.ts where well formatted PEM public key is given into library.
Check that is there need to add tests for saml.ts getKeyInfosAsPem() function. (most likely yes)

cjbarth · 2022-11-21T16:23:14Z

..but all these code scanning alerts will have to be addressed. ofc. I'll check what is the best way to test potential fix before pushing changes. (I have not used Github code scanning tools before)

One of the best ways is to just look at the manual: https://codeql.github.com/codeql-query-help/javascript/js-polynomial-redos/. As suggested, anchoring the string or using some lookbehind helps a lot.

src/crypto.ts

RopoMen · 2022-11-24T11:56:15Z

@cjbarth I have now;

Added more tests for crypto.ts
Fixed regex issues
Added test for public key
Added tests for resolveAndParseKeyInfosToPem()
Updated readme to contain mention about public key support

src/crypto.ts

src/saml.ts

test/crypto.spec.ts

tsconfig.json

src/crypto.ts

cjbarth · 2022-11-24T19:17:23Z

Thanks for all your hard work and the clarity of your code. I see that this patch is almost at 100%, good job! Keep at it!

RopoMen · 2022-12-07T03:37:45Z

@cjbarth is there something that should be done to get this PR into master?

DeepScan finding is false-positive
codecov finding is minor

src/saml.ts

src/pem-label.enum.ts

cjbarth · 2022-12-13T13:26:16Z

@cjbarth is there something that should be done to get this PR into master?
* DeepScan finding is false-positive

* codecov finding is minor

Yes, please address the code review comments. I don't even see a reply on some of them.

test/tests.spec.ts

RopoMen · 2023-01-20T12:09:10Z

@cjbarth Hi, is there something that you are expecting me to work with this PR? Or do you have time allocation issues with this PR? I would like to work to get this PR finished.

cjbarth · 2023-01-20T12:28:17Z

It seems that my previous PR comments are unresolved. Why not start there? I'm also interested in addressing those regex warnings, but I need more time to review what can be done there.

…other than 'CERTIFICATE' PEM's.

… with tests.

… and changed 'certs' to 'pemFiles'.

…ile handling.

…is used inside SAML class protected function 'getKeyInfosAsPem()'

…natures. Cleanup 'stripPemHeaderAndFooter()' implementation.

…g and organized it better, also added information of public key support.

…s callback function.

…eyInfosToPem' not to contain nested arrays.

…positive issue.

…ut had to add also prettier ignore because new rules work with "(typeof PemLabel)..." and old rules work with "typeof PemLabel..." so it cannot be made to work with both. Added eslint ignore for 'any' inside types.ts

RopoMen · 2023-01-27T13:28:32Z

DeepScan stuff fixed, I'll check if I can get coverage a bit better.

Edit
This line getKeyInfo: () => "<X509Data></X509Data>", was not covered before nor now. And it is not currently used, if I read code correctly from here https://github.com/node-saml/xml-crypto/blob/52d5ad5a3f800cd1a252b81116f42bc943c09e55/lib/signed-xml.js#L355-L413

It only reads data from keyInfoProvider.getKey() and locally I tested setting it to empty string getKeyInfo: () => "", and at least current tests are passing.

RopoMen · 2023-02-08T05:09:08Z

@cjbarth Hi, I resolved most of the conversations on couple of weeks ago, but there are still those two open conversations. For now I need more feedback from you in order to get things moving.

If there are some issues which needs to be addressed in specific way other than those two open conversations, I wish you point out those explicitly where the issue is and how you want it to be fixed so that I can make those fixes.

cjbarth · 2023-04-19T01:39:33Z

I'm sorry for the delay; I was unavailable for a while. I'm back to giving this project more attention. I've read over the changes you've made and they look good. I'll take time to take a second look at my comments that are still open and see if they are still valid or if I need to add more details.

Conflicts: README.md test/tests.spec.ts

cjbarth · 2023-05-26T15:50:20Z

@RopoMen , I've pushed up a lot of changes to improve this code. We don't want to export code just for testing, so many tests were changed, or even removed, for that reason. Many tests were redundant.

I see the point about the codecov tool. I'll merge if you agree with the changes I've made. I look forward to your feedback.

RopoMen · 2023-05-26T15:54:49Z

@RopoMen , I've pushed up a lot of changes to improve this code. We don't want to export code just for testing, so many tests were changed, or even removed, for that reason. Many tests were redundant.

I see the point about the codecov tool. I'll merge if you agree with the changes I've made. I look forward to your feedback.

Hi @cjbarth
You can merge this if you are done. I don't mind about the changes, main point is more important.

Thank you to be active again 😁

github-advanced-security bot found potential problems Nov 18, 2022

View reviewed changes

src/crypto.ts Fixed Show fixed Hide fixed

src/crypto.ts Fixed Show fixed Hide fixed

src/crypto.ts Fixed Show fixed Hide fixed

cjbarth added the enhancement New feature or request label Nov 18, 2022

cjbarth reviewed Nov 23, 2022

View reviewed changes

src/crypto.ts Outdated Show resolved Hide resolved

src/crypto.ts Outdated Show resolved Hide resolved

src/crypto.ts Show resolved Hide resolved