You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/05/29 04:56:32 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #15824: Switch to rely on Netty for Hostname Verification
michaeljmarshall opened a new pull request, #15824:
URL: https://github.com/apache/pulsar/pull/15824
### Motivation
Currently, we perform hostname verification for the Java Client and the Proxy Java Client using a custom Pulsar hostname verifier. In order to simplify the code, I propose that we refactor these clients so they rely on a Netty, its SslHandler, and the JVM, to perform the hostname verification.
When `HTTPS` is configured as the endpoint verification algorithm, it uses [RFC 2818](https://datatracker.ietf.org/doc/html/rfc2818) to perform hostname verification. This is defined by the `Java Security Standard Algorithm Names` documentation for JDK versions 8, 11, and 17. Here are the official docs:
* https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html
* https://docs.oracle.com/en/java/javase/11/docs/specs/security/standard-names.html
* https://docs.oracle.com/en/java/javase/17/docs/specs/security/standard-names.html
### Modifications
* Update the Java Client so that it configures the SslHandler's SslEngine to use `HTTPS` for endpoint verification and remove unnecessary custom hostname verification logic.
* Update the Proxy's `DirectProxyHandler` class so that it configures the SslHandler to perform hostname verification and so that the proxy handler itself does not perform that verification.
* Make it possible to disable hostname verification checking in the `HttpClient` used by the HTTP Lookup Client code in the Java Client. Currently, it defaults to being always enabled.
### Verifying this change
There are tests that already cover the changes, and I performed integration testing on a minikube cluster with Cert-Manager created certs.
### Does this pull request potentially affect one of the following parts:
This change deprecates support for CN matching in the Pulsar Java Client. A future change should remove this support from the Pulsar Admin Client, which relies on Pulsar's verifier that still supports CN matching.
### Documentation
There is no need for documentation. This is an internal change.
--
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] lhotari commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1142203396
@michaeljmarshall I'll rebase this PR and include my changes to fix the tests. I hope this is fine.
--
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] eolivelli commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1140465301
/pulsarbot rerun-failure-checks
--
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] github-actions[bot] commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1140377372
@michaeljmarshall:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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] github-actions[bot] commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1242627880
@michaeljmarshall Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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] lhotari commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1142211906
@michaeljmarshall It seems that when you opened the PR you didn't grant access to others for collaborating in the PR. I pushed changes to my fork in https://github.com/apache/pulsar/compare/master...lhotari:use-netty .
one way to make your branch to match the changes:
```
git pull https://github.com/lhotari/pulsar.git use-netty
git reset --hard FETCH_HEAD
```
--
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] lhotari commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1140850413
There are 2 test failures:
* org.apache.pulsar.client.api.AuthenticationTlsHostnameVerificationTest ► testTlsSyncProducerAndConsumerWithInvalidBrokerHost
* org.apache.pulsar.proxy.server.ProxyWithAuthorizationTest ►
testTlsHostVerificationProxyToClient
I debugged and fixed the first one. The test was invalid.
I pushed the commit to my fork: https://github.com/lhotari/pulsar/commit/7c9545ec
One observation is that when the handshake fails because of hostname verification, the client will keep on retrying for some time. Here's an example log: https://gist.github.com/lhotari/8455fc155e0c7526398f36e9bd0bd88c
(can be reproduced by running AuthenticationTlsHostnameVerificationTest with the above commit)
--
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] github-actions[bot] commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1140377368
@michaeljmarshall:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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] michaeljmarshall commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1143110376
> LGTM
>
> I have a quest:
>
> > If a subjectAltName extension of type dNSName is present, that MUST
> > be used as the identity. Otherwise, the (most specific) Common Name
> > field in the Subject field of the certificate MUST be used. Although
> > the use of the Common Name is existing practice, it is deprecated and
> > Certification Authorities are encouraged to use the dNSName instead.
>
> Do your changes follow up on this rule?
@nodece - I did not test this case, but the Java Naming Spec indicates that the hostname verifier will follow RFC 2818, so I assume that it should conform to the portion of the spec you quoted.
--
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] lhotari merged pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
lhotari merged PR #15824:
URL: https://github.com/apache/pulsar/pull/15824
--
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] github-actions[bot] commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1140377371
@michaeljmarshall:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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] lhotari commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1141340456
I got both failing tests fixed. I pushed my change to my fork and I'm running tests there: https://github.com/lhotari/pulsar/pull/72
--
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] michaeljmarshall commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1142217228
@lhotari thank you for finding and fixing those tests! I just gave maintainers access to collaborate. If you’re no longer available to push, I’ll get the commits in soon.
--
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] eolivelli commented on pull request #15824: Switch to rely on Netty for Hostname Verification
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #15824:
URL: https://github.com/apache/pulsar/pull/15824#issuecomment-1140388832
/pulsarbot rerun-failure-checks
--
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