You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jinmei Liao <ji...@pivotal.io> on 2017/08/03 21:15:10 UTC
Review Request 61417: GEODE-3328: adding ssl-truststore-type to the
config
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61417/
-----------------------------------------------------------
Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, Patrick Rhomberg, and Udo Kohlmeyer.
Repository: geode
Description
-------
GEODE-3328: adding ssl-truststore-type to the config
this is what's requested in the JIRA ticket. This changeset just deals with adding the property into gemfire properties
Diffs
-----
geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java 63f6505101f6edb62167b854d3d16d76d0a893cd
geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java 795f6a5a4a4f42fe065c9ca6013fd5598f9311d8
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java c2a395de0bfe21ed8efeb6b25e331f7085b3bf4f
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java fbe894c96447b2e30594eb2ed0652dd08e1028ce
geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java f86f07eb5e58b8509e909b7748795530efd65339
geode-core/src/main/java/org/apache/geode/management/GemFireProperties.java 08fa9b54ea066b4158478ae89d8295ed0b1a337b
geode-core/src/main/java/org/apache/geode/management/internal/beans/BeanUtilFuncs.java 499ef010f354ebf88765190f1b5eb945da83accc
geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java 525f988cd3cb4f19872168df9b7105645f5c0498
Diff: https://reviews.apache.org/r/61417/diff/1/
Testing
-------
precheckin running
Thanks,
Jinmei Liao
Re: Review Request 61417: GEODE-3328: adding ssl-truststore-type to
the config
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61417/#review182233
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
Lines 691 (patched)
<https://reviews.apache.org/r/61417/#comment258115>
Is this declaration needed? It doesn't appear to be used anywhere
- Ken Howe
On Aug. 3, 2017, 9:15 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61417/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2017, 9:15 p.m.)
>
>
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, Patrick Rhomberg, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-3328: adding ssl-truststore-type to the config
>
> this is what's requested in the JIRA ticket. This changeset just deals with adding the property into gemfire properties
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java 63f6505101f6edb62167b854d3d16d76d0a893cd
> geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java 795f6a5a4a4f42fe065c9ca6013fd5598f9311d8
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java c2a395de0bfe21ed8efeb6b25e331f7085b3bf4f
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java fbe894c96447b2e30594eb2ed0652dd08e1028ce
> geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java f86f07eb5e58b8509e909b7748795530efd65339
> geode-core/src/main/java/org/apache/geode/management/GemFireProperties.java 08fa9b54ea066b4158478ae89d8295ed0b1a337b
> geode-core/src/main/java/org/apache/geode/management/internal/beans/BeanUtilFuncs.java 499ef010f354ebf88765190f1b5eb945da83accc
> geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java 525f988cd3cb4f19872168df9b7105645f5c0498
>
>
> Diff: https://reviews.apache.org/r/61417/diff/1/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 61417: GEODE-3328: adding ssl-truststore-type to
the config
Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61417/#review182230
-----------------------------------------------------------
Ship it!
Ship It!
- Jared Stewart
On Aug. 3, 2017, 9:15 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61417/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2017, 9:15 p.m.)
>
>
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, Patrick Rhomberg, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-3328: adding ssl-truststore-type to the config
>
> this is what's requested in the JIRA ticket. This changeset just deals with adding the property into gemfire properties
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java 63f6505101f6edb62167b854d3d16d76d0a893cd
> geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java 795f6a5a4a4f42fe065c9ca6013fd5598f9311d8
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java c2a395de0bfe21ed8efeb6b25e331f7085b3bf4f
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java fbe894c96447b2e30594eb2ed0652dd08e1028ce
> geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java f86f07eb5e58b8509e909b7748795530efd65339
> geode-core/src/main/java/org/apache/geode/management/GemFireProperties.java 08fa9b54ea066b4158478ae89d8295ed0b1a337b
> geode-core/src/main/java/org/apache/geode/management/internal/beans/BeanUtilFuncs.java 499ef010f354ebf88765190f1b5eb945da83accc
> geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java 525f988cd3cb4f19872168df9b7105645f5c0498
>
>
> Diff: https://reviews.apache.org/r/61417/diff/1/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 61417: GEODE-3328: adding ssl-truststore-type to
the config
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61417/#review182172
-----------------------------------------------------------
Ship it!
Ship It!
- Udo Kohlmeyer
On Aug. 3, 2017, 9:15 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61417/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2017, 9:15 p.m.)
>
>
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, Patrick Rhomberg, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-3328: adding ssl-truststore-type to the config
>
> this is what's requested in the JIRA ticket. This changeset just deals with adding the property into gemfire properties
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java 63f6505101f6edb62167b854d3d16d76d0a893cd
> geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java 795f6a5a4a4f42fe065c9ca6013fd5598f9311d8
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java c2a395de0bfe21ed8efeb6b25e331f7085b3bf4f
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java fbe894c96447b2e30594eb2ed0652dd08e1028ce
> geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java f86f07eb5e58b8509e909b7748795530efd65339
> geode-core/src/main/java/org/apache/geode/management/GemFireProperties.java 08fa9b54ea066b4158478ae89d8295ed0b1a337b
> geode-core/src/main/java/org/apache/geode/management/internal/beans/BeanUtilFuncs.java 499ef010f354ebf88765190f1b5eb945da83accc
> geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java 525f988cd3cb4f19872168df9b7105645f5c0498
>
>
> Diff: https://reviews.apache.org/r/61417/diff/1/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 61417: GEODE-3328: adding ssl-truststore-type to
the config
Posted by Dave Barnes <db...@pivotal.io>.
Hi Jinmei,
1. Yes, a parallel change would be good for keystore. In both cases, we
need to specify the default. My sample text said empty string ('') -- is
that correct, or is the default JKS?
2 & 3: Good - glad they're correct. I wasn't sure, just noticed the
patterns.
-Dave
On Sat, Aug 5, 2017 at 12:15 PM, Jinmei Liao <ji...@pivotal.io> wrote:
>
>
> > On Aug. 3, 2017, 11:36 p.m., Dave Barnes wrote:
> > > 1. The help message for the property should state what it does and
> whether it has a default. JKS was our assumption in the past, but with the
> introduction of this feature other values (such as 'pkcs12') are possible.
> > > Current wording: "For Java truststore file format, this property has
> the value jks (or JKS)."
> > > Suggested wording: "Specifies the type of the ssl truststore. The
> default is '' (an empty string) For Java truststore format, specify 'jks'
> or 'JKS'."
> > >
> > > The following questions come from me reading for patterns, please
> forgive if they're stupid ones:
> > >
> > > 2. In configureLegacyClusterSSL (and ..ServerSSL and ..JMXSSL and
> ..ServiceSSL), I see two occurrences of "sslConfig.setTruststoreType(
> getDistributionConfig().getClusterSSLKeyStoreType());". Should the second
> one in each case be getClusterSSLTrustStoreType() ?
> > >
> > > 3. In DistributionConfigImpl.java, there's a long list of static
> imports for gateway, http, jmx, etc. KEYSTORE_TYPE is included for each
> group, but not TRUSTSTORE_TYPE. Is this correct?
>
> 1. I am simply copying from the ssl-keystore-type, should this suggested
> description also apply to the keystore as well?
> 2. the truststore type does not exist for legacy ssl specifications, so
> there is no getClusterSSLTrustStoreType() method. Here I am just assuming
> the keystore and truststore type is the same.
> 3. IDEA did that according to the latest import style.
>
>
> - Jinmei
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61417/#review182181
> -----------------------------------------------------------
>
>
> On Aug. 3, 2017, 9:15 p.m., Jinmei Liao wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/61417/
> > -----------------------------------------------------------
> >
> > (Updated Aug. 3, 2017, 9:15 p.m.)
> >
> >
> > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund,
> Patrick Rhomberg, and Udo Kohlmeyer.
> >
> >
> > Repository: geode
> >
> >
> > Description
> > -------
> >
> > GEODE-3328: adding ssl-truststore-type to the config
> >
> > this is what's requested in the JIRA ticket. This changeset just deals
> with adding the property into gemfire properties
> >
> >
> > Diffs
> > -----
> >
> > geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
> 63f6505101f6edb62167b854d3d16d76d0a893cd
> > geode-core/src/main/java/org/apache/geode/distributed/internal/
> AbstractDistributionConfig.java 795f6a5a4a4f42fe065c9ca6013fd5598f9311d8
> > geode-core/src/main/java/org/apache/geode/distributed/
> internal/DistributionConfig.java c2a395de0bfe21ed8efeb6b25e331f7085b3bf4f
> > geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
> fbe894c96447b2e30594eb2ed0652dd08e1028ce
> > geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java
> f86f07eb5e58b8509e909b7748795530efd65339
> > geode-core/src/main/java/org/apache/geode/management/GemFireProperties.java
> 08fa9b54ea066b4158478ae89d8295ed0b1a337b
> > geode-core/src/main/java/org/apache/geode/management/
> internal/beans/BeanUtilFuncs.java 499ef010f354ebf88765190f1b5eb945da83accc
> > geode-core/src/test/java/org/apache/geode/distributed/internal/
> DistributionConfigJUnitTest.java 525f988cd3cb4f19872168df9b7105645f5c0498
> >
> >
> > Diff: https://reviews.apache.org/r/61417/diff/1/
> >
> >
> > Testing
> > -------
> >
> > precheckin running
> >
> >
> > Thanks,
> >
> > Jinmei Liao
> >
> >
>
>
Re: Review Request 61417: GEODE-3328: adding ssl-truststore-type to
the config
Posted by Jinmei Liao <ji...@pivotal.io>.
> On Aug. 3, 2017, 11:36 p.m., Dave Barnes wrote:
> > 1. The help message for the property should state what it does and whether it has a default. JKS was our assumption in the past, but with the introduction of this feature other values (such as 'pkcs12') are possible.
> > Current wording: "For Java truststore file format, this property has the value jks (or JKS)."
> > Suggested wording: "Specifies the type of the ssl truststore. The default is '' (an empty string) For Java truststore format, specify 'jks' or 'JKS'."
> >
> > The following questions come from me reading for patterns, please forgive if they're stupid ones:
> >
> > 2. In configureLegacyClusterSSL (and ..ServerSSL and ..JMXSSL and ..ServiceSSL), I see two occurrences of "sslConfig.setTruststoreType(getDistributionConfig().getClusterSSLKeyStoreType());". Should the second one in each case be getClusterSSLTrustStoreType() ?
> >
> > 3. In DistributionConfigImpl.java, there's a long list of static imports for gateway, http, jmx, etc. KEYSTORE_TYPE is included for each group, but not TRUSTSTORE_TYPE. Is this correct?
1. I am simply copying from the ssl-keystore-type, should this suggested description also apply to the keystore as well?
2. the truststore type does not exist for legacy ssl specifications, so there is no getClusterSSLTrustStoreType() method. Here I am just assuming the keystore and truststore type is the same.
3. IDEA did that according to the latest import style.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61417/#review182181
-----------------------------------------------------------
On Aug. 3, 2017, 9:15 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61417/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2017, 9:15 p.m.)
>
>
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, Patrick Rhomberg, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-3328: adding ssl-truststore-type to the config
>
> this is what's requested in the JIRA ticket. This changeset just deals with adding the property into gemfire properties
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java 63f6505101f6edb62167b854d3d16d76d0a893cd
> geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java 795f6a5a4a4f42fe065c9ca6013fd5598f9311d8
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java c2a395de0bfe21ed8efeb6b25e331f7085b3bf4f
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java fbe894c96447b2e30594eb2ed0652dd08e1028ce
> geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java f86f07eb5e58b8509e909b7748795530efd65339
> geode-core/src/main/java/org/apache/geode/management/GemFireProperties.java 08fa9b54ea066b4158478ae89d8295ed0b1a337b
> geode-core/src/main/java/org/apache/geode/management/internal/beans/BeanUtilFuncs.java 499ef010f354ebf88765190f1b5eb945da83accc
> geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java 525f988cd3cb4f19872168df9b7105645f5c0498
>
>
> Diff: https://reviews.apache.org/r/61417/diff/1/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 61417: GEODE-3328: adding ssl-truststore-type to
the config
Posted by Dave Barnes <db...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61417/#review182181
-----------------------------------------------------------
1. The help message for the property should state what it does and whether it has a default. JKS was our assumption in the past, but with the introduction of this feature other values (such as 'pkcs12') are possible.
Current wording: "For Java truststore file format, this property has the value jks (or JKS)."
Suggested wording: "Specifies the type of the ssl truststore. The default is '' (an empty string) For Java truststore format, specify 'jks' or 'JKS'."
The following questions come from me reading for patterns, please forgive if they're stupid ones:
2. In configureLegacyClusterSSL (and ..ServerSSL and ..JMXSSL and ..ServiceSSL), I see two occurrences of "sslConfig.setTruststoreType(getDistributionConfig().getClusterSSLKeyStoreType());". Should the second one in each case be getClusterSSLTrustStoreType() ?
3. In DistributionConfigImpl.java, there's a long list of static imports for gateway, http, jmx, etc. KEYSTORE_TYPE is included for each group, but not TRUSTSTORE_TYPE. Is this correct?
- Dave Barnes
On Aug. 3, 2017, 9:15 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61417/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2017, 9:15 p.m.)
>
>
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, Patrick Rhomberg, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-3328: adding ssl-truststore-type to the config
>
> this is what's requested in the JIRA ticket. This changeset just deals with adding the property into gemfire properties
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java 63f6505101f6edb62167b854d3d16d76d0a893cd
> geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java 795f6a5a4a4f42fe065c9ca6013fd5598f9311d8
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java c2a395de0bfe21ed8efeb6b25e331f7085b3bf4f
> geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java fbe894c96447b2e30594eb2ed0652dd08e1028ce
> geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java f86f07eb5e58b8509e909b7748795530efd65339
> geode-core/src/main/java/org/apache/geode/management/GemFireProperties.java 08fa9b54ea066b4158478ae89d8295ed0b1a337b
> geode-core/src/main/java/org/apache/geode/management/internal/beans/BeanUtilFuncs.java 499ef010f354ebf88765190f1b5eb945da83accc
> geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionConfigJUnitTest.java 525f988cd3cb4f19872168df9b7105645f5c0498
>
>
> Diff: https://reviews.apache.org/r/61417/diff/1/
>
>
> Testing
> -------
>
> precheckin running
>
>
> Thanks,
>
> Jinmei Liao
>
>