Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

Nodetool SSL access via JMX #74

Merged
merged 13 commits into from
Apr 17, 2020
Merged

Nodetool SSL access via JMX #74

merged 13 commits into from
Apr 17, 2020

Conversation

shubhanilBag
Copy link
Contributor

@shubhanilBag shubhanilBag commented Apr 3, 2020

This PR-

  1. Enables remote JMX only when JMX_LOCAL_ONLY is disabled and TRANSPORT_ENCRYPTION_ENABLED is enabled
  2. Adds CM nodetool-ssl.properties
  3. Adds a new test in tls suite with JMX_LOCAL disabled, tests nodetool access from a separate pod
  4. Updates doc on using SSL JMX.

JIRA: https://jira.d2iq.com/browse/D2IQ-63186

Copy link
Contributor

@ANeumann82 ANeumann82 left a comment

Choose a reason for hiding this comment

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

@shubhanilBag Is it possible to change this so that JMX-SSL is separated from the TRANSPORT_ENCRYPTION_ENABLED? I think there might be reasonable use cases to have each of those enabled separately.

@zmalik @nfnt What's your opinion on this?

tests/cassandra/nodetool.go Show resolved Hide resolved
@shubhanilBag
Copy link
Contributor Author

shubhanilBag commented Apr 7, 2020

@shubhanilBag Is it possible to change this so that JMX-SSL is separated from the TRANSPORT_ENCRYPTION_ENABLED? I think there might be reasonable use cases to have each of those enabled separately.

@zmalik @nfnt What's your opinion on this?

Yeah it is possible. Then the responsibility lies on the user to enable TLS when local JMX is disabled. I tied TLS with SSL JMX in order to implement similar to this: Allow clusterwide access to JMX only if SSL is enabled ( or at least make it enabled by default if JMX is not local-only)
ref: https://jira.d2iq.com/browse/D2IQ-63186

reasonable use cases to have each of those enabled separately.

yeah people may want to do this, for this to be done TLS logic needs to be enabled in the operator only for generating tls artifacts but not enable node-node encryption in c* yaml (needs to be tested if this setup will not cause any errors), also fyi SSL JMX is indirectly enabled by disabling LOCAL_JMX flag

Copy link
Contributor

@nfnt nfnt left a comment

Choose a reason for hiding this comment

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

Yeah, we should separate TRANSPORT_ENCRYPTION_ENABLED and JMX_LOCAL_ONLY. E.g., what if as user wants node-to-node encryption but a local-only JMX port? While this might create a possibility for users to shoot themselves in the foot, we shouldn't make assumptions on the users configuration. Though, we need to make sure that we provide default values for these parameters that don't contradict themselves.

Please merge the latest master, liveness probes have been added that call nodetool info. Their call has to be updated to take a possible SSL connection into account.

@shubhanilBag
Copy link
Contributor Author

what if as user wants node-to-node encryption but a local-only JMX port?

@nfnt User can do this with the current setup. The only thing that this PR binds is to force TLS encryption (node to node only) only when user wants remote JMX (i.e local-only JMX set to "false"). I think it's also best practice to enable encryption when remote JMX is enabled and also without any JMX authentication right now.
but as I said we can prevent(i.e separate) node-to-node encryption when TLS is used with JMX,which is currently getting enabled when TRANSPORT_ENCRYPTION_ENABLED is set.

@nfnt
Copy link
Contributor

nfnt commented Apr 8, 2020

They can't, because the negation of https://github.com/mesosphere/kudo-cassandra-operator/pull/74/files#diff-23d348f721199b8500d3ee65aaca43edR252 is JMX_LOCAL_ONLY == true and TRANSPORT_ENCRYPTION_ENABLED == false which is assumed for the "else" case below. We need a nested if-clause to distinguish between these cases.
I'm wrong, the negation of "and" is "or".

@shubhanilBag
Copy link
Contributor Author

They can't, because the negation of https://github.com/mesosphere/kudo-cassandra-operator/pull/74/files#diff-23d348f721199b8500d3ee65aaca43edR252 is JMX_LOCAL_ONLY == true and TRANSPORT_ENCRYPTION_ENABLED == false which is assumed for the "else" case below. We need a nested if-clause to distinguish between these cases.

The node-to-node encryption is configured here https://github.com/mesosphere/kudo-cassandra-operator/blob/nil/ssl-jmx/operator/templates/cassandra-yaml.yaml#L1162

@zmalik
Copy link
Contributor

zmalik commented Apr 8, 2020

exposing un-secure JMX for remote access should be discouraged and not possible by default.
In the case of Cassandra, any JMX access can be used to invoke critical operations. Unless we have a feature request explaining why the insecure remote JMX access is required we shouldn't enable it. This feature was removed from some DC/OS data services.

https://jira.d2iq.com/browse/D2IQ-32989
https://jira.d2iq.com/browse/D2IQ-33506

Signed-off-by: Shubhanil Bag <[email protected]>
Signed-off-by: Shubhanil Bag <[email protected]>
Signed-off-by: Shubhanil Bag <[email protected]>
Signed-off-by: Shubhanil Bag <[email protected]>
Signed-off-by: Shubhanil Bag <[email protected]>
Signed-off-by: Shubhanil Bag <[email protected]>
Signed-off-by: Shubhanil Bag <[email protected]>
Signed-off-by: Shubhanil Bag <[email protected]>
Signed-off-by: Shubhanil Bag <[email protected]>
@shubhanilBag shubhanilBag requested a review from nfnt April 9, 2020 14:00
Copy link
Contributor

@ANeumann82 ANeumann82 left a comment

Choose a reason for hiding this comment

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

Nice! One small naming nit, apart from that it looks good.

I'm not a big fan of the ne JMX_LOCAL_ONLY "true" compare, but I also don't see a better way without renaming the parameter tos something like ENCRYPTED_REMOTE_JMX and i'm not sure if that's a better name.

tests/suites/tls/tls_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nfnt nfnt left a comment

Choose a reason for hiding this comment

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

LGTM! I share the same opinion about ne $.Params.JMX_LOCAL_ONLY "true as @ANeumann82. Though I see the consistency this creates and would rather see that. than eq $.Params.JMX_LOCAL_ONLY "false". And, it's just nitpicking, so please feel free to ignore if you think that changing this would change things for the worse.

operator/templates/cassandra-env-sh.yaml Show resolved Hide resolved
- cassandra-exporter-config-yml.yaml
- service-monitor.yaml
- node-scripts.yaml
- generate-tls-artifacts-sh.yaml
- generate-cqlshrc-sh.yaml
- pdb.yaml
- nodetool-ssl-properties.yaml
- stateful-set.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

the order was changed because of any issue?
I'm just asking if the order matters, we shouldn't keep them in same task but use it as two steps of a plan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we should do it like KUDO Kafka does, as sometimes the CM are created after the STS. I think a separate PR is better for that change.

Copy link
Contributor

@zmalik zmalik left a comment

Choose a reason for hiding this comment

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

LGTM! 👏

@shubhanilBag shubhanilBag merged commit 8731cc9 into master Apr 17, 2020
@shubhanilBag shubhanilBag deleted the nil/ssl-jmx branch April 17, 2020 08:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants