Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manager should not assume CQL-SSL in cluster sessions #3679

Closed
Tracked by #1674
rzetelskik opened this issue Jan 3, 2024 · 1 comment · Fixed by #3711
Closed
Tracked by #1674

Manager should not assume CQL-SSL in cluster sessions #3679

rzetelskik opened this issue Jan 3, 2024 · 1 comment · Fixed by #3711
Assignees
Milestone

Comments

@rzetelskik
Copy link
Member

rzetelskik commented Jan 3, 2024

What happened?

Currently, if you set up CQL-SSL in your ScyllaDB cluster, the manager sets up a TLS client when establishing a CQL session with a cluster, even if it wasn't provided with certs.

What did you expect to happen?

Scylla Manager should not use TLS by default. Scylla Manager should configure a TLS client when establishing a CQL session with a cluster only when specifically configured to do so.

How can we reproduce it?

  1. Set up a Scylla Manager instance
  2. Deploy a ScyllaDB cluster with the following config (omitting irrelevant fields)
...
client_encryption_options:
  certificate: /var/run/secrets/scylla-operator.scylladb.com/scylladb/serving-certs/tls.crt
  enabled: true
  keyfile: /var/run/secrets/scylla-operator.scylladb.com/scylladb/serving-certs/tls.key
  optional: false
  require_client_auth: true
  truststore: /var/run/secrets/scylla-operator.scylladb.com/scylladb/client-ca/tls.crt
native_transport_port: 9042
native_transport_port_ssl: 9142
...
  1. Register the cluster with Scylla Manager without providing the certs.
  2. Verify that Scylla Manager throws errors on a CQL session get attempt:
$ sctool restore -c scylla/scylla -L gcs:rzetelskik
Error: get resource: create restore target, units and views: create worker: get CQL cluster session: TLS/SSL key/cert is not registered: not found
Trace ID: 4nGqrQEUQU2e93kHWadCyw (grep in scylla-manager logs)
{"L":"INFO","T":"2024-01-03T13:39:45.929Z","N":"cluster","M":"Creating new Scylla REST client","cluster_id":"ae1376b9-2524-4e1d-a8f9-4a4681fc7d32","_trace_id":"4nGqrQEUQU2e93kHWadCyw"}
{"L":"INFO","T":"2024-01-03T13:39:45.933Z","N":"cluster.client","M":"Measuring datacenter latencies","dcs":["us-east-1"],"_trace_id":"4nGqrQEUQU2e93kHWadCyw"}
{"L":"INFO","T":"2024-01-03T13:39:45.946Z","N":"http","M":"POST /api/v1/cluster/scylla%2Fscylla/tasks","from":"127.0.0.1:36628","status":404,"bytes":203,"duration":"18ms","error":"get resource: create restore target, units and views: create worker: get CQL cluster session: TLS/SSL key/cert is not registered: not found","_trace_id":"4nGqrQEUQU2e93kHWadCyw"}

Similarly, sctool status fails on fetching the TLS config.

