Skip to content

Conversation

0x5457
Copy link
Contributor

@0x5457 0x5457 commented Aug 17, 2025

fix: #2601

After testing, I found that the fetch behavior is as follows:

  • Fetch only uses the NODE_TLS_REJECT_UNAUTHORIZED environment variable when the rejectUnauthorized property is completely absent from agent options
  • If rejectUnauthorized is explicitly set to any value (including undefined), the environment variable is ignored

Also should I can create test certificates in the test-data directory to avoid introducing the selfsigned library

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 17, 2025
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 20, 2025
@brendandburns
Copy link
Contributor

/lgtm
/approve

Thanks for updating the test.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0x5457, brendandburns

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2025
@brendandburns
Copy link
Contributor

Tests are failing with some error related to generating the certificate.

@mstruebing
Copy link
Member

Tests are failing with some error related to generating the certificate.

@0x5457 seems like you would need to pass in an option to define the key size: https://github.com/jfromaniello/selfsigned?tab=readme-ov-file#options 2048 should be sufficient.

Having a fixture certificate seems better to me as well but as this is only a dev dependency I would be fine with it.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2025
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@0x5457 0x5457 force-pushed the fix/node-tls-reject-unauthorized-env-var branch from 1782cf2 to 830dd41 Compare September 6, 2025 10:44
@0x5457
Copy link
Contributor Author

0x5457 commented Sep 6, 2025

Replaced selfsigned dependency with static test certificates

Generated using OpenSSL:

mkdir -p testdata/certs
openssl genrsa -out testdata/certs/test-key.pem 2048
openssl req -new -x509 -key testdata/certs/test-key.pem \
    -out testdata/certs/test-cert.pem -days 999999 -subj "/CN=localhost"

@0x5457 0x5457 force-pushed the fix/node-tls-reject-unauthorized-env-var branch from 830dd41 to 42cde8e Compare September 6, 2025 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NODE_TLS_REJECT_UNAUTHORIZED environment variable not respected in applyToFetchOptions
5 participants