Skip to content

GEODE-3868: Client security example should use SSL#45

Merged
sboorlagadda merged 4 commits intoapache:developfrom
sboorlagadda:clientSecurit_ssl
Jan 29, 2018
Merged

GEODE-3868: Client security example should use SSL#45
sboorlagadda merged 4 commits intoapache:developfrom
sboorlagadda:clientSecurit_ssl

Conversation

@sboorlagadda
Copy link
Member

Updated the example to use SSL between all members and
between client/server.

   Updated the example to use SSL between all members and
   client/server.
Copy link
Contributor

@davebarnes97 davebarnes97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved - Good material! I suggest few language tweaks, none are showstoppers:
top-level README.md:

  • "run against latest" -> "run against the latest"
  • "you can checkout" -> "you can check out"

clientSecurity/README.md:

  • "Geode cluster, also it demonstrates use" -> "Geode cluster. It also demonstrates the use"
  • "between all members and between client/server" -> "between all members and between a client and a server"

Copy link
Contributor

@upthewaterspout upthewaterspout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good.

I think the example should have a separate keystore and truststore for the client and the server, because that is what users should be doing in practice.

about your suggestions at [dev@geode.apache.org](mailto:dev@geode.apache.org)
or submit a [pull request](https://github.com/apache/geode/pull/new/develop).

# Apache Geode Version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just tell people to check out the master branch, which should be the default. That will have the examples that run against the latest geode release.

We shouldn't point people at the snapshot releases per apache policy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed readme

# Recall that the --classpath option is specified relative to the locator's working directory.

start locator --name=locator --bind-address=127.0.0.1\
start locator --name=locator --bind-address=127.0.0.1 --connect=false --enable-cluster-configuration=false\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this --enable-cluster-configuration=false overwritten by the use of a security manager?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to show examples of not using cluster configuration...do we?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment here, we do not want to show an example with cluster configuration disabled.

Copy link
Member Author

@sboorlagadda sboorlagadda Jan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I agree we shouldnt be showing a wrong example. I was trying to make the example work with relative paths which is the easiest way for developers to run the examples. GEODE-4332 fixes not to retrieve cluster configuration service status when auto-connect is false, with this fix we are good providing keystore relative to locator`s JVM and then a second step gfsh can connect to the locator with paths relative to gfsh jvm.

@sboorlagadda sboorlagadda merged commit eef628b into apache:develop Jan 29, 2018
@sboorlagadda sboorlagadda deleted the clientSecurit_ssl branch January 29, 2018 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants