You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "Bill (GitHub)" <gi...@apache.org> on 2020/03/03 01:13:59 UTC

[GitHub] [geode] Bill opened pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

[GEODE-7837](https://issues.apache.org/jira/browse/GEODE-7837)

### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [x] Is your initial contribution a single, squashed commit?

- [x] 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.


[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Why not provide the SNI regardless of proxy?

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "Bill (GitHub)" <gi...@apache.org>.
Yes indeed. (recall this is a draft PR.) From the comments:

```
This SNI proxy is not suitable for CI tests. It's for developer testing only.
```

The actual test is `@Ignore`d because:

```
This test is ignored because it's connecting to a private toolsmith's BOSH environment.
```

Is it reasonable to have such an (`@Ignore`d) test make it into Geode as a temporary development utility? Or should I be sure to delete this whole test before publishing the (non-draft) PR?

As it stands, any developer with access to an SNI proxy, could edit these settings and test against their own environment. I realize it isn't ideal, but it's something.

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "Bill (GitHub)" <gi...@apache.org>.
If I understand your question, I chose to pass a lambda to `configureClientSSLSocket()` rather than passing in a (possibly-`null` SNI hostname).

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "Bill (GitHub)" <gi...@apache.org>.
Yeah, it's currently a "security" property merely because SNI is part of TLS and TLS is configured via "security" properties.

The URL idea is interesting! I'm letting it soak in now…

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "Bill (GitHub)" <gi...@apache.org>.
deleting this test from this PR…

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
What is the purpose of passing it as an http URL here? I believe there are parsers already in Geode for splitting host:port.

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "Bill (GitHub)" <gi...@apache.org>.
Good point. There are many host+port parsers in the codebase. The one I was thinking of was the one that parses locator host+port: `GMSUtil.parseLocators()`. I opted not to use that one because it dictates a nonstandard syntax e.g. `hostname[123]`. That format is used in a few places in Geode but it's unusual to folks who are used to web browsers.

Do you know, offhand, of another parser we've got that parses the more orthodox colon-separated port (and supports fqdn's, IPv4 addys and IPv6 ones too?)

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill closed pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "Bill (GitHub)" <gi...@apache.org>.
[ pull request closed by Bill ]

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
I really don't understand why this is in the SSL config. Proxy in general happens before TLS would happen. The implementation really highly couples proxy with TLS, which I don't feel we should do.

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on issue #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "Bill (GitHub)" <gi...@apache.org>.
that IntegrationTestOpenJDK11 is a well-known flaky test outside this PR: https://issues.apache.org/jira/browse/GEODE-7843?jql=text%20~%20%22testConcurrentHSetNX%22

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "Bill (GitHub)" <gi...@apache.org>.
Yes indeed. (recall this is a draft PR.) From the comments:

```
This SNI proxy is not suitable for CI tests. It's for developer testing only.
```

The actual test is `@Ignore`d because:

```
This test is ignored because it's connecting to a private toolsmith's BOSH environment.
```

Is it reasonable to have such an (`@Ignore`d) test make it into Geode as a temporary development utility? Or should I be sure to delete this whole test before publishing the (non-draft) PR?

As it stands, any developer with access to an SNI proxy, could edit these settings and test against their own environment. I realize it isn't ideal, but it's something. FYI I am working on an approach to CI testing agin a proxy spun-up by the test itself. TBD if that test makes it into this PR or a follow-on.

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "Bill (GitHub)" <gi...@apache.org>.
Oh I see what you mean. I'd rather not add that header, which is superfluous in the common case i.e. no proxy. But yeah like you, I think functionally that could work.

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Gotcha...

You might look at https://github.com/dlundquist/sniproxy for a simple integration test.

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pivotal-jbarrett commented on pull request #4758: GEODE-7837: add gemfire.security.sni-proxy system property

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
I am thinking from the standpoint that everything is the same except for where it can't. So in this case the initialization of the SSL socket is identical regardless of the proxy. Less moving parts.

[ Full content available at: https://github.com/apache/geode/pull/4758 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org