Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NODE-6253): use runtime linking against system kerberos libraries by default #188

Merged
merged 2 commits into from
Jul 10, 2024

Conversation

nbbeeken
Copy link
Collaborator

@nbbeeken nbbeeken commented Jul 8, 2024

Description

What is changing?

Set kerberos_use_rtld to true by default

Is there new documentation needed for these changes?

No

What is the motivation for this change?

See highlight

Release Highlight

Fix segfault when running kerberos on systems with 1.x OpenSSL versions and Node.js 18+

Kerberos depends on OpenSSL and Node.js always bundles a copy of OpenSSL. Unfortunately an incompatiblity arises when Node's SSL version is not compatible with the version that the system kerberos library was built with.

Kerberos will now load the system library by default with runtime dynamic linking. This enables us to specify that kerberos use the SSL version it was built against (RTLD_DEEPBIND) so it does not adopt the symbols available in Node.js' address space.

Starting in Node 18+ these Node's SSL symbols are from OpenSSL 3+, whereas on RHEL 8 the system SSL library is 1.1.1k.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

Copy link
Contributor

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

GYP_DEFINES: kerberos_use_rtld=true
should probably be inverted so that we keep testing both variants?

@nbbeeken nbbeeken requested a review from addaleax July 8, 2024 16:39
@W-A-James W-A-James self-assigned this Jul 10, 2024
@W-A-James W-A-James added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jul 10, 2024
@W-A-James W-A-James added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jul 10, 2024
@W-A-James W-A-James merged commit 04044d2 into main Jul 10, 2024
11 checks passed
@W-A-James W-A-James deleted the NODE-6253-use-system-ssl-by-default branch July 10, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
3 participants