You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/07/22 02:04:00 UTC
[GitHub] [geode] kamilla1201 opened a new pull request #6714: [DRAFT] GEODE-9396
kamilla1201 opened a new pull request #6714:
URL: https://github.com/apache/geode/pull/6714
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](http://www.apache.org/legal/resolved.html#category-a)?
### 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.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] upthewaterspout commented on pull request #6714: GEODE-9396: Upgrades using SSL fail with mismatch of hostname in certificates
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on pull request #6714:
URL: https://github.com/apache/geode/pull/6714#issuecomment-956863658
> I'm wondering if someone would have a reason to specify an IP address as the bind address that this might break. Cases I could think about:
>
> * Maybe there is no DNS name? The call to `getHost()` might fail
>
> * Maybe the DNS server isn't updated at the time the member starts. This might be the case in a IaaS where VMs or containers are launched as soon as the gemfire process starts. I guess for this case an old member would be less likely to not be resolvable?
>
>
> I'm not sure if this should reject this approach or not, if there are no better options. Or maybe we just failures from getHost and retain the IP address.
I wonder if it would be possible to fall back to the old logic for hostname validation if the member is from an old member, rather than changing the bind address when deserializing. Would that be doable or any safer?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kamilla1201 closed pull request #6714: GEODE-9396: Upgrades using SSL fail with mismatch of hostname in certificates
Posted by GitBox <gi...@apache.org>.
kamilla1201 closed pull request #6714:
URL: https://github.com/apache/geode/pull/6714
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #6714: GEODE-9396: Upgrades using SSL fail with mismatch of hostname in certificates
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6714:
URL: https://github.com/apache/geode/pull/6714#discussion_r708741605
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java
##########
@@ -455,6 +455,12 @@ public void readExternal(ObjectInput in) throws IOException, ClassNotFoundExcept
durableClientAttributes = null;
}
+ @Override
+ public void toDataPre_GEODE_1_15_0_0(DataOutput out, SerializationContext context)
+ throws IOException {
+ memberIdentifier.toDataPre_GEODE_1_15_0_0(out, context);
+ }
+
Review comment:
sorry to be nitpicky but would you mind moving `toData()` so it's before each of the version-specific to-data methods. The to-data methods should be ordered in the source file from newest to oldest. I believe that's the pattern established in other DSFIDs.
Ordering them consistently helps maintainers.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kamilla1201 commented on a change in pull request #6714: GEODE-9396: Upgrades using SSL fail with mismatch of hostname in certificates
Posted by GitBox <gi...@apache.org>.
kamilla1201 commented on a change in pull request #6714:
URL: https://github.com/apache/geode/pull/6714#discussion_r733848832
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -693,10 +701,43 @@ public void fromData(DataInput in,
// nope - it's from a pre-GEODE client or WAN site
}
}
+ convertIpToHostnameIfNeeded();
+ }
+
+ public void fromDataPre_GEODE_1_15_0_0(DataInput in, DeserializationContext context)
+ throws IOException, ClassNotFoundException {
+ readMemberData(in);
+ memberData.readAdditionalData(in);
+ convertIpToHostnameIfNeeded();
}
public void fromDataPre_GFE_9_0_0_0(DataInput in, DeserializationContext context)
throws IOException {
+ readMemberData(in);
+ convertIpToHostnameIfNeeded();
+ }
+
+ /**
+ * In older versions of Geode {@link MemberData#getHostName()} can return an IP address
+ * if network partition is enabled. Also, older versions of Geode don't use bind-address for
+ * SNI endpoint identification and do a reverse lookup to find the fully qualified hostname.
+ * This can become a problem if TLS certificate doesn't have the fully qualified name in it
+ * as a Subject Alternate Name, therefore this behavior was changed to preserve the bind-address.
+ *
+ * During a rolling upgrade, we need to ensure that {@link MemberData#getHostName()} returns
+ * a hostname because it might be later used for SNI endpoint identification.
+ */
+ private void convertIpToHostnameIfNeeded() {
+ if (memberData.isNetworkPartitionDetectionEnabled()) {
Review comment:
It would be good to check if SSL endpoint identification is enabled but there's no easy way to do that in geode-membership. We could probably grab the setting from a global and somehow make it accessible to the `fromData() `through the `DeserializationContext`, but `DeserializationContext` doesn't seem to be a good place to store this information. I think that this code should work even without this check. If SSL endpoint identification is enabled, users shouldn't use an IP address as a Subject Alternative Name in their certificate. If it's disabled, it shouldn't matter if there's a hostname or an IP address in the `hostname` field.
After running some tests I realized that we probably shouldn't check if network partition is enabled either because this setting only matters when we upgrade from a version without this fix to a version with this fix. Before GEODE-9139, `memberData.hostname` contained an IP address if network partition was enabled. But GEODE-9139 changed this behavior so that `hostname` contains either bind-address (if specified) or an FQDN (regardless of network partition setting). So if we keep this check, the behavior of `convertIpToHostnameIfNeeded` will differ depending on the version we're upgrading from, and I think we should avoid that.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kamilla1201 commented on pull request #6714: GEODE-9396: Upgrades using SSL fail with mismatch of hostname in certificates
Posted by GitBox <gi...@apache.org>.
kamilla1201 commented on pull request #6714:
URL: https://github.com/apache/geode/pull/6714#issuecomment-971793482
Closing this PR because we decided to implement the fix that Dan suggested.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] upthewaterspout commented on pull request #6714: GEODE-9396: Upgrades using SSL fail with mismatch of hostname in certificates
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on pull request #6714:
URL: https://github.com/apache/geode/pull/6714#issuecomment-956863658
> I'm wondering if someone would have a reason to specify an IP address as the bind address that this might break. Cases I could think about:
>
> * Maybe there is no DNS name? The call to `getHost()` might fail
>
> * Maybe the DNS server isn't updated at the time the member starts. This might be the case in a IaaS where VMs or containers are launched as soon as the gemfire process starts. I guess for this case an old member would be less likely to not be resolvable?
>
>
> I'm not sure if this should reject this approach or not, if there are no better options. Or maybe we just failures from getHost and retain the IP address.
I wonder if it would be possible to fall back to the old logic for hostname validation if the member is from an old member, rather than changing the bind address when deserializing. Would that be doable or any safer?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kamilla1201 commented on pull request #6714: GEODE-9396: Upgrades using SSL fail with mismatch of hostname in certificates
Posted by GitBox <gi...@apache.org>.
kamilla1201 commented on pull request #6714:
URL: https://github.com/apache/geode/pull/6714#issuecomment-957881600
@upthewaterspout If there’s no DNS, then `getHost()` should return the textual representation of an IP address. `getHost()` calls `InetAddress.getCanonicalHostName()` which is a best effort method, it should return an IP address if there is no DNS name. I’m not sure about the second scenario you described, but it sounds unlikely.
> I wonder if it would be possible to fall back to the old logic for hostname validation if the member is from an old member, rather than changing the bind address when deserializing. Would that be doable or any safer?
I think we could do that in `Connection.createIoFilter()`, by replacing this:
```
if (remoteAddr != null) {
hostName = remoteAddr.getHostName();
} else {
hostName = SocketCreator.getHostName(address.getAddress());
}
```
with something like:
```
if (remoteAddr != null) {
if (remoteAddr.getVersion().isOlderThan(KnownVersion.GEODE_1_15_0)) {
hostName = address.getHostString();
if (owner.getDM().getConfig().getSSLEndPointIdentificationEnabled()
&& InetAddressValidator.getInstance().isValid(hostName)) {
// attempt to get a hostname instead of the proffered numeric address
try {
hostName = InetAddress.getByName(hostName).getCanonicalHostName();
} catch (UnknownHostException e) {
// ignore - we'll see what happens with endpoint validation using a numeric address
}
}
} else {
hostName = remoteAddr.getHostName();
}
} else {
hostName = SocketCreator.getHostName(address.getAddress());
}
```
Here we know whether endpoint identification is enabled, which should help us avoid problems with IP numbers.
But I’m not sure whether this code will work correctly if we decide to back-port this change (along with GEODE-9139).
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] upthewaterspout commented on pull request #6714: GEODE-9396: Upgrades using SSL fail with mismatch of hostname in certificates
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on pull request #6714:
URL: https://github.com/apache/geode/pull/6714#issuecomment-956863658
> I'm wondering if someone would have a reason to specify an IP address as the bind address that this might break. Cases I could think about:
>
> * Maybe there is no DNS name? The call to `getHost()` might fail
>
> * Maybe the DNS server isn't updated at the time the member starts. This might be the case in a IaaS where VMs or containers are launched as soon as the gemfire process starts. I guess for this case an old member would be less likely to not be resolvable?
>
>
> I'm not sure if this should reject this approach or not, if there are no better options. Or maybe we just failures from getHost and retain the IP address.
I wonder if it would be possible to fall back to the old logic for hostname validation if the member is from an old member, rather than changing the bind address when deserializing. Would that be doable or any safer?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kamilla1201 commented on pull request #6714: GEODE-9396: Upgrades using SSL fail with mismatch of hostname in certificates
Posted by GitBox <gi...@apache.org>.
kamilla1201 commented on pull request #6714:
URL: https://github.com/apache/geode/pull/6714#issuecomment-957881600
@upthewaterspout If there’s no DNS, then `getHost()` should return the textual representation of an IP address. `getHost()` calls `InetAddress.getCanonicalHostName()` which is a best effort method, it should return an IP address if there is no DNS name. I’m not sure about the second scenario you described, but it sounds unlikely.
> I wonder if it would be possible to fall back to the old logic for hostname validation if the member is from an old member, rather than changing the bind address when deserializing. Would that be doable or any safer?
I think we could do that in `Connection.createIoFilter()`, by replacing this:
```
if (remoteAddr != null) {
hostName = remoteAddr.getHostName();
} else {
hostName = SocketCreator.getHostName(address.getAddress());
}
```
with something like:
```
if (remoteAddr != null) {
if (remoteAddr.getVersion().isOlderThan(KnownVersion.GEODE_1_15_0)) {
hostName = address.getHostString();
if (owner.getDM().getConfig().getSSLEndPointIdentificationEnabled()
&& InetAddressValidator.getInstance().isValid(hostName)) {
// attempt to get a hostname instead of the proffered numeric address
try {
hostName = InetAddress.getByName(hostName).getCanonicalHostName();
} catch (UnknownHostException e) {
// ignore - we'll see what happens with endpoint validation using a numeric address
}
}
} else {
hostName = remoteAddr.getHostName();
}
} else {
hostName = SocketCreator.getHostName(address.getAddress());
}
```
Here we know whether endpoint identification is enabled, which should help us avoid problems with IP numbers.
But I’m not sure whether this code will work correctly if we decide to back-port this change (along with GEODE-9139).
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kamilla1201 commented on pull request #6714: GEODE-9396: Upgrades using SSL fail with mismatch of hostname in certificates
Posted by GitBox <gi...@apache.org>.
kamilla1201 commented on pull request #6714:
URL: https://github.com/apache/geode/pull/6714#issuecomment-957881600
@upthewaterspout If there’s no DNS, then `getHost()` should return the textual representation of an IP address. `getHost()` calls `InetAddress.getCanonicalHostName()` which is a best effort method, it should return an IP address if there is no DNS name. I’m not sure about the second scenario you described, but it sounds unlikely.
> I wonder if it would be possible to fall back to the old logic for hostname validation if the member is from an old member, rather than changing the bind address when deserializing. Would that be doable or any safer?
I think we could do that in `Connection.createIoFilter()`, by replacing this:
```
if (remoteAddr != null) {
hostName = remoteAddr.getHostName();
} else {
hostName = SocketCreator.getHostName(address.getAddress());
}
```
with something like:
```
if (remoteAddr != null) {
if (remoteAddr.getVersion().isOlderThan(KnownVersion.GEODE_1_15_0)) {
hostName = address.getHostString();
if (owner.getDM().getConfig().getSSLEndPointIdentificationEnabled()
&& InetAddressValidator.getInstance().isValid(hostName)) {
// attempt to get a hostname instead of the proffered numeric address
try {
hostName = InetAddress.getByName(hostName).getCanonicalHostName();
} catch (UnknownHostException e) {
// ignore - we'll see what happens with endpoint validation using a numeric address
}
}
} else {
hostName = remoteAddr.getHostName();
}
} else {
hostName = SocketCreator.getHostName(address.getAddress());
}
```
Here we know whether endpoint identification is enabled, which should help us avoid problems with IP numbers.
But I’m not sure whether this code will work correctly if we decide to back-port this change (along with GEODE-9139).
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #6714: GEODE-9396: Upgrades using SSL fail with mismatch of hostname in certificates
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6714:
URL: https://github.com/apache/geode/pull/6714#discussion_r708752232
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -693,10 +701,43 @@ public void fromData(DataInput in,
// nope - it's from a pre-GEODE client or WAN site
}
}
+ convertIpToHostnameIfNeeded();
+ }
+
+ public void fromDataPre_GEODE_1_15_0_0(DataInput in, DeserializationContext context)
+ throws IOException, ClassNotFoundException {
+ readMemberData(in);
+ memberData.readAdditionalData(in);
+ convertIpToHostnameIfNeeded();
}
public void fromDataPre_GFE_9_0_0_0(DataInput in, DeserializationContext context)
throws IOException {
+ readMemberData(in);
+ convertIpToHostnameIfNeeded();
+ }
+
+ /**
+ * In older versions of Geode {@link MemberData#getHostName()} can return an IP address
+ * if network partition is enabled. Also, older versions of Geode don't use bind-address for
+ * SNI endpoint identification and do a reverse lookup to find the fully qualified hostname.
+ * This can become a problem if TLS certificate doesn't have the fully qualified name in it
+ * as a Subject Alternate Name, therefore this behavior was changed to preserve the bind-address.
Review comment:
Second sentence confuses me. What do you think of this:
In older versions of Geode {@link MemberData#getHostName()} can return an IP address if network partition detection is enabled. If SSL endpoint identification is enabled, those product versions supply the result of a reverse lookup to the TLS handshake API. Endpoint identification will fail if e.g. the lookup returned a fully-qualified name but the certificate had just a (non-fully-qualified) hostname in a Subject Alternate Name field.
In version 1.15.0 member identifiers were changed so that if a bind address is specified, that exact string will be carried as the host name. That gives the administrator better control over endpoint identification. When upgrading from earlier versions we convert any IP numbers to hostnames via reverse lookup here.
##########
File path: geode-core/src/integrationTest/resources/org/apache/geode/codeAnalysis/sanctionedDataSerializables.txt
##########
@@ -1749,18 +1751,10 @@ org/apache/geode/internal/cache/tier/sockets/ObjectPartList,2
fromData,148
toData,201
-org/apache/geode/internal/cache/tier/sockets/ObjectPartList651,2
-fromData,165
-toData,223
Review comment:
ah ok 👍 `ObjectPartList651` isn't in the project any more. Ditto `SerializedObjectPartList` below.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java
##########
@@ -455,6 +455,12 @@ public void readExternal(ObjectInput in) throws IOException, ClassNotFoundExcept
durableClientAttributes = null;
}
+ @Override
+ public void toDataPre_GEODE_1_15_0_0(DataOutput out, SerializationContext context)
+ throws IOException {
+ memberIdentifier.toDataPre_GEODE_1_15_0_0(out, context);
+ }
+
Review comment:
sorry to be nitpicky but would you mind putting `toData()` before each of the version-specific to-data methods. And then order the to-data methods from newest to oldest. I believe that's the pattern established in other DSFIDs.
Ordering them consistently helps maintainers.
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -693,10 +701,43 @@ public void fromData(DataInput in,
// nope - it's from a pre-GEODE client or WAN site
}
}
+ convertIpToHostnameIfNeeded();
+ }
+
+ public void fromDataPre_GEODE_1_15_0_0(DataInput in, DeserializationContext context)
+ throws IOException, ClassNotFoundException {
+ readMemberData(in);
+ memberData.readAdditionalData(in);
+ convertIpToHostnameIfNeeded();
}
public void fromDataPre_GFE_9_0_0_0(DataInput in, DeserializationContext context)
throws IOException {
+ readMemberData(in);
+ convertIpToHostnameIfNeeded();
+ }
+
+ /**
+ * In older versions of Geode {@link MemberData#getHostName()} can return an IP address
+ * if network partition is enabled. Also, older versions of Geode don't use bind-address for
+ * SNI endpoint identification and do a reverse lookup to find the fully qualified hostname.
+ * This can become a problem if TLS certificate doesn't have the fully qualified name in it
+ * as a Subject Alternate Name, therefore this behavior was changed to preserve the bind-address.
+ *
+ * During a rolling upgrade, we need to ensure that {@link MemberData#getHostName()} returns
+ * a hostname because it might be later used for SNI endpoint identification.
+ */
+ private void convertIpToHostnameIfNeeded() {
+ if (memberData.isNetworkPartitionDetectionEnabled()) {
Review comment:
should we be guarding also on (SSL endpoint identification is enabled)?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org