lookup: add jose

[panva]

adds jose

It covers the node:crypto, and Web Cryptography API modules.

[panva]

May need a skip on fips if we any fips citgm runs in place? I'm not sure, but the test suite contains all available algorithms, some of which may not be allowed in a fips build.

[codecov-commenter]

Codecov Report

Base: 95.16% // Head: 95.34% // Increases project coverage by +0.18% 🎉

Coverage data is based on head (9c287a5) compared to base (a35b3fd).
Patch has no changes to coverable lines.

❗ Current head 9c287a5 differs from pull request most recent head 522696c. Consider uploading reports for the commit 522696c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #931      +/-   ##
==========================================
+ Coverage   95.16%   95.34%   +0.18%     
==========================================
  Files          28       28              
  Lines        2149     2149              
==========================================
+ Hits         2045     2049       +4     
+ Misses        104      100       -4     

Impacted Files	Coverage Δ
lib/match-conditions.js	94.00% <0.00%> (+4.00%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[panva]

cc @mhdawson

[panva]

Hard Requirements

• Module source code must be on Github.
• Published versions must include a tag on Github
• The test process must be executable with only the commands npm install && npm test (actually two unique test suites using the different crypto modules are present)
• The tests pass on supported major release lines (skipped 14 because of its lack of webcrypto)
• The maintainers of the module remain responsive when there are problems
• At least one module maintainer must be added to the lookup maintainers field

Soft Requirements

• The module must be actively used by the community
• The module must be heavily depended on
• The module must cover unique portions of our API (crypto, webcrypto)
• ❓ The module fits into a key category (e.g. Testing, Streams, Monitoring, etc.)
• The module is under the Node.js foundation Github org
• The module is identified as an important module by a Node.js Working Group
  Procedure

[panva]

@targos is there a CITGM specific environment variable based on which I could disable flaky network tests?

[targos]

I'm not sure, but in any case, you can add the env variables of your choice in the lookup like here:

citgm/lib/lookup.json

Line 254 in a35b3fd

"envVar": { "CI": true },

[panva]

@nodejs/citgm I believe this is ready for your review (CI)

There are errors in CI but they are not related to jose and happen before citgm is even downloaded or run.

[mhdawson]

I believe this is related to openid support and I believe that it's good/important that we have coverage of that area in CITGM. It would be also be great to add https://www.npmjs.com/package/openid-client as well.

[mhdawson]

I don't think we run CITGM on fips @richardlau can you fact check me on that?

[mhdawson]

LGTM, agree that the failures don't look related.

[mhdawson]

In respect to

would be also be great to add https://www.npmjs.com/package/openid-client as well.

I see that already happened, so nevermind :)

EDIT, actually I see the PR was closed without landing. @panva was that so you could get jose in first?

[panva]

actually I see the PR was closed without landing. @panva was that so you could get jose in first?

I need to update the test runner to something more flexible and predictable. So it's coming, eventually.

[mhdawson]

I need to update the test runner to something more flexible and predictable. So it's coming, eventually.

Thanks, good to know.

[richardlau]

I don't think we run CITGM on fips @richardlau can you fact check me on that?

We do not.

IBM did add some "expectFail": "fips" entries in lookup.json, e.g.

citgm/lib/lookup.json

Line 58 in a35b3fd

"expectFail": "fips",

for when they were internally running CITGM in a fips configuration, but I believe that stopped when Node.js moved to OpenSSL 1.1.1 where there was no upstream fips support.

[mhdawson]

@richardlau so it would be good to add "expectFail": "fips", in this PR for jose to handle a future where we might do fips testing again, right?

[panva]

@richardlau so it would be good to add "expectFail": "fips", in this PR for jose to handle a future where we might do fips testing again, right?

Who's to know without actually having a build? It may fail because of expectFail and the suite being fine, it might as well fail because of the opposite reason.

When we have a fips build we'll deal with it.

[mhdawson]

@panva that works for me :)

[richardlau]

@richardlau so it would be good to add "expectFail": "fips", in this PR for jose to handle a future where we might do fips testing again, right?

I'm inclined to agree with @panva and ignore it until it becomes an issue. I don't see much benefit for running CITGM against a FIPS enabled Node.js on a regular basis -- a lot of modules are going to fail for using (or using a dependency that is using) algorithms like md5 to do hashing, which is totally reasonable but not an algorithm permitted by FIPS.

[panva]

This is good to go now, i've even expanded the tests.

• 14
• 16
• 18
• 19

[mhdawson]

@nodejs/citgm any other reviewers?

[panva]

@richardlau @mhdawson Is this good to land then?

[mhdawson]

I'd like @richardlau's confirmation as I think he's been more active in the citgm repo.

[richardlau]

I'd like @richardlau's confirmation as I think he's been more active in the citgm repo.

I approved this. It LGTM.

[mhdawson]

Landed, @panva thanks for your work on this.

[panva]

Thank you!