removed group name and password checks from rest calls

[hasancelik]

As Neil mentioned at the issue, when you make a REST call that does not contain group-password, it failed with {"status":"forbidden"}. But starting with hazelcast-3.8.2 release, there is no need for a group password at member side.
https://docs.hazelcast.org/docs/3.10.4/manual/html-single/index.html#cluster-groups-before-hazelcast-382

Current behaviour
I have started 3.10.4 member with only groupName config then make some REST calls:

Hasans-MacBook:~ hasancelik$ curl --data "hasan" http://192.168.99.1:5701/hazelcast/rest/management/cluster/state
{"status":"forbidden"}

If you do not configure any groupPassword, you must use dev-pass:

Hasans-MacBook:~ hasancelik$ curl --data "hasan&dev-pass" http://192.168.99.1:5701/hazelcast/rest/management/cluster/state
{"status":"success","state":"active"}

---

I have removed group-name checks as well from handle methods then converted some of the methods which do not take any parameter into GET(/nodes and /state). You can use --data as well, we are keeping POST methods of these two methods for backward-compatible by linking functionality(allowing both GET & POST):

New behaviour
I have started 3.11-SS -which includes these changes- member then make some REST calls:

Hasans-MacBook:~ hasancelik$ curl http://192.168.99.1:5701/hazelcast/rest/management/cluster/state
{"status":"success","state":"active"}
Hasans-MacBook:~ hasancelik$ curl http://192.168.99.1:5701/hazelcast/rest/management/cluster/nodes
{"status":"success","response":"[Member [192.168.99.1]:5701 - 05c688c8-41ef-429e-b412-296db9af3f57 this]
3.11-SNAPSHOT
1.8.0_60"}

Hasans-MacBook:~ hasancelik$ curl --data "hasan&hasan" http://192.168.99.1:5701/hazelcast/rest/management/cluster/state
{"status":"success","state":"active"}

Hasans-MacBook:~ hasancelik$ curl -X POST -H "Content-Length: 0" http://10.216.1.42:5701/hazelcast/rest/management/cluster/memberShutdown
{"status":"success"}

hasancelik$ curl --data 'hasan&hasan' http://10.216.1.42:5701/hazelcast/rest/management/cluster/memberShutdown
{"status":"success"}

---

fixes https://github.com/hazelcast/hazelcast-boshrelease/issues/16 and #13753

After this PR merged we need to update our documentation. Reference Manual issue --> hazelcast/hazelcast-reference-manual#584

[emre-aydin]

We should remove the passwords from cluster.sh as well.

[hasancelik]

@emre-aydin good catch 👍 I have removed them from cluster.sh

[tkountis]

Why don't we get rid of the POST body altogether?
No name & No password. It is very misleading, offering a false sense of security, and provides no functionality whatsoever. We direct a REST call to an IP:PORT that is meant for that cluster, the group-name is useless in this context. I strongly believe we should remove this check, removing it shouldn't break backward compatibility either.

[hasancelik]

@tkountis good idea actually, WDYT @emre-aydin and @mesutcelik?

[emre-aydin]

Sounds good to me.

[hasancelik]

@tkountis @emre-aydin I have updated the PR according the Thomas' advices. PTAL.

[emre-aydin]

@hasancelik I think we should leave methods that change the member's state (such as cluster/node shutdown, force start etc.) as HTTP POST even though they require no parameters.

[hasancelik]

@emre-aydin you are right, essentially GET is used to retrieve remote data, and POST is used to insert/update remote data/application. I have converted them into GET because if we use only POST requests for these operations, user must define request type and length of content (-X POST -H "Content-Length: 0") to make successful REST call . My motivation was making it REST easier to use but following general convention is better idea, I will update the PR 👍

[tkountis]

This looks good, can we add tests to verify backwards compatibility and to avoid any breaking change in the future? @hasancelik

Regarding Emre's comment, GET vs POST for the state changing APIs, i agree that in the future we need to refactor the API to make it more REST friendly. The current state feels more like RPC rather than REST. Having a proper REST API will allow the shutdown to be changed to /cluster/state body= {state: 'shutdown'}.

[tkountis]

LGTM - please add some backwards comp tests.

[hasancelik]

@tkountis and @emre-aydin I have added tests to verify backwards compatibility and made some enhancements on REST call which has one parameter(/changeState, /changeVersion and /mancenter/changeurl) to ensure backwards compatibility. PTAL

[enesakar]

Guys, we should not merge this unless we solve this: https://hazelcast.atlassian.net/wiki/spaces/PM/pages/147718270/Security+Concerns+on+Shutting+Down+Cluster+via+REST+API

Currently one can shutdown a cluster, if he knows ip address and port of any hazelcast member.
The shutdown endpoint is open to public even user disables the REST.

In hazelcast cloud, because there is no security/auth support of REST, we are still using group-name/password to secure REST. If you merge this PR, then the hazelcast cloud instance can be easily shutted down.

@kwart @jerrinot Can you review please?

[kwart]

I'm against merging this into 3.11 as it's too risky. It doesn't include solution based on JAAS authentication in the EE. (We have separate JAAS login modules config for members and clients.)

The change should be aligned with solution for #11867 (removing password check from clients).

This also deserves a proper design document.

[kwart]

@tkountis Even if we remove the password check in the future we should keep the group name check in place (as we do with members and clients). It'll be the safety belt which will prevent accidental operations on wrong cluster.

[tkountis]

Guys, I understand the concerns, but group info is not a security measure.
It gives a false impression to the end user, and it offers no extra safety net since when we send operations to specific IP:PORT so that should be enough. Regarding the cloud, REST is insecure regardless of this property, we should be intercepting the calls or keeping it completely disabled.

However, I agree that this might need a proper PRD and we could hold it back for 3.11.

[enesakar]

@tkountis This is is not specific to hazelcast cloud. Any hazelcast instance can be shutdown with this endpoint even if you disable REST. We can not ask all users to intercept these calls. Group name/password may not be a proven security but it is better than nothing.
Instead of workarounds we need to solve this:
https://hazelcast.atlassian.net/wiki/spaces/PM/pages/147718270/Security+Concerns+on+Shutting+Down+Cluster+via+REST+API

[enesakar]

This PR request should not be merged till we propoes a security solution to the : https://hazelcast.atlassian.net/wiki/spaces/PM/pages/147718270/Security+Concerns+on+Shutting+Down+Cluster+via+REST+API

[hasancelik]

@enesakar @kwart @tkountis I will prepare a PRD for this PR. I will also copy your comments into it so we can continue to discuss at the PRD.

[dbrimley]

FYI, I've raised the priority of the REST API security work, it's now in the voting for 3.12

[mmedenjak]

@hasancelik is this PR still valid?

[kwart]

Closing, this PR was replaced by #14386.