You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "shibd (via GitHub)" <gi...@apache.org> on 2023/02/22 14:54:55 UTC
[GitHub] [pulsar-client-node] shibd opened a new pull request, #300: [fix] MacOS arm64 tls get certs failed.
shibd opened a new pull request, #300:
URL: https://github.com/apache/pulsar-client-node/pull/300
### Motivation
#281
MacOS arm64 NAPI is cross-compiled through MacOS x86's runner, the path to cacert is not automatically recognized during compilation.
### Modifications
- Specify the certs path when compiling `curl` and `openSSL`.
### Verifying this change
- I reproduced the issue on my local machine and used the [package](https://github.com/shibd/pulsar-client-node/suites/11135154708/artifacts/567885331) to test the pass.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on pull request #300: [fix] MacOS arm64 tls get certs failed.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1441304238
> export CURL_CA_BUNDLE="/etc/ssl/cert.pem"
It does not work because this environment variable only works for the `curl` binary. The libcurl library does not read the environment variable for CA cert store.
I didn't verify `SSL_CERT_DIR` yet, but there is no reason that the libopenssl library will read this environment variable. Even yes, it only specifies the directory, the cert file name could be different. For example, it's `ca-certificates.crt` on Ubuntu and `ca-bundle.crt` on CentOS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on pull request #300: [fix] MacOS arm64 tls get certs failed.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1441227560
This PR is okay but I think it does not essentially solve the issue. With this PR, the macOS users might feel good. But for Linux users, they might still have to configure the `tlsTrustCertsFilePath`.
For example, after https://github.com/apache/pulsar-client-node/pull/288, which is included in Node.js client 1.8.1, the Node.js client was built on CentOS 7 for glibc based Linux. The bundled CA path is `/etc/pki/tls/certs/ca-bundle.crt`. So when I ran an example on Ubuntu 20.04, I have to add the following config:
```javascript
tlsTrustCertsFilePath: '/etc/ssl/certs/ca-certificates.crt'
```
However, with Node.js client 1.8.0, which was built on Node.js container that is based on Debian, the bundled CA path is `/etc/ssl/certs/ca-certificates.crt` so I didn't need to configure `tlsTrustCertsFilePath`. Of course, it does not work on RedHat based Linux distros.
In conclusion, it's a trade-off of the convenience to install the client without building the C++ add-on locally. Users could also solve this issue by copying the cert to a certain directory without any code change.
I might move the discussion to mail list later. But for now it's also good to hear your opinions. @eolivelli @nicoloboschi @shibd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on pull request #300: [fix] MacOS tls get certs failed.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1441481741
I've tested the Node.js client on Windows, it still cannot find the default CA cert file. But installing the Node.js client 1.7.0 or earlier is really hard and I cannot verify it at the moment.
My conclusion is that it should not block this release if there is no good solution. I'd rather like to document the default CA cert location for different systems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on pull request #300: [fix] MacOS tls get certs failed.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1441456029
> Is there a way to release two versions of the package? One with statically linked libraries and one with dynamic linking?
If we linked to OpenSSL dynamically, we have to link other dependencies dynamically too. It would require users to install all the dependencies (e.g. Boost, Protobuf, etc.) into their systems. At the moment, I cannot think of a way to fix this issue.
I'm wondering if we can get the default CA cert bundle location via the Node.js code and then set it as the default `tlsTrustCertsFilePath` config?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] eolivelli commented on pull request #300: [fix] MacOS arm64 tls get certs failed.
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1440223092
one thing I don't understand here is that there error was reported by @nicoloboschi on a Intel based Mac and not on an arm64
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on pull request #300: [fix] MacOS arm64 tls get certs failed.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1440272453
> one thing I don't understand here is that there error was reported by @nicoloboschi on a Intel based Mac and not on an arm64
@nicoloboschi Can you help confirm this?
And can your pre verify this pr?
1. Download and unzip NAPI. [MacOS x86_64](https://github.com/shibd/pulsar-client-node/suites/11136563235/artifacts/567980911) or [MacOS arm64](https://github.com/shibd/pulsar-client-node/suites/11136563235/artifacts/567980910)
2. Move `pulsar.node` to your project `node_modules/pulsar-client/lib/binding/`
3. Run you test code.
4. The first time running, you will be receive alerted macOS system.
<img width="269" alt="image" src="https://user-images.githubusercontent.com/33416836/220673073-ff9008f0-d9ee-4a8b-81d7-4ca942753527.png">
<img width="694" alt="image" src="https://user-images.githubusercontent.com/33416836/220673515-9062c5bf-7964-44a2-ac52-c7ee4f17d261.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on pull request #300: [fix] MacOS arm64 tls get certs failed.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1440269156
Boost calls `SSL_CTX_set_default_verify_paths` (from libopenssl) to determine the default CA cert path. I guess it's because the OpenSSL that is built from source does not have the correct path?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on pull request #300: [fix] MacOS tls get certs failed.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1442726605
> I'm wondering if we can get the default CA cert bundle location via the Node.js code and then set it as the default tlsTrustCertsFilePath config?
Eventually I found Python client fixes this issue with the same way: https://github.com/apache/pulsar-client-python/blob/9b3c55f17adfe6b5a3fa7b016c94d687338c54da/pulsar/__init__.py#L458-L460
Node.js client also provides [an API](https://nodejs.org/api/tls.html#tlsrootcertificates) to use its bundled certificates, instead of using system's certificates. After discussing with @shibd, we're going to adopt this way. So I closed this PR first. After all, we can verify the release by following https://lists.apache.org/thread/xj9ypz8lnoxwgjgks7lsz3hxwvnw5kww
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower closed pull request #300: [fix] MacOS tls get certs failed.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower closed pull request #300: [fix] MacOS tls get certs failed.
URL: https://github.com/apache/pulsar-client-node/pull/300
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on pull request #300: [fix] MacOS arm64 tls get certs failed.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1441270452
> In conclusion, it's a trade-off of the convenience to install the client without building the C++ add-on locally. Users could also solve this issue by copying the cert to a certain directory without any code change.
Agreed, I think we try to match the system to the certification path. If it really doesn't work, the user can solve it by following ways:
- Copy the certificate to the correct path
- Setting the environment variable:
```
export SSL_CERT_DIR="/etc/ssl/"
export CURL_CA_BUNDLE="/etc/ssl/cert.pem"
```
- Setting the `tlsTrustCertsFilePath ` configuration.
This PR solves the problem with paths under MacOS, and I don't think we should block the release of 1.8.1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on pull request #300: [fix] MacOS arm64 tls get certs failed.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1440261986
I agree that we might need to add a simple check in CI. I'm not sure if this fix works. Because the SSL handshake error was reported from Boost, not libcurl. I think we need to make Boost depend on a correct OpenSSL to get default verify paths via the `set_default_verify_paths` method:
https://github.com/apache/pulsar-client-cpp/blob/92c182acf7d1212218cb67e315656a23ee32a904/lib/ClientConnection.cc#L216
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on pull request #300: [fix] MacOS arm64 tls get certs failed.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1440276357
> I'm not sure if this fix works. Because the SSL handshake error was reported from Boost, not libcurl. I think we need to make Boost depend on a correct OpenSSL to get default verify paths via the set_default_verify_paths method:
https://github.com/apache/pulsar-client-node/pull/300/files#diff-33eaabd27404f12df463ee1983d2646944310824dcedc87f20e832463fdaef4eR67
@BewareMyPower This change will tell OpenSSL the correct certs path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] shibd commented on pull request #300: [fix] MacOS tls get certs failed.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1441417669
> Is there a way to release two versions of the package? One with statically linked libraries and one with dynamic linking?
If using dynamic linking, need to install the CPP client first. This falls back to 1.7.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] nicoloboschi commented on pull request #300: [fix] MacOS arm64 tls get certs failed.
Posted by "nicoloboschi (via GitHub)" <gi...@apache.org>.
nicoloboschi commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1440288430
> > one thing I don't understand here is that there error was reported by @nicoloboschi on a Intel based Mac and not on an arm64
>
> @nicoloboschi Can you help confirm this?
>
> And can your pre verify this pr?
>
> 1. Download and unzip NAPI. [MacOS x86_64](https://github.com/shibd/pulsar-client-node/suites/11136563235/artifacts/567980911) or [MacOS arm64](https://github.com/shibd/pulsar-client-node/suites/11136563235/artifacts/567980910)
> 2. Move `pulsar.node` to your project `node_modules/pulsar-client/lib/binding/`
> 3. Run you test code.
> 4. The first time running, you will be receive alerted macOS system.
>
> <img alt="image" width="269" src="https://user-images.githubusercontent.com/33416836/220673073-ff9008f0-d9ee-4a8b-81d7-4ca942753527.png">
>
> <img alt="image" width="694" src="https://user-images.githubusercontent.com/33416836/220673515-9062c5bf-7964-44a2-ac52-c7ee4f17d261.png">
I confirm this fix is effective and it worked in my tests. Good work!
I still think that a proper check is needed in the CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on pull request #300: [fix] MacOS arm64 tls get certs failed.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1441313763
> I don't think we should block the release of 1.8.1
Totally I agree. But the comment [here](https://lists.apache.org/thread/pr9921vflxg96yj0vf870rqptgg05wz1) from @nicoloboschi deserves the discussion.
> I wonder why it wasn't needed in 1.7.0 and now it's required in 1.8.1.
> This can be problematic for users because the client upgrade requires code
> changes and redeployment.
The new way of installing the Node.js client brings the requirement of code changes. I think the only way is to copying the cert file to correct paths. Then I think we must document it well that which cert file path should users configure for these systems:
- Windows
- Linux (including several distributions)
- macOS
It's a breaking change of 1.8.0, we'd better fix it since we've already realized it. If we cannot fix it, we should document the workaround.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-node] eolivelli commented on pull request #300: [fix] MacOS arm64 tls get certs failed.
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #300:
URL: https://github.com/apache/pulsar-client-node/pull/300#issuecomment-1441318036
Is there a way to release two versions of the package? Once we statically linked libraries and one with dynamic linking?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org