You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Bruce Schuchardt <bs...@pivotal.io> on 2017/09/05 17:57:35 UTC
Review Request 62088: GEODE-3249 Validate internal client/server
messages
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/
-----------------------------------------------------------
Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
Bugs: GEODE-3249
https://issues.apache.org/jira/browse/GEODE-3249
Repository: geode
Description
-------
This change leaves the security hole in place but allows you to plug it by setting the system property
geode.disallow-internal-messages-without-credentials=true
Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
Diffs
-----
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
Diff: https://reviews.apache.org/r/62088/diff/1/
Testing
-------
Thanks,
Bruce Schuchardt
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Alexander Murmann <am...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/#review184591
-----------------------------------------------------------
Ship it!
I am rather disappointed that this required no test changes. It seems unreasonable to require that as part of this small change. How would you feel about adding a chore to backfill test coverage in the near future? It makes me quite uneasy that we don't have coverage for something this important.
- Alexander Murmann
On Sept. 5, 2017, 5:57 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2017, 5:57 p.m.)
>
>
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-3249
> https://issues.apache.org/jira/browse/GEODE-3249
>
>
> Repository: geode
>
>
> Description
> -------
>
> This change leaves the security hole in place but allows you to plug it by setting the system property
>
> geode.disallow-internal-messages-without-credentials=true
>
> Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
>
>
> Diff: https://reviews.apache.org/r/62088/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Anthony Baker <ab...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/#review184596
-----------------------------------------------------------
Ship it!
Ship It!
- Anthony Baker
On Sept. 5, 2017, 5:57 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2017, 5:57 p.m.)
>
>
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-3249
> https://issues.apache.org/jira/browse/GEODE-3249
>
>
> Repository: geode
>
>
> Description
> -------
>
> This change leaves the security hole in place but allows you to plug it by setting the system property
>
> geode.disallow-internal-messages-without-credentials=true
>
> Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
>
>
> Diff: https://reviews.apache.org/r/62088/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/#review184986
-----------------------------------------------------------
Ship it!
Ship It!
- Hitesh Khamesra
On Sept. 7, 2017, 5:43 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 5:43 p.m.)
>
>
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-3249
> https://issues.apache.org/jira/browse/GEODE-3249
>
>
> Repository: geode
>
>
> Description
> -------
>
> This change leaves the security hole in place but allows you to plug it by setting the system property
>
> geode.disallow-internal-messages-without-credentials=true
>
> Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
>
> New tests have been added to perform backward-compatibility testing with the old security implementation and the internal message command classes have been modified to perform validation of credentials if the system property is set to true.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/AddPdxEnum.java 5a4a07b81b18d33e465bd3aa46ad4232b976b608
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/AddPdxType.java 041e12fbd04e81f1f69520c53ef9c2f7481132fd
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetFunctionAttribute.java 76cc4a59bff691c4760083861362825d70ba326e
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXEnumById.java 5e59640e5067ec8ac5fc50807ec276e1bdc025dd
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXIdForEnum.java b0ebaf23f27e91278c7afe3648954ad6113206a8
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXIdForType.java f2172ef4d8fa9f83929d8f5b2aa0c5377d7cf57e
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXTypeById.java e46445bc96d735a66aa09330a1790b951591251e
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPdxEnums70.java 3fe9750f8577a70e4cda9e76da83070f6e6606b1
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPdxTypes70.java e64683fb620985d698357912bb1d1b52e8b24681
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/RegisterDataSerializers.java eef5195eae3bedb414aa2e2fca748b31e0b27908
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/RegisterInstantiators.java a402cb360f05f99442833e6098c736d2ac18d69a
> geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationDUnitTest.java ca7b2b6b7a2c8d8215eda828901a05dcabdf3625
> geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationPart2DUnitTest.java f8ebe056e21228f1d9e32e1dd271f6a4bfb4af71
> geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationTestCase.java 0ecd72f4ee321f7f8aa5e998fa176551e45f025c
> geode-core/src/test/java/org/apache/geode/security/ClientAuthorizationDUnitTest.java 09aedbec86f95ab9affa1f76b387a5a03c0098ec
> geode-core/src/test/java/org/apache/geode/security/ClientAuthorizationTestCase.java a4fd365ffaa51447d56c2bcb481311082ddcbc31
> geode-core/src/test/java/org/apache/geode/security/SecurityTestUtils.java e69f36de1efbd0061ad8621db45fe3a64686968e
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/MonitorCQ.java f5e31df988f5955d2fbeef5269a7729ec97c9d03
> geode-cq/src/test/java/org/apache/geode/security/ClientAuthorizationTwoDUnitTest.java f5f686c0595c7500c4275292edb2e8f87593c67e
>
>
> Diff: https://reviews.apache.org/r/62088/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/
-----------------------------------------------------------
(Updated Sept. 7, 2017, 10:43 a.m.)
Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
Changes
-------
modified tests to also run with the current version in clients
Bugs: GEODE-3249
https://issues.apache.org/jira/browse/GEODE-3249
Repository: geode
Description
-------
This change leaves the security hole in place but allows you to plug it by setting the system property
geode.disallow-internal-messages-without-credentials=true
Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
New tests have been added to perform backward-compatibility testing with the old security implementation and the internal message command classes have been modified to perform validation of credentials if the system property is set to true.
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/AddPdxEnum.java 5a4a07b81b18d33e465bd3aa46ad4232b976b608
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/AddPdxType.java 041e12fbd04e81f1f69520c53ef9c2f7481132fd
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetFunctionAttribute.java 76cc4a59bff691c4760083861362825d70ba326e
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXEnumById.java 5e59640e5067ec8ac5fc50807ec276e1bdc025dd
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXIdForEnum.java b0ebaf23f27e91278c7afe3648954ad6113206a8
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXIdForType.java f2172ef4d8fa9f83929d8f5b2aa0c5377d7cf57e
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXTypeById.java e46445bc96d735a66aa09330a1790b951591251e
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPdxEnums70.java 3fe9750f8577a70e4cda9e76da83070f6e6606b1
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPdxTypes70.java e64683fb620985d698357912bb1d1b52e8b24681
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/RegisterDataSerializers.java eef5195eae3bedb414aa2e2fca748b31e0b27908
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/RegisterInstantiators.java a402cb360f05f99442833e6098c736d2ac18d69a
geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationDUnitTest.java ca7b2b6b7a2c8d8215eda828901a05dcabdf3625
geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationPart2DUnitTest.java f8ebe056e21228f1d9e32e1dd271f6a4bfb4af71
geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationTestCase.java 0ecd72f4ee321f7f8aa5e998fa176551e45f025c
geode-core/src/test/java/org/apache/geode/security/ClientAuthorizationDUnitTest.java 09aedbec86f95ab9affa1f76b387a5a03c0098ec
geode-core/src/test/java/org/apache/geode/security/ClientAuthorizationTestCase.java a4fd365ffaa51447d56c2bcb481311082ddcbc31
geode-core/src/test/java/org/apache/geode/security/SecurityTestUtils.java e69f36de1efbd0061ad8621db45fe3a64686968e
geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/MonitorCQ.java f5e31df988f5955d2fbeef5269a7729ec97c9d03
geode-cq/src/test/java/org/apache/geode/security/ClientAuthorizationTwoDUnitTest.java f5f686c0595c7500c4275292edb2e8f87593c67e
Diff: https://reviews.apache.org/r/62088/diff/3/
Changes: https://reviews.apache.org/r/62088/diff/2-3/
Testing
-------
Thanks,
Bruce Schuchardt
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/
-----------------------------------------------------------
(Updated Sept. 7, 2017, 10:32 a.m.)
Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
Bugs: GEODE-3249
https://issues.apache.org/jira/browse/GEODE-3249
Repository: geode
Description (updated)
-------
This change leaves the security hole in place but allows you to plug it by setting the system property
geode.disallow-internal-messages-without-credentials=true
Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
New tests have been added to perform backward-compatibility testing with the old security implementation and the internal message command classes have been modified to perform validation of credentials if the system property is set to true.
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/AddPdxEnum.java 5a4a07b81b18d33e465bd3aa46ad4232b976b608
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/AddPdxType.java 041e12fbd04e81f1f69520c53ef9c2f7481132fd
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetFunctionAttribute.java 76cc4a59bff691c4760083861362825d70ba326e
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXEnumById.java 5e59640e5067ec8ac5fc50807ec276e1bdc025dd
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXIdForEnum.java b0ebaf23f27e91278c7afe3648954ad6113206a8
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXIdForType.java f2172ef4d8fa9f83929d8f5b2aa0c5377d7cf57e
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXTypeById.java e46445bc96d735a66aa09330a1790b951591251e
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPdxEnums70.java 3fe9750f8577a70e4cda9e76da83070f6e6606b1
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPdxTypes70.java e64683fb620985d698357912bb1d1b52e8b24681
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/RegisterDataSerializers.java eef5195eae3bedb414aa2e2fca748b31e0b27908
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/RegisterInstantiators.java a402cb360f05f99442833e6098c736d2ac18d69a
geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationDUnitTest.java ca7b2b6b7a2c8d8215eda828901a05dcabdf3625
geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationPart2DUnitTest.java f8ebe056e21228f1d9e32e1dd271f6a4bfb4af71
geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationTestCase.java 0ecd72f4ee321f7f8aa5e998fa176551e45f025c
geode-core/src/test/java/org/apache/geode/security/ClientAuthorizationDUnitTest.java 09aedbec86f95ab9affa1f76b387a5a03c0098ec
geode-core/src/test/java/org/apache/geode/security/ClientAuthorizationTestCase.java a4fd365ffaa51447d56c2bcb481311082ddcbc31
geode-core/src/test/java/org/apache/geode/security/SecurityTestUtils.java e69f36de1efbd0061ad8621db45fe3a64686968e
geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/MonitorCQ.java f5e31df988f5955d2fbeef5269a7729ec97c9d03
geode-cq/src/test/java/org/apache/geode/security/ClientAuthorizationTwoDUnitTest.java f5f686c0595c7500c4275292edb2e8f87593c67e
Diff: https://reviews.apache.org/r/62088/diff/2/
Changes: https://reviews.apache.org/r/62088/diff/1-2/
Testing
-------
Thanks,
Bruce Schuchardt
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/#review184594
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
Line 92 (original), 92 (patched)
<https://reviews.apache.org/r/62088/#comment260757>
This comment is a bit confusing.
For this boolean I think it defaults to TRUE because Boolean.getBoolean returns FALSE if the sys prop is not defined and the code negates that. The comment says it defaults to OFF which I would think means FALSE.
I think this javadoc should tell you what this boolean does if it is true and what it does if it is false.
It would probably be more helpful to say what setting geode.disallow-internal-messages-without-credentials to true does since that is the non-default behavior.
- Darrel Schneider
On Sept. 5, 2017, 10:57 a.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2017, 10:57 a.m.)
>
>
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-3249
> https://issues.apache.org/jira/browse/GEODE-3249
>
>
> Repository: geode
>
>
> Description
> -------
>
> This change leaves the security hole in place but allows you to plug it by setting the system property
>
> geode.disallow-internal-messages-without-credentials=true
>
> Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
>
>
> Diff: https://reviews.apache.org/r/62088/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On Sept. 5, 2017, 11 a.m., Hitesh Khamesra wrote:
> > Ship It!
I've had to do more work on this & would appreciate another review.
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/#review184565
-----------------------------------------------------------
On Sept. 7, 2017, 10:32 a.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 10:32 a.m.)
>
>
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-3249
> https://issues.apache.org/jira/browse/GEODE-3249
>
>
> Repository: geode
>
>
> Description
> -------
>
> This change leaves the security hole in place but allows you to plug it by setting the system property
>
> geode.disallow-internal-messages-without-credentials=true
>
> Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
>
> New tests have been added to perform backward-compatibility testing with the old security implementation and the internal message command classes have been modified to perform validation of credentials if the system property is set to true.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/AddPdxEnum.java 5a4a07b81b18d33e465bd3aa46ad4232b976b608
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/AddPdxType.java 041e12fbd04e81f1f69520c53ef9c2f7481132fd
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetFunctionAttribute.java 76cc4a59bff691c4760083861362825d70ba326e
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXEnumById.java 5e59640e5067ec8ac5fc50807ec276e1bdc025dd
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXIdForEnum.java b0ebaf23f27e91278c7afe3648954ad6113206a8
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXIdForType.java f2172ef4d8fa9f83929d8f5b2aa0c5377d7cf57e
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPDXTypeById.java e46445bc96d735a66aa09330a1790b951591251e
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPdxEnums70.java 3fe9750f8577a70e4cda9e76da83070f6e6606b1
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/GetPdxTypes70.java e64683fb620985d698357912bb1d1b52e8b24681
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/RegisterDataSerializers.java eef5195eae3bedb414aa2e2fca748b31e0b27908
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/RegisterInstantiators.java a402cb360f05f99442833e6098c736d2ac18d69a
> geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationDUnitTest.java ca7b2b6b7a2c8d8215eda828901a05dcabdf3625
> geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationPart2DUnitTest.java f8ebe056e21228f1d9e32e1dd271f6a4bfb4af71
> geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationTestCase.java 0ecd72f4ee321f7f8aa5e998fa176551e45f025c
> geode-core/src/test/java/org/apache/geode/security/ClientAuthorizationDUnitTest.java 09aedbec86f95ab9affa1f76b387a5a03c0098ec
> geode-core/src/test/java/org/apache/geode/security/ClientAuthorizationTestCase.java a4fd365ffaa51447d56c2bcb481311082ddcbc31
> geode-core/src/test/java/org/apache/geode/security/SecurityTestUtils.java e69f36de1efbd0061ad8621db45fe3a64686968e
> geode-cq/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/MonitorCQ.java f5e31df988f5955d2fbeef5269a7729ec97c9d03
> geode-cq/src/test/java/org/apache/geode/security/ClientAuthorizationTwoDUnitTest.java f5f686c0595c7500c4275292edb2e8f87593c67e
>
>
> Diff: https://reviews.apache.org/r/62088/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Hitesh Khamesra <hk...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/#review184565
-----------------------------------------------------------
Ship it!
Ship It!
- Hitesh Khamesra
On Sept. 5, 2017, 5:57 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2017, 5:57 p.m.)
>
>
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-3249
> https://issues.apache.org/jira/browse/GEODE-3249
>
>
> Repository: geode
>
>
> Description
> -------
>
> This change leaves the security hole in place but allows you to plug it by setting the system property
>
> geode.disallow-internal-messages-without-credentials=true
>
> Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
>
>
> Diff: https://reviews.apache.org/r/62088/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/#review184602
-----------------------------------------------------------
Ship it!
Ship It!
- Udo Kohlmeyer
On Sept. 5, 2017, 5:57 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2017, 5:57 p.m.)
>
>
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-3249
> https://issues.apache.org/jira/browse/GEODE-3249
>
>
> Repository: geode
>
>
> Description
> -------
>
> This change leaves the security hole in place but allows you to plug it by setting the system property
>
> geode.disallow-internal-messages-without-credentials=true
>
> Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
>
>
> Diff: https://reviews.apache.org/r/62088/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 62088: GEODE-3249 Validate internal client/server messages
Posted by Brian Baynes <bb...@pivotal.io>.
Ah, I see. Makes sense.
On Sep 6, 2017 12:23 PM, "Bruce Schuchardt" <bs...@pivotal.io> wrote:
I think we will want to remove this property in the next major release and
have the behavior it enables be how the servers always act.
On 9/6/17 10:23 AM, Brian Baynes wrote:
In this case, won't we be changing the default of this property with the
next major release? So perhaps the choice is to follow the default=false
convention now, or with the next major release..?
On Wed, Sep 6, 2017 at 8:47 AM, Bruce Schuchardt <bs...@pivotal.io>
wrote:
>
>
> > On Sept. 5, 2017, 5:09 p.m., Galen O'Sullivan wrote:
> > > I prefer config option names to be as unambiguous as possible. I think
> `allow` would be clearer than `disallow` because it avoids
> double-negatives. Can we use
> > > `allow-internal-messages-without-credentials` and have it default to
> `true`?
>
> In general Java properties ought to default to _false_ if they aren't
> set. We've had other properties default to _true_ in the past and they
> were awkward.
>
>
> - Bruce
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/#review184608
> -----------------------------------------------------------
>
>
> On Sept. 5, 2017, 10:57 a.m., Bruce Schuchardt wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/62088/
> > -----------------------------------------------------------
> >
> > (Updated Sept. 5, 2017, 10:57 a.m.)
> >
> >
> > Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh
> Khamesra, and Udo Kohlmeyer.
> >
> >
> > Bugs: GEODE-3249
> > https://issues.apache.org/jira/browse/GEODE-3249
> >
> >
> > Repository: geode
> >
> >
> > Description
> > -------
> >
> > This change leaves the security hole in place but allows you to plug it
> by setting the system property
> >
> > geode.disallow-internal-messages-without-credentials=true
> >
> > Clients must be upgraded to the release containing this change if you
> set this system property to true and client/server authentication is
> enabled. Otherwise client messages to register PDX types or Instantiators
> will be rejected by the servers.
> >
> >
> > Diffs
> > -----
> >
> > geode-core/src/main/java/org/apache/geode/internal/cache/ti
> er/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
> >
> >
> > Diff: https://reviews.apache.org/r/62088/diff/1/
> >
> >
> > Testing
> > -------
> >
> >
> > Thanks,
> >
> > Bruce Schuchardt
> >
> >
>
>
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Bruce Schuchardt <bs...@pivotal.io>.
I think we will want to remove this property in the next major release
and have the behavior it enables be how the servers always act.
On 9/6/17 10:23 AM, Brian Baynes wrote:
> In this case, won't we be changing the default of this property with
> the next major release? So perhaps the choice is to follow the
> default=false convention now, or with the next major release..?
>
>
> On Wed, Sep 6, 2017 at 8:47 AM, Bruce Schuchardt
> <bschuchardt@pivotal.io <ma...@pivotal.io>> wrote:
>
>
>
> > On Sept. 5, 2017, 5:09 p.m., Galen O'Sullivan wrote:
> > > I prefer config option names to be as unambiguous as possible.
> I think `allow` would be clearer than `disallow` because it avoids
> double-negatives. Can we use
> > > `allow-internal-messages-without-credentials` and have it
> default to `true`?
>
> In general Java properties ought to default to _false_ if they
> aren't set. We've had other properties default to _true_ in the
> past and they were awkward.
>
>
> - Bruce
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/#review184608
> <https://reviews.apache.org/r/62088/#review184608>
> -----------------------------------------------------------
>
>
> On Sept. 5, 2017, 10:57 a.m., Bruce Schuchardt wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/62088/
> <https://reviews.apache.org/r/62088/>
> > -----------------------------------------------------------
> >
> > (Updated Sept. 5, 2017, 10:57 a.m.)
> >
> >
> > Review request for geode, Alexander Murmann, Galen O'Sullivan,
> Hitesh Khamesra, and Udo Kohlmeyer.
> >
> >
> > Bugs: GEODE-3249
> > https://issues.apache.org/jira/browse/GEODE-3249
> <https://issues.apache.org/jira/browse/GEODE-3249>
> >
> >
> > Repository: geode
> >
> >
> > Description
> > -------
> >
> > This change leaves the security hole in place but allows you to
> plug it by setting the system property
> >
> > geode.disallow-internal-messages-without-credentials=true
> >
> > Clients must be upgraded to the release containing this change
> if you set this system property to true and client/server
> authentication is enabled. Otherwise client messages to register
> PDX types or Instantiators will be rejected by the servers.
> >
> >
> > Diffs
> > -----
> >
> >
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
> b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
> >
> >
> > Diff: https://reviews.apache.org/r/62088/diff/1/
> <https://reviews.apache.org/r/62088/diff/1/>
> >
> >
> > Testing
> > -------
> >
> >
> > Thanks,
> >
> > Bruce Schuchardt
> >
> >
>
>
Re: Review Request 62088: GEODE-3249 Validate internal client/server messages
Posted by Brian Baynes <bb...@pivotal.io>.
In this case, won't we be changing the default of this property with the
next major release? So perhaps the choice is to follow the default=false
convention now, or with the next major release..?
On Wed, Sep 6, 2017 at 8:47 AM, Bruce Schuchardt <bs...@pivotal.io>
wrote:
>
>
> > On Sept. 5, 2017, 5:09 p.m., Galen O'Sullivan wrote:
> > > I prefer config option names to be as unambiguous as possible. I think
> `allow` would be clearer than `disallow` because it avoids
> double-negatives. Can we use
> > > `allow-internal-messages-without-credentials` and have it default to
> `true`?
>
> In general Java properties ought to default to _false_ if they aren't
> set. We've had other properties default to _true_ in the past and they
> were awkward.
>
>
> - Bruce
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/#review184608
> -----------------------------------------------------------
>
>
> On Sept. 5, 2017, 10:57 a.m., Bruce Schuchardt wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/62088/
> > -----------------------------------------------------------
> >
> > (Updated Sept. 5, 2017, 10:57 a.m.)
> >
> >
> > Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh
> Khamesra, and Udo Kohlmeyer.
> >
> >
> > Bugs: GEODE-3249
> > https://issues.apache.org/jira/browse/GEODE-3249
> >
> >
> > Repository: geode
> >
> >
> > Description
> > -------
> >
> > This change leaves the security hole in place but allows you to plug it
> by setting the system property
> >
> > geode.disallow-internal-messages-without-credentials=true
> >
> > Clients must be upgraded to the release containing this change if you
> set this system property to true and client/server authentication is
> enabled. Otherwise client messages to register PDX types or Instantiators
> will be rejected by the servers.
> >
> >
> > Diffs
> > -----
> >
> > geode-core/src/main/java/org/apache/geode/internal/cache/
> tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee
> 2d7216f1f7
> >
> >
> > Diff: https://reviews.apache.org/r/62088/diff/1/
> >
> >
> > Testing
> > -------
> >
> >
> > Thanks,
> >
> > Bruce Schuchardt
> >
> >
>
>
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Bruce Schuchardt <bs...@pivotal.io>.
> On Sept. 5, 2017, 5:09 p.m., Galen O'Sullivan wrote:
> > I prefer config option names to be as unambiguous as possible. I think `allow` would be clearer than `disallow` because it avoids double-negatives. Can we use
> > `allow-internal-messages-without-credentials` and have it default to `true`?
In general Java properties ought to default to _false_ if they aren't set. We've had other properties default to _true_ in the past and they were awkward.
- Bruce
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/#review184608
-----------------------------------------------------------
On Sept. 5, 2017, 10:57 a.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2017, 10:57 a.m.)
>
>
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-3249
> https://issues.apache.org/jira/browse/GEODE-3249
>
>
> Repository: geode
>
>
> Description
> -------
>
> This change leaves the security hole in place but allows you to plug it by setting the system property
>
> geode.disallow-internal-messages-without-credentials=true
>
> Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
>
>
> Diff: https://reviews.apache.org/r/62088/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruce Schuchardt
>
>
Re: Review Request 62088: GEODE-3249 Validate internal client/server
messages
Posted by Galen O'Sullivan <go...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62088/#review184608
-----------------------------------------------------------
I prefer config option names to be as unambiguous as possible. I think `allow` would be clearer than `disallow` because it avoids double-negatives. Can we use
`allow-internal-messages-without-credentials` and have it default to `true`?
- Galen O'Sullivan
On Sept. 5, 2017, 5:57 p.m., Bruce Schuchardt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62088/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2017, 5:57 p.m.)
>
>
> Review request for geode, Alexander Murmann, Galen O'Sullivan, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Bugs: GEODE-3249
> https://issues.apache.org/jira/browse/GEODE-3249
>
>
> Repository: geode
>
>
> Description
> -------
>
> This change leaves the security hole in place but allows you to plug it by setting the system property
>
> geode.disallow-internal-messages-without-credentials=true
>
> Clients must be upgraded to the release containing this change if you set this system property to true and client/server authentication is enabled. Otherwise client messages to register PDX types or Instantiators will be rejected by the servers.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java b243d8ebb8f7fb698a4637c7a787ee2d7216f1f7
>
>
> Diff: https://reviews.apache.org/r/62088/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bruce Schuchardt
>
>