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

Use the certificate provided by the node.js #301

Merged
merged 4 commits into from
Feb 25, 2023
Merged

Conversation

shibd
Copy link
Member

@shibd shibd commented Feb 24, 2023

Master Issue: #281

Motivation

After 1.8.0, we helped users precompile CPP, resulting in users not being able to obtain system certificates when using TLS-related functions.

Current usage issues list reference: https://github.com/BewareMyPower/pulsar-tls-examples

Node.js provides bundled Mozilla CA, which we can use by default.

Modifications

  • Use Node.js bundled Mozilla CA when tlsTrustCertsFilePath is empty.
  • Since the CPP client can't set cert content direct, we first need to write the certificate content to a file and pass in the path to the CPP client.

Verifying this change

You can clone the repository installation and verify:

git clone [email protected]:shibd/pulsar-client-tls-test.git
npm install
node tls-oauth2.js
node tls-token.js

The environment I currently verified passed is:

  • Windows x64
  • MacOS arm64
  • Ubuntu 20.04.5

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@shibd shibd self-assigned this Feb 24, 2023
@shibd shibd added this to the 1.9.0 milestone Feb 24, 2023
src/Client.js Outdated Show resolved Hide resolved
src/Client.js Outdated Show resolved Hide resolved
@BewareMyPower BewareMyPower requested a review from nodece February 24, 2023 10:18
@nodece
Copy link
Member

nodece commented Feb 24, 2023

I suggest we should built-in the cacert. In the install step, and the network is well, we can update this cert.

@shibd
Copy link
Member Author

shibd commented Feb 24, 2023

I suggest we should built-in the cacert. In the install step, and the network is well, we can update this cert.

Since Node.js already has built-in certificates, we don't need to make a copy in our repository, we just need to use it when installing.

index.js Outdated Show resolved Hide resolved
GenCertFile.js Outdated Show resolved Hide resolved
src/Client.js Outdated Show resolved Hide resolved
Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM, Great @shibd

@shibd shibd merged commit a89b64f into apache:master Feb 25, 2023
shibd added a commit that referenced this pull request Feb 25, 2023
* Use the certificate provided by the node.js.

* Gen cert file when npm install.

* Certificate generation is performed only once

* Fix code reviews.

(cherry picked from commit a89b64f)
@eolivelli
Copy link

eolivelli commented Feb 25, 2023

Great work!

I would like to reiterate that it is better that we don't bundle a CAcert file statically, in that case we would take responsibility on the contents of the files and to maintain the file. This is a burden that the Pulsar project couldn't bear.

The approach on this patch is perfect

}

static genCertFile() {
fs.rmSync(certsFilePath, { force: true });

Choose a reason for hiding this comment

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

What happens if you bootstrap two clients on the same machine?
Will they use the same file?

Also, we need to set permissions appropriately on this file, otherwise other users may override the contents and lead to a security hole

Cc @michaeljmarshall @lhotari @BewareMyPower

Copy link
Member Author

Choose a reason for hiding this comment

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

What happens if you bootstrap two clients on the same machine?
Will they use the same file?

The file is generated only once at installation time.
If pulsar-client is the global install, they will use the same file.
If pulsar-client is not installed globally, each project generates files and uses them separately.

Also, we need to set permissions appropriately on this file, otherwise other users may override the contents and lead to a security hole

How do I set permissions? I don't think it's necessary, just like the default /etc/ssl/cert.pem file in linux, users can override it too, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

The permission of ca-certificates.crt on Linux is:

$ ls -lh /etc/ssl/certs/ca-certificates.crt
-rw-r--r-- 1 root root 191K Feb  7 15:20 /etc/ssl/certs/ca-certificates.crt

But I don't think we need to change the permission. Because this file would be installed under node_modules/ in the project directory. The owner is who ran npm install. It would be that user's fault if the permission was overridden by others.

Copy link

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

@shibd can you please confirm that with this patch we are using the cacerts file provided by the user environment and we are not bundling it at build time?

@shibd
Copy link
Member Author

shibd commented Feb 25, 2023

we are using the cacerts file provided by the user environment

We are using the Mozilla CA file provided by Node.js

and we are not bundling it at build time?

Yes, when the user installed pulsar-client, we just went to get this certificate content by tls.rootCertificates and append a cert file.

BTW: Since the CPP client can't set cert content direct, we first need to write the certificate content to a file and pass in the path to the CPP client.

@nodece
Copy link
Member

nodece commented Feb 25, 2023

@shibd I think that we need to add the CAcerts notice to the README.md, so like:

CA certificates

We are using the Mozilla CA file provided by Node.js

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.

4 participants