Skip to content

Conversation

@tjuhaszrh
Copy link
Contributor

@tjuhaszrh tjuhaszrh commented Jun 18, 2025

Add two test cases to verify that if:

  • Container is in fips mode, node is also using fips mode.
  • Container isnt in fips mode, node also isnt using fips mode.

Nodejs fips state is verified with: node -e 'const crypto = require("crypto"); return crypto.getFips(); which should return the same value (1 for enabled, 0 for disabled) as cat "/proc/sys/crypto/fips_enabled".

@phracek
Copy link
Member

phracek commented Jun 19, 2025

Let's try first set of shots

[test]

@github-actions
Copy link

github-actions bot commented Jun 19, 2025

Testing Farm results

namecomposearchstatusstarted (UTC)timelogs
Fedora - 22-minimalFedora-latestx86_64✅ passed26.06.2025 08:28:247min 35stest pipeline
CentOS Stream 9 - 20-minimalCentOS-Stream-9x86_64✅ passed26.06.2025 08:28:227min 25stest pipeline
Fedora - 22Fedora-latestx86_64✅ passed26.06.2025 08:28:259min test pipeline
Fedora - 20Fedora-latestx86_64✅ passed26.06.2025 08:36:479min 22stest pipeline
CentOS Stream 10 - 22-minimalCentOS-Stream-10x86_64✅ passed26.06.2025 08:28:357min 37stest pipeline
CentOS Stream 9 - 20CentOS-Stream-9x86_64✅ passed26.06.2025 08:36:599min 8stest pipeline
CentOS Stream 10 - 22CentOS-Stream-10x86_64✅ passed26.06.2025 08:28:368min 53stest pipeline
RHEL9 - FIPS Enabled - 22-minimalRHEL-9.6.0-Nightlyx86_64✅ passed26.06.2025 10:50:4324min 1stest pipeline
RHEL9 - FIPS Enabled - 20RHEL-9.6.0-Nightlyx86_64❌ error26.06.2025 08:28:2416min 21stest pipeline
RHEL10 - 22-minimalRHEL-10-Nightlyx86_64✅ passed26.06.2025 08:28:2314min 42stest pipeline
RHEL8 - 22RHEL-8.10.0-Nightlyx86_64✅ passed26.06.2025 08:36:3720min 20stest pipeline
RHEL10 - 22RHEL-10-Nightlyx86_64✅ passed26.06.2025 08:28:5117min 17stest pipeline
RHEL8 - 22-minimalRHEL-8.10.0-Nightlyx86_64✅ passed26.06.2025 08:28:2517min 25stest pipeline
RHEL10 - FIPS Enabled - 22-minimalRHEL-10-Nightlyx86_64❌ error26.06.2025 12:20:2617min 18stest pipeline
RHEL9 - FIPS Enabled - 20-minimalRHEL-9.6.0-Nightlyx86_64✅ passed26.06.2025 10:50:4322min 54stest pipeline
RHEL10 - FIPS Enabled - 22RHEL-10-Nightlyx86_64❌ error26.06.2025 11:51:4216min 17stest pipeline
RHEL9 - FIPS Enabled - 22RHEL-9.6.0-Nightlyx86_64✅ passed26.06.2025 10:50:4425min 54stest pipeline
RHEL9 - 22-minimalRHEL-9.4.0-Nightlyx86_64✅ passed25.06.2025 12:25:5657min 56stest pipeline
RHEL8 - 20-minimalRHEL-8.10.0-Nightlyx86_64✅ passed26.06.2025 08:28:3518min 2stest pipeline
RHEL8 - 20RHEL-8.10.0-Nightlyx86_64✅ passed26.06.2025 08:28:3318min 5stest pipeline
RHEL9 - 20-minimalRHEL-9.6.0-Nightlyx86_64✅ passed26.06.2025 08:28:2518min 40stest pipeline
RHEL9 - 22RHEL-9.6.0-Nightlyx86_64✅ passed26.06.2025 08:38:4720min 29stest pipeline
Fedora - 20-minimalFedora-latestx86_64✅ passed26.06.2025 08:28:258min 5stest pipeline
RHEL9 - 20RHEL-9.6.0-Nightlyx86_64✅ passed26.06.2025 08:28:2420min 56stest pipeline

@phracek
Copy link
Member

phracek commented Jun 19, 2025

@tjuhaszrh The error is here:

Running test test_nodejs_fips_mode_off (starting at 2025-06-19 07:27:56+00:00) ... 
-----------------------------------------------
[eval]:1
(crypto=>{{const crypto = require(crypto); return crypto.getFips();}})(require('node:crypto'))
                                  ^

ReferenceError: Cannot access 'crypto' before initialization
    at [eval]:1:35
    at [eval]:1:71
    at runScriptInThisContext (node:internal/vm:209:10)
    at node:internal/process/execution:449:12
    at [eval]-wrapper:6:24
    at runScriptInContext (node:internal/process/execution:447:60)
    at evalFunction (node:internal/process/execution:87:30)
    at evalScript (node:internal/process/execution:99:3)
    at node:internal/main/eval_string:74:3

Node.js v22.15.0

@phracek
Copy link
Member

phracek commented Jun 19, 2025

@tjuhaszrh Thanks for this pull request. I have several question regarding nodeJs.

What do you thinkg about this code?

function test_nodejs_fips_mode_off() {
  local ret_val
  local is_fips_enabled
  # Read fips mode from host in case exists
  if [[ -f /proc/sys/crypto/fips_enabled ]]; then
    is_fips_enabled=$(cat /proc/sys/crypto/fips_enabled)
  else
    # Set to 0 if not exists
    is_fips_enabled="0"
  fi
  if [[ "$is_fips_enabled" == "0" ]]; then
    echo "FIPS is disabled on host"
    echo "What is expected output in case disabled fips???"
    fips=$(docker run --rm ${IMAGE_NAME}-testapp /bin/bash -c "! node -e 'const crypto = require("crypto"); return crypto.getFips();'")
    echo "FIPS from app: '$fips'" # For me $fips is empty...
    if [[ "$fips" == "" ]]; then
      ct_check_testcase_result "0"
    else
      ct_check_testcase_result "$retval"
    fi
  else
    # What is expected behavior here in case FIPS is enabled and we test for fips is disabled. Does it make sense either?
    # Check fips mode in container as well
    if docker run --rm ${IMAGE_NAME}-testapp /bin/bash -c "cat /proc/sys/crypto/fips_enabled | grep -q 0"; then
      fips=$(docker run --rm ${IMAGE_NAME}-testapp /bin/bash -c "node -e 'const crypto = require("crypto"); return crypto.getFips();'")
      echo "FIPS from app: '$fips'"
      ct_check_testcase_result "$?"
    fi
  fi
}

@tjuhaszrh
Copy link
Contributor Author

[test]

@tjuhaszrh
Copy link
Contributor Author

Sorry I think the issue was caused just by messy string nesting in require("crypto") .

Adding backslash seems to have fixed it.

➜  test git:(fips-nodejs) fips=$(podman run --rm localhost/node-app:latest /bin/bash -c "! node -e 'const crypto = require("crypto"); return crypto.getFips();'")             
[eval]:1
(crypto=>{{const crypto = require(crypto); return crypto.getFips();}})(require('node:crypto'))
                                  ^

ReferenceError: Cannot access 'crypto' before initialization
    at [eval]:1:35
    at [eval]:1:71
    at runScriptInThisContext (node:internal/vm:209:10)
    at node:internal/process/execution:118:14
    at [eval]-wrapper:6:24
    at runScript (node:internal/process/execution:101:62)
    at evalScript (node:internal/process/execution:133:3)
    at node:internal/main/eval_string:51:3

Node.js v20.19.2
➜  test git:(fips-nodejs) fips=$(podman run --rm localhost/node-app:latest /bin/bash -c "! node -e 'const crypto = require(\"crypto\"); return crypto.getFips();'")           
➜  test git:(fips-nodejs) 

@tjuhaszrh
Copy link
Contributor Author

tjuhaszrh commented Jun 19, 2025

@tjuhaszrh Thanks for this pull request. I have several question regarding nodeJs.

What do you thinkg about this code?

function test_nodejs_fips_mode_off() {
  local ret_val
  local is_fips_enabled
  # Read fips mode from host in case exists
  if [[ -f /proc/sys/crypto/fips_enabled ]]; then
    is_fips_enabled=$(cat /proc/sys/crypto/fips_enabled)
  else
    # Set to 0 if not exists
    is_fips_enabled="0"
  fi
  if [[ "$is_fips_enabled" == "0" ]]; then
    echo "FIPS is disabled on host"
    echo "What is expected output in case disabled fips???"
    fips=$(docker run --rm ${IMAGE_NAME}-testapp /bin/bash -c "! node -e 'const crypto = require("crypto"); return crypto.getFips();'")
    echo "FIPS from app: '$fips'" # For me $fips is empty...
    if [[ "$fips" == "" ]]; then
      ct_check_testcase_result "0"
    else
      ct_check_testcase_result "$retval"
    fi
  else
    # What is expected behavior here in case FIPS is enabled and we test for fips is disabled. Does it make sense either?
    # Check fips mode in container as well
    if docker run --rm ${IMAGE_NAME}-testapp /bin/bash -c "cat /proc/sys/crypto/fips_enabled | grep -q 0"; then
      fips=$(docker run --rm ${IMAGE_NAME}-testapp /bin/bash -c "node -e 'const crypto = require("crypto"); return crypto.getFips();'")
      echo "FIPS from app: '$fips'"
      ct_check_testcase_result "$?"
    fi
  fi
}

My understanding is that crypto.getFips(); returns exactly the same values as cat /proc/sys/crypto/fips_enabled (so 1 for enabled, 0 for disabled fips).

My usage of the fips variable was also wrong since I only care about the $? value and it doesn't store anything, I got slightly to inspired by the nodemon test.

So I think:
if [[ "$is_fips_enabled" == "0" ]]; then echo "FIPS is disabled on host" echo "What is expected output in case disabled fips???" fips=$(docker run --rm ${IMAGE_NAME}-testapp /bin/bash -c "! node -e 'const crypto = require("crypto"); return crypto.getFips();'") echo "FIPS from app: '$fips'" # For me $fips is empty... if [[ "$fips" == "" ]]; then ct_check_testcase_result "0" else ct_check_testcase_result "$retval" fi
this would probably always pass.

@tjuhaszrh tjuhaszrh force-pushed the fips-nodejs branch 2 times, most recently from c36fca6 to 415e2cd Compare June 19, 2025 15:44
@tjuhaszrh
Copy link
Contributor Author

Adjustments:

  • removed unused variable fips.
  • moved the logic into one function that verifies both states of fips mode.
  • added check for presence of /proc/sys/crypto/fips_enabled file.

@mhdawson
Copy link
Member

Great to see this test being added

@phracek
Copy link
Member

phracek commented Jun 20, 2025

[test]

@tjuhaszrh tjuhaszrh force-pushed the fips-nodejs branch 2 times, most recently from 364e024 to 31dd709 Compare June 23, 2025 08:15
@phracek
Copy link
Member

phracek commented Jun 23, 2025

Great to see this test being added

Hi @mhdawson , does it make sense to test also some a simple app, like we have in directory test-app (https://github.com/sclorg/s2i-nodejs-container/tree/master/test/test-app) also for FIPS mode? But may be with different name, like test-fips? What do you think?

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is almost done. Thanks for this pull request. Great work.

@tjuhaszrh tjuhaszrh force-pushed the fips-nodejs branch 2 times, most recently from cfff436 to 2c3f72f Compare June 23, 2025 08:50
@tjuhaszrh tjuhaszrh requested a review from phracek June 23, 2025 09:27
@phracek
Copy link
Member

phracek commented Jun 23, 2025

[test]

@mhdawson
Copy link
Member

@phracek a simple test application would be a good idea as well.

@phracek
Copy link
Member

phracek commented Jun 24, 2025

@tjuhaszrh Add to this pull request also fips https://github.com/sclorg/s2i-nodejs-container/blob/master/test/test-lib-nodejs.sh#L133
The code should be like

    app|hw|fips|express-webapp|binary)
      pushd "${test_dir}/test-${1}" >/dev/null
      prepare_dummy_git_repo
      popd >/dev/null
      ;;

Add there also function like is here https://github.com/sclorg/s2i-nodejs-container/blob/master/test/test-lib-nodejs.sh#L44

run_s2i_build_fips() {
  ct_s2i_build_as_df file://${test_dir}/test-fips ${IMAGE_NAME} ${IMAGE_NAME}-testfips ${s2i_args} $(ct_build_s2i_npm_variables) $1
}

@tjuhaszrh
Copy link
Contributor Author

Added a simple app that in the context of a server verifies what kind of Hash algorithms and Ciphers are allowed to be used under FIPS.

  • Allowed: SHA256, AES
  • Not allowed: MLD, 3DES with only two keys.

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the new app. Please address my changes.

tjuhaszrh and others added 2 commits June 25, 2025 14:24
Add test that verifies that if:
	Container is in fips mode, node is also using fips mode.

	Container isnt in fips mode, node also isnt using fips mode.

Add simple Fips application which in the context of server verifies:
*	That FIPS allows the generation  of only secure hash algorithms and ciphers.
*	Should allow: SHA256, AES. Should Fail: MLD, 2Key 3DES.
Couple test fixes caught by testing on RHEL9 host.

Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
@phracek
Copy link
Member

phracek commented Jun 25, 2025

Rebased against upstream. Fixing some tests.

[test]

@phracek
Copy link
Member

phracek commented Jun 26, 2025

Let's tests one more time

[test]

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests have passed. GREAT WORK. THank you

@phracek phracek merged commit d9f9c28 into sclorg:master Jun 27, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants