GEODE-8655: handling exception on SNIHostName #5669

mkevo · 2020-10-26T11:24:13Z

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically develop)?
Is your initial contribution a single, squashed commit?
Does gradlew build run cleanly?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

DonalEvans

Is there a typo in the name of this PR? GEODE-8665 does not appear to be a ticket number that exists.

kohlmu-pivotal

@mkevo is there any test that would be covering/testing the changes you have made here?

bschuchardt · 2020-10-26T23:11:35Z

geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java

-    serverNames.add(new SNIHostName(hostName));
+    try {
+      serverNames.add(new SNIHostName(hostName));
+    } catch (Exception ex) {


If we can't establish the SNI field as requested shouldn't we throw an informative exception? If you throw an SSLException, for instance, ConnectionFactoryImpl will catch it and log it.

Hi @bschuchardt ,
What do you mean by throwing an informative exception?
Is it ok to log it in warn level?

throw new SSLException("unable to establish requested server name", ex)

I'm not sure that's actually necessary though - ConnectionFactoryImpl already catches the exception this PR is guarding and logs it. Do you have a failure that this PR is fixing?

This issue didn't exist before adding GEODE-7852.
I can't find it where it catches this exception from the ticket description and where it logs it. This catch is added to handle this exception:
java.lang.IllegalArgumentException: Contains non-LDH ASCII characters
I described a failure in the ticket, and need a help to write test for it as it is using ipv6 and TLS.
You can try to reproduce it locally. If you need more information to reproduce the issue please contact me via mail or slack.

I'm removing myself as a reviewer of this PR. I will not be working on Apache Geode in the near future and should not be blocking any PRs.

mkevo · 2020-10-27T06:16:36Z

Is there a typo in the name of this PR? GEODE-8665 does not appear to be a ticket number that exists.

Sorry, mistake.

mkevo · 2020-10-27T06:51:21Z

@mkevo is there any test that would be covering/testing the changes you have made here?

Hi @kohlmu-pivotal ,
I need a help with writing test, as I don't have an idea how to write test with using ipv6 address.
Also you can see manual steps in ticket description.

bschuchardt

You also need to include a test with this PR

bschuchardt

Thanks for correcting the ticket number. I can see this is concerning server-side endpoint verification now. I don't think we should catch and ignore that exception because we'd be silently ignoring the request to verify the server certificate host names. I can see there are many complaints on the internet about SNIHostName not accepting IPv6 numeric addresses while it does support IPv4 numeric addresses.

What we need is the ability to specify a hostname for the locator's bind-address in gfsh and have this string be preserved and passed into InternalLocator's startDistributedSystem() method. I tried that in GEODE-8144 but gave up as it required a lot of code change in gfsh/LocatorLauncher code. Instead I used getCanonicalHostName() if no bind-address is specified but this doesn't seem to be working in your environment.

We need to move the whole code base in the direction of preserving the hostname strings specified by operators and not turn them into InetAddresses. This is why we introduced the HostAndPort class. for instance.

mkevo · 2020-11-04T11:07:18Z

Hi @bschuchardt ,

I think that this will not also work on your laptop too, you just need to add ipv6 address to you machine and follow steps described in the ticket.
For now, I think it will be good to check if ipv6 is used, if yes ignore for it, until someone do this huge change in gfsh/LocatorLauncher as you mentioned.

onichols-pivotal · 2021-02-04T05:00:48Z

This PR has been inactive for some while, can it be closed?

mkevo · 2021-02-04T07:26:54Z

Hi,
I need some help there, as I don't know how to run test with ipv6 address.

bschuchardt

I am wondering if this PR is needed if we change SocketCreator to only establish a SNI hostname when necessary. @upthewaterspout changed the code to always establish a SNI hostname but we really only need it if a proxy hostname has been set, or if validation of the server's hostname has been requested. In both of those cases a non-numeric hostname ought to be available.

echobravopapa · 2021-02-04T18:33:38Z

@bschuchardt that approach might work, @mkevo do you have a test for this scenario you could share with us?

mkevo · 2021-02-05T08:07:49Z

@echobravopapa , I don't have a test, but these are manually steps:

change your machine to use ipv6 address. Steps for Ubuntu:

in /etc/hosts file change ipv4 to ipv6 address(you can find ipv6 address if you type ifconfig)

Create locator dir from where you will run the following command:
usr/lib/jvm/java-8-openjdk-amd64/jre/bin/java -server -classpath /home/apache-geode-1.14.0-build.0/lib/geode-core-1.14.0-build.0.jar:/home/apache-geode-1.14.0-build.0/lib/geode-dependencies.jar -Djava.net.preferIPv6Addresses=true -DgemfireSecurityPropertyFile=/home/geode-examples/clientSecurity/example_security.properties -Dgemfire.enable-cluster-configuration=true -Dgemfire.load-cluster-configuration-from-dir=false -Dgemfire.launcher.registerSignalHandlers=true -Djava.awt.headless=true -Dsun.rmi.dgc.server.gcInterval=9223372036854775806 org.apache.geode.distributed.LocatorLauncher start locator --port=10334
Set your values for java, jars and security file path.
Open gfsh from another window and run offline status command:
status locator --dir=/home/apache-geode-1.14.0-build.0/bin/locatorssl --security-properties-file=/home/geode-examples/clientSecurity/example_security.properties
Output will be:
Locator in /home/apache-geode-1.14.0-build.0/bin/locatorssl on mkevo-XPS-15-9570[10334] is currently not responding.

If you try with my proposal to fix it you will got in logs java.lang.IllegalArgumentException and status command will be successful. Without my fix command failed as it not handling this exception.
If you have any question, please feel free to contact me.

echobravopapa · 2021-02-10T00:40:04Z

Hello @mkevo may I ask you for more detailed steps to reproduce this? i.e. i'm attempting to intuitively follow the info you have above and I'm getting exceptions on step 2:
[error 2021/02/09 16:38:31.243 PST <main> tid=0x1] TcpClient.stop(): exception connecting to locator HostAndPort[/0:0:0:0:0:0:0:0:10334]java.lang.IllegalArgumentException: Contains non-LDH ASCII characters
I am running on Mac, but seems reasonable to expect similar outcomes...

mkevo · 2021-02-10T09:03:05Z

I think that the reason why you got the problem on the start locator command is that you didn't configured right ipv6 on your local machine. I will try to send you more detailed steps via mail.

echobravopapa · 2021-02-24T23:50:52Z

Hey @mkevo your last email got me to a similar reproduction... and I realized that you should be able to convert this use case into a test by using docker containers, have a look at /geode-assembly/src/acceptanceTest/java/org/apache/geode/client/sni/SingleServerSNIAcceptanceTest.java and supporting files/scripts...
I think a similar approach will allow you to create the test needed here.

onichols-pivotal · 2021-03-26T06:18:27Z

This PR has been open for some while with no recent activity. Please keep Geode tidy by closing PRs you're not actively using.

mkevo · 2021-03-26T06:46:01Z

@onichols-pivotal,
I'm still working on the test. It is hard to write a test with ipv6, so it takes some time.

Bill

Rejected for now. Waiting on a test.

Bill is taking over for Bruce for this PR

@Bill

@Bill is codeowner for this

onichols-pivotal · 2021-09-10T23:47:57Z

This PR has been untouched for several months, can it be closed?

mkevo · 2021-09-13T05:44:12Z

This PR has been untouched for several months, can it be closed?

No, need some help to write a test with ipv6.

kohlmu-pivotal · 2021-09-13T05:53:07Z

@echobravopapa @Bill @kamilla1201 is this something that you could help with?

onichols-pivotal · 2022-02-05T08:06:43Z

this PR appears to be abandoned, can it be closed?

mkevo · 2022-02-07T07:49:29Z

this PR appears to be abandoned, can it be closed?

Still waiting for some help for a test with Ipv6.

onichols-pivotal · 2022-02-07T09:19:58Z

Please rebase with latest develop to resolve conflicts and reach out to the dev list if you are stuck

DonalEvans reviewed Oct 26, 2020

View reviewed changes

kohlmu-pivotal requested review from bschuchardt and Bill October 26, 2020 20:47

kohlmu-pivotal previously requested changes Oct 26, 2020

View reviewed changes

bschuchardt requested changes Oct 26, 2020

View reviewed changes

mkevo changed the title ~~GEODE-8665: handling exception on SNIHostName~~ GEODE-8655: handling exception on SNIHostName Oct 27, 2020

bschuchardt requested changes Oct 27, 2020

View reviewed changes

bschuchardt previously requested changes Nov 2, 2020

View reviewed changes

bschuchardt reviewed Feb 4, 2021

View reviewed changes

bschuchardt self-requested a review April 27, 2021 16:12

Bill requested changes Apr 27, 2021

View reviewed changes

onichols-pivotal marked this pull request as draft May 4, 2021 02:31

GEODE-8665: handling exception on SNIHostName

bd275ae

mkevo force-pushed the feature/GEODE-8665 branch from 795169b to bd275ae Compare February 8, 2022 10:09

mkevo closed this Jun 2, 2022

mkevo deleted the feature/GEODE-8665 branch June 2, 2022 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-8655: handling exception on SNIHostName #5669

GEODE-8655: handling exception on SNIHostName #5669

mkevo commented Oct 26, 2020

DonalEvans left a comment

kohlmu-pivotal left a comment

bschuchardt Oct 26, 2020

mkevo Oct 27, 2020

bschuchardt Oct 27, 2020

bschuchardt Oct 27, 2020

mkevo Nov 2, 2020

bschuchardt Apr 27, 2021

mkevo commented Oct 27, 2020

mkevo commented Oct 27, 2020

bschuchardt left a comment

bschuchardt left a comment

mkevo commented Nov 4, 2020

onichols-pivotal commented Feb 4, 2021

mkevo commented Feb 4, 2021

bschuchardt left a comment

echobravopapa commented Feb 4, 2021

mkevo commented Feb 5, 2021

echobravopapa commented Feb 10, 2021

mkevo commented Feb 10, 2021

echobravopapa commented Feb 24, 2021 •

edited

Loading

onichols-pivotal commented Mar 26, 2021

mkevo commented Mar 26, 2021

Bill left a comment

onichols-pivotal commented Sep 10, 2021

mkevo commented Sep 13, 2021

kohlmu-pivotal commented Sep 13, 2021

onichols-pivotal commented Feb 5, 2022

mkevo commented Feb 7, 2022

onichols-pivotal commented Feb 7, 2022

GEODE-8655: handling exception on SNIHostName #5669

GEODE-8655: handling exception on SNIHostName #5669

Conversation

mkevo commented Oct 26, 2020

For all changes:

Note:

DonalEvans left a comment

Choose a reason for hiding this comment

kohlmu-pivotal left a comment

Choose a reason for hiding this comment

bschuchardt Oct 26, 2020

Choose a reason for hiding this comment

mkevo Oct 27, 2020

Choose a reason for hiding this comment

bschuchardt Oct 27, 2020

Choose a reason for hiding this comment

bschuchardt Oct 27, 2020

Choose a reason for hiding this comment

mkevo Nov 2, 2020

Choose a reason for hiding this comment

bschuchardt Apr 27, 2021

Choose a reason for hiding this comment

mkevo commented Oct 27, 2020

mkevo commented Oct 27, 2020

bschuchardt left a comment

Choose a reason for hiding this comment

bschuchardt left a comment

Choose a reason for hiding this comment

mkevo commented Nov 4, 2020

onichols-pivotal commented Feb 4, 2021

mkevo commented Feb 4, 2021

bschuchardt left a comment

Choose a reason for hiding this comment

echobravopapa commented Feb 4, 2021

mkevo commented Feb 5, 2021

echobravopapa commented Feb 10, 2021

mkevo commented Feb 10, 2021

echobravopapa commented Feb 24, 2021 • edited Loading

onichols-pivotal commented Mar 26, 2021

mkevo commented Mar 26, 2021

Bill left a comment

Choose a reason for hiding this comment

onichols-pivotal commented Sep 10, 2021

mkevo commented Sep 13, 2021

kohlmu-pivotal commented Sep 13, 2021

onichols-pivotal commented Feb 5, 2022

mkevo commented Feb 7, 2022

onichols-pivotal commented Feb 7, 2022

echobravopapa commented Feb 24, 2021 •

edited

Loading