$ sctool status -c scylla/scylla
Datacenter: us-east-1
+----+-------------+----------+---------------+----------+------+--------+----------+-------+--------------------------------------+
|    | CQL         | REST     | Address       | Uptime   | CPUs | Memory | Scylla   | Agent | Host ID                              |
+----+-------------+----------+---------------+----------+------+--------+----------+-------+--------------------------------------+
| UN | ERROR (0ms) | UP (0ms) | 10.104.56.229 | 1h54m59s | 6    | 7.751G | 2023.1.3 | 3.2.5 | 1a903c90-35ca-4211-8c0f-113317868963 |
+----+-------------+----------+---------------+----------+------+--------+----------+-------+--------------------------------------+
Errors:
- 10.104.56.229 CQL: fetch TLS config: get SSL user cert from secrets store: not found
{"L":"ERROR","T":"2024-01-03T13:41:38.579Z","N":"healthcheck","M":"Node info fetch failed","cluster_id":"ae1376b9-2524-4e1d-a8f9-4a4681fc7d32","host":"10.104.56.229","error":"fetch TLS config: get SSL user cert from secrets store: not found","_trace_id":"GVkIpd4-TwmFWJRXHXZL8g","errorStack":"github.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).nodeInfo\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:422\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelNodeInfoFunc.func1.1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:155\ngithub.com/scylladb/scylla-manager/v3/pkg/util/parallel.Run\n\tgithub.com/scylladb/scylla-manager/v3/pkg/util/parallel/parallel.go:44\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelNodeInfoFunc.func1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:149\ngolang.org/x/sync/errgroup.(*Group).Go.func1\n\tgolang.org/x/sync@v0.2.0/errgroup/errgroup.go:75\nruntime.goexit\n\truntime/asm_amd64.s:1598\n","S":"github.com/scylladb/go-log.Logger.log\n\tgithub.com/scylladb/go-log@v0.0.7/logger.go:101\ngithub.com/scylladb/go-log.Logger.Error\n\tgithub.com/scylladb/go-log@v0.0.7/logger.go:84\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelNodeInfoFunc.func1.1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:157\ngithub.com/scylladb/scylla-manager/v3/pkg/util/parallel.Run\n\tgithub.com/scylladb/scylla-manager/v3/pkg/util/parallel/parallel.go:44\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelNodeInfoFunc.func1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:149\ngolang.org/x/sync/errgroup.(*Group).Go.func1\n\tgolang.org/x/sync@v0.2.0/errgroup/errgroup.go:75"}
{"L":"ERROR","T":"2024-01-03T13:41:38.587Z","N":"healthcheck","M":"CQL ping failed","cluster_id":"ae1376b9-2524-4e1d-a8f9-4a4681fc7d32","host":"10.104.56.229","error":"fetch TLS config: get SSL user cert from secrets store: not found","_trace_id":"GVkIpd4-TwmFWJRXHXZL8g","errorStack":"github.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).nodeInfo\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:422\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).pingCQL\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:345\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelCQLPingFunc.func1.1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:222\ngithub.com/scylladb/scylla-manager/v3/pkg/util/parallel.Run\n\tgithub.com/scylladb/scylla-manager/v3/pkg/util/parallel/parallel.go:44\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelCQLPingFunc.func1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:214\ngolang.org/x/sync/errgroup.(*Group).Go.func1\n\tgolang.org/x/sync@v0.2.0/errgroup/errgroup.go:75\nruntime.goexit\n\truntime/asm_amd64.s:1598\n","S":"github.com/scylladb/go-log.Logger.log\n\tgithub.com/scylladb/go-log@v0.0.7/logger.go:101\ngithub.com/scylladb/go-log.Logger.Error\n\tgithub.com/scylladb/go-log@v0.0.7/logger.go:84\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelCQLPingFunc.func1.1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:225\ngithub.com/scylladb/scylla-manager/v3/pkg/util/parallel.Run\n\tgithub.com/scylladb/scylla-manager/v3/pkg/util/parallel/parallel.go:44\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelCQLPingFunc.func1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:214\ngolang.org/x/sync/errgroup.(*Group).Go.func1\n\tgolang.org/x/sync@v0.2.0/errgroup/errgroup.go:75"}
{"L":"ERROR","T":"2024-01-03T13:41:38.593Z","N":"healthcheck","M":"Unable to fetch node information","error":"fetch TLS config: get SSL user cert from secrets store: not found","_trace_id":"GVkIpd4-TwmFWJRXHXZL8g","errorStack":"github.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).nodeInfo\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:422\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelCQLPingFunc.func1.1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:248\ngithub.com/scylladb/scylla-manager/v3/pkg/util/parallel.Run\n\tgithub.com/scylladb/scylla-manager/v3/pkg/util/parallel/parallel.go:44\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelCQLPingFunc.func1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:214\ngolang.org/x/sync/errgroup.(*Group).Go.func1\n\tgolang.org/x/sync@v0.2.0/errgroup/errgroup.go:75\nruntime.goexit\n\truntime/asm_amd64.s:1598\n","S":"github.com/scylladb/go-log.Logger.log\n\tgithub.com/scylladb/go-log@v0.0.7/logger.go:101\ngithub.com/scylladb/go-log.Logger.Error\n\tgithub.com/scylladb/go-log@v0.0.7/logger.go:84\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelCQLPingFunc.func1.1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:250\ngithub.com/scylladb/scylla-manager/v3/pkg/util/parallel.Run\n\tgithub.com/scylladb/scylla-manager/v3/pkg/util/parallel/parallel.go:44\ngithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck.(*Service).parallelCQLPingFunc.func1\n\tgithub.com/scylladb/scylla-manager/v3/pkg/service/healthcheck/service.go:214\ngolang.org/x/sync/errgroup.(*Group).Go.func1\n\tgolang.org/x/sync@v0.2.0/errgroup/errgroup.go:75"}

Scylla Manager version

3.2.5

Further details

Looking into the manager code, it seems that, when selecting a CQL port, it chooses between an SSL and non-SSL ports based on client encryption being enabled:

if ni.ClientEncryptionEnabled {

I think this is not correct and it should be configurable. As in the above example, you can configure both cql-ssl and cql ports, and the manager shouldn't just assume which one to use.

cc @Michal-Leszczynski @karol-kokoszka

@tnozicka
Copy link

tnozicka commented Jan 4, 2024

I think, in the end, whether to use TLS or not should be a per cluster option, the same as the TLS certs.

That said, at least a global option would be a good start.

vponomaryov added a commit to vponomaryov/scylla-cluster-tests that referenced this issue Jan 19, 2024
Starting with the 'v1.11.0' version of the scylla-operator the TLS
feature gets enabled by default.
At first, we do not need it.
At second, it makes the scylla-manager operations fail [1].

So, disable it explicitly if we do not expect it to overwrite the
default setting.

[1] scylladb/scylla-manager#3679
vponomaryov added a commit to vponomaryov/scylla-cluster-tests that referenced this issue Jan 19, 2024
Starting with the 'v1.11.0' version of the scylla-operator the TLS
feature gets enabled by default.
At first, we do not need it.
At second, it makes the scylla-manager operations fail [1].

So, disable it explicitly if we do not expect it to overwrite the
default setting.

[1] scylladb/scylla-manager#3679
fruch pushed a commit to scylladb/scylla-cluster-tests that referenced this issue Jan 21, 2024
Starting with the 'v1.11.0' version of the scylla-operator the TLS
feature gets enabled by default.
At first, we do not need it.
At second, it makes the scylla-manager operations fail [1].

So, disable it explicitly if we do not expect it to overwrite the
default setting.

[1] scylladb/scylla-manager#3679
@karol-kokoszka karol-kokoszka self-assigned this Jan 26, 2024
@karol-kokoszka karol-kokoszka added this to the 3.2.6 milestone Feb 1, 2024
karol-kokoszka added a commit that referenced this issue Feb 8, 2024
karol-kokoszka added a commit that referenced this issue Feb 9, 2024
karol-kokoszka added a commit that referenced this issue Feb 12, 2024
karol-kokoszka added a commit that referenced this issue Feb 13, 2024
karol-kokoszka added a commit that referenced this issue Feb 13, 2024
karol-kokoszka added a commit that referenced this issue Feb 13, 2024
karol-kokoszka added a commit that referenced this issue Feb 13, 2024
karol-kokoszka added a commit that referenced this issue Feb 14, 2024
karol-kokoszka added a commit that referenced this issue Feb 14, 2024
karol-kokoszka added a commit that referenced this issue Feb 14, 2024
karol-kokoszka added a commit that referenced this issue Feb 22, 2024
karol-kokoszka added a commit that referenced this issue Feb 22, 2024
karol-kokoszka added a commit that referenced this issue Feb 22, 2024
karol-kokoszka added a commit that referenced this issue Feb 22, 2024
karol-kokoszka added a commit that referenced this issue Feb 22, 2024
karol-kokoszka added a commit that referenced this issue Feb 26, 2024
karol-kokoszka added a commit that referenced this issue Feb 26, 2024
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 a pull request may close this issue.

4 participants