Add FIPS support#417
Conversation
Necessary Changes
|
There was a problem hiding this comment.
Pull request overview
This PR adds FIPS-aware behavior to the PEM/OpenSSL integration so the library can run in FIPS mode (notably on OpenSSL 3.x) while preserving existing non-FIPS defaults and backward compatibility.
Changes:
- Updated key/PKCS#12 handling to be FIPS-compatible (avoid
-legacy/-traditionalin FIPS; add-pbmac1_pbkdf2; migrate RSA ops togenpkey/pkeywhere needed). - Adjusted default DH/EC parameter behavior and updated tests to pass in both FIPS and non-FIPS environments (including conditional skips).
- Refreshed/added test fixtures (FIPS-compliant vs legacy variants) and normalized some docs files’ line endings.
Reviewed changes
Copilot reviewed 21 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/pem.spec.js | Makes assertions/fixtures conditional on FIPS mode; adjusts PKCS#12 password usage and CA creation tests. |
| test/openssl.spec.js | Switches DH param size expectations based on FIPS mode. |
| test/fixtures/testnopw.key | Updates fixture to a FIPS-friendly PKCS#8 private key. |
| test/fixtures/testnopw_des.key | Adds legacy DES/RSA key fixture for non-FIPS runs. |
| test/fixtures/test.p7b | Replaces PKCS7 fixture with a FIPS-friendly variant. |
| test/fixtures/test.key | Replaces encrypted key fixture with a FIPS-friendly encrypted PKCS#8 key. |
| test/fixtures/test.dh | Updates DH params fixture to a larger/FIPS-acceptable group. |
| test/fixtures/test.csr | Updates CSR fixture to match new key/cert material. |
| test/fixtures/test.crt | Updates certificate fixture to match new key/cert material. |
| test/fixtures/test_des.p7b | Adds legacy PKCS7 fixture for non-FIPS runs. |
| test/fixtures/test_des.key | Adds legacy encrypted RSA key fixture for non-FIPS runs. |
| test/fixtures/test_des.csr | Adds legacy CSR fixture for non-FIPS runs. |
| test/fixtures/test_des.crt | Adds legacy certificate fixture for non-FIPS runs. |
| test/fixtures/test_1024_bit.dh | Adds legacy 1024-bit DH params fixture for non-FIPS runs. |
| test/fixtures/rsa_pkcs12_5_keyStore2.p12 | Updates PKCS#12 fixture for FIPS-compatible settings. |
| test/fixtures/rsa_pkcs12_5_keyStore2_legacy.p12 | Adds/updates legacy PKCS#12 fixture for non-FIPS runs. |
| test/fixtures/rsa_pkcs12_5_keyStore.p12 | Updates PKCS#12 fixture for FIPS-compatible settings. |
| test/fixtures/rsa_pkcs12_5_keyStore_legacy.p12 | Adds/updates legacy PKCS#12 fixture for non-FIPS runs. |
| test/fixtures/rsa_pkcs12_5_key_RSA.pem | Updates RSA key fixture (now PKCS#8) for FIPS compatibility. |
| test/fixtures/rsa_pkcs12_5_key_RSA_traditional.pem | Adds traditional-format RSA key fixture for non-FIPS compatibility. |
| test/fixtures/idsrv3test.pfx | Updates PFX fixture for FIPS-compatible settings. |
| test/fixtures/idsrv3test_legacy.pfx | Adds legacy PFX fixture for non-FIPS runs. |
| test/convert.spec.js | Switches fixture inputs based on FIPS mode for conversion tests. |
| test/ca.spec.js | Marks the root CA certificate creation as CA: true. |
| lib/pem.js | Implements FIPS-aware defaults/flags, migrates RSA handling to genpkey/pkey, and adjusts PKCS#12 behavior. |
| lib/helper.js | Adjusts password-file argument validation and handling for genpkey. |
| lib/convert.js | Adds FIPS-aware PKCS#12 flags during conversions. |
| docs/jsdoc/helper.js.html | Line-ending change; embedded listing may be stale vs current helper implementation. |
| docs/docco/lib/helper.html | Line-ending change; embedded listing may be stale vs current helper implementation. |
Comments suppressed due to low confidence (1)
lib/pem.js:688
getPublicKey()treats PEM blocks beginning with-----BEGIN ENCRYPTED PRIVATE KEY-----as certificates (falls through to the x509 branch), so extracting a public key from an encrypted PKCS#8 private key will fail. Extend the private-key detection to include ENCRYPTED PRIVATE KEY and keep using thepkey -puboutpath (and ensure passphrase handling matches how other functions handle encrypted keys).
if (certificate.match(/BEGIN(\sNEW)? CERTIFICATE REQUEST/)) {
params = ['req',
'-in',
'--TMPFILE--',
'-pubkey',
'-noout'
]
} else if (certificate.match(/BEGIN RSA PRIVATE KEY/) || certificate.match(/BEGIN PRIVATE KEY/)) {
params = ['pkey',
'-in',
'--TMPFILE--',
'-pubout'
]
} else {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This reverts commit a0d717e.
…rsion to traditional format
|
@Dexus after the first Copilot review, I made the necessary changes to make this library compatible with both FIPS and non-FIPS environments, and with both OpenSSL and LibreSSL, while avoiding the outdated, low-level commands of OpenSSL at the same time. Can you please take a look at this PR? |
|
I will review and check in my environment in the next days. Thank you for you contribution. |
There was a problem hiding this comment.
Pull request overview
This PR adds FIPS-mode compatibility across the module by switching away from legacy OpenSSL subcommands/flags, conditionally adjusting defaults and PKCS#12 handling when crypto.getFips() indicates FIPS mode, and updating tests/fixtures so the suite can run in both FIPS and non-FIPS environments.
Changes:
- Replace legacy OpenSSL key/param generation commands (
genrsa,ecparam,dhparam, etc.) withgenpkey/pkey/pkeyparamand add conditional FIPS behavior (e.g., DH/EC defaults, PKCS#12 pbkdf settings). - Update tests to skip non-FIPS-allowed algorithms/sizes and to use FIPS-compliant fixture variants.
- Add/replace crypto fixtures (certs/keys/pkcs12 bundles) to support both modes; normalize some generated documentation files and line endings.
Reviewed changes
Copilot reviewed 22 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/pem.spec.js | Adjusts tests for FIPS vs non-FIPS behavior (DH/EC defaults, MD5 skips, PKCS#12 passwords, fixture selection). |
| test/openssl.spec.js | Updates DH parameter size expectations based on FIPS mode. |
| test/convert.spec.js | Switches fixtures used by conversion tests based on FIPS mode. |
| test/ca.spec.js | Marks CA cert creation with CA: true to produce CA-appropriate extensions. |
| lib/pem.js | Core FIPS support: new key conversion helper, genpkey/pkey/pkeyparam adoption, PKCS#12 pbmac changes, modulus/DH parsing changes, CA CSR config support. |
| lib/openssl.js | Narrows EC-parameter special-casing to only the legacy ecparam path. |
| lib/helper.js | Updates password-file handling and argument emission, plus line-ending normalization. |
| lib/convert.js | Adds -pbmac1_pbkdf2 for PKCS#12 flows when OpenSSL 3+ and FIPS. |
| docs/jsdoc/helper.js.html | Line-ending change, but content now appears stale vs updated lib/helper.js. |
| docs/docco/lib/helper.html | Line-ending change, but content now appears stale vs updated lib/helper.js. |
| test/fixtures/testnopw_des.key | Adds non-FIPS (DES-era) fixture variant used when not in FIPS mode. |
| test/fixtures/testnopw.key | Replaces/updates key fixture for FIPS-compliant runs. |
| test/fixtures/test_des.p7b | Adds non-FIPS PKCS7 fixture variant. |
| test/fixtures/test_des.key | Adds non-FIPS encrypted key fixture variant. |
| test/fixtures/test_des.csr | Adds non-FIPS CSR fixture variant. |
| test/fixtures/test_des.crt | Adds non-FIPS cert fixture variant. |
| test/fixtures/test_1024_bit.dh | Adds 1024-bit DH params fixture for non-FIPS expectations. |
| test/fixtures/test.p7b | Updates PKCS7 fixture (FIPS-compliant version). |
| test/fixtures/test.key | Updates encrypted key fixture (FIPS-compliant version). |
| test/fixtures/test.dh | Updates DH params fixture (FIPS-compliant version). |
| test/fixtures/test.csr | Updates CSR fixture (FIPS-compliant version). |
| test/fixtures/test.crt | Updates cert fixture (FIPS-compliant version). |
| test/fixtures/rsa_pkcs12_5_key_RSA_traditional.pem | Adds a “traditional” RSA private key fixture used in PKCS#12 tests. |
| test/fixtures/rsa_pkcs12_5_key_RSA.pem | Updates RSA key fixture (PKCS#8 form). |
| test/fixtures/rsa_pkcs12_5_keyStore_legacy.p12 | Adds legacy PKCS#12 fixture for non-FIPS mode. |
| test/fixtures/rsa_pkcs12_5_keyStore2_legacy.p12 | Adds legacy PKCS#12 fixture for non-FIPS mode. |
| test/fixtures/rsa_pkcs12_5_keyStore2.p12 | Adds/updates PKCS#12 fixture for FIPS mode. |
| test/fixtures/rsa_pkcs12_5_keyStore.p12 | Adds/updates PKCS#12 fixture for FIPS mode. |
| test/fixtures/idsrv3test_legacy.pfx | Adds legacy PFX fixture for non-FIPS mode. |
| test/fixtures/idsrv3test.pfx | Adds/updates PFX fixture for FIPS mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -632,7 +756,7 @@ function getPublicKey(certificate, callback) { | |||
| '-noout' | |||
| ] | |||
| } else if (certificate.match(/BEGIN RSA PRIVATE KEY/) || certificate.match(/BEGIN PRIVATE KEY/)) { | |||
There was a problem hiding this comment.
Looks like getPublicKkey() was originally designed to take a decrypted private key as input, and including this change would mean a password should be given as an argument too. What do you suggest?
Description
This module currently uses the flags
-legacyand-traditionalfor backwards compatibility with older versions of OpenSSL and some functions use non-FIPS approved algorithms by default. This PR has changes which support FIPS mode while also keeping the existing default settings when this module is run in non-FIPS environment.Changes
ifconditions where "Vendor" and "VendorVersionMajor" are being checked,getFips()is used to add flags which are necessary in FIPS environment. Similarly,!getFips()is used to avoid-legacyand-traditionalflags which do not function properly in FIPS mode.genrsa,rsa,dhparam, andecparamare replaced withgenpkey,pkey, andpkeyparambecause the former are legacy. The only exceptions are when converting keys to the traditional format and verifying the validity of the keys when LibreSSL is being used. As a result, if the module is running in non-FIPS mode, the generated private key will be changed to the traditional format to replicate the existing functionality ofgenrsa -traditional.test/fixtures/which are being read while running the tests and are not FIPS compliant are renamed and FIPS compliant versions of those files are created.lib/helper.js,docs/jsdoc/helper.js.html, anddocs/docco/lib/helper.htmlis changed to Unix since the rest of the files in the repository are in the Unix format.Testing
Test cases are updated such that they pass both in FIPS and non-FIPS environment with OpenSSL 3.6.1, and in non-FIPS environment with OpenSSL 1.1.1f and LibreSSL 4.2.1. The commands used to test these changes are
yarn install(for initial setup) andnpm run test(executes the test cases) in both FIPS and non-FIPS environments.