Fix TLS setup between SM and SM DB - #4801
Conversation
b5364ae to
4ff684a
Compare
81a5b3a to
1e77961
Compare
Before this commit, SSL_ENABLED test env var controlled just the managed clusters TLS setup. This commit makes it so it also controls SM DB TLS setup. All Scylla clusters use the same TLS certs for simplicity. Since TLS setup is controlled with SM DB and SM yaml configs, SM DB config is now generated in the same way as other Scylla configs were generated (using base file and modyfing it according to env vars). On the other hand, since SM config is small enough, it is still hardcoded. Additionally, this commit wraps IP addresses in SM config in quotes to ensure their correct parsing.
This was missed in the past, as our CI does not run SM DB discovery and configuration as in the prod env. Even though main the TLS CQL connection established by SM later on worked fine, SM startup used to fail when SM DB TLS port was not configured as the hardcoded 9042. Fixes https://scylladb.atlassian.net/browse/CLOUD-1187
Scylla TLS certs were not previously validated in our test env as we didn't generate Scylla TLS certs with proper SANs to verify. This commit adds the IPs (from all clusters and minio with all IP families) entries to DB TLS certs SANs extensions.
Even though the CA extensions used to allow CA to sign TLS certs were defined in its config, they were ignored by default. This commit fixes that by specifying them in the CA TLS cert creation cmd.
This commit makes it so make run-server can be used to validate that SM server started correctly.
This commit extends job running small pkg integration tests with a simple check that SM server starts correctly. This is not covered by our CI where we interact with SM services directly, not via running SM server. This check would catch simple issues like https://scylladb.atlassian.net/browse/CLOUD-1187. Refs https://scylladb.atlassian.net/browse/CLOUD-1187
cd .github/cfg && go run main.go
1e77961 to
aa0b027
Compare
karol-kokoszka
left a comment
karol-kokoszka
left a comment
There was a problem hiding this comment.
Please update "Install" with examples of how to setup TLS, and how to setup mTLS between Scylla Manager and Scylla Manager DB.
| database: | ||
| hosts: | ||
| - "192.168.200.100" | ||
| ssl: true |
There was a problem hiding this comment.
WDYT about removing "ssl: true" completely ?
It would make sense if we skip cert validation, but this PR is opposite - it forces certificate validation
There was a problem hiding this comment.
I don't think I follow. Removing ssl: true results in SM not using TLS when connecting to SM DB. This PR made it so SM DB is set up with TLS when SSL_ENABLED=true. So just removing it would result in errors. I guess that you just wanted to remove some cert validation. We can remove it from this config, as then we don't validate SM DB TLS cert which is required in default secure TLS. We could remove it from SM DB scylla.yaml, but since scylla allows for easy setup of mTLS and we already validate SM certs when connecting to first/second cluster nodes with TLS, I kept the mTLS setup. It's just that by default we have one-way TLS (scylla validates SM, SM doesn't validate scylla), so adding regular TLS results in mTLS out of the box.
There was a problem hiding this comment.
From UX perspective - I see ssl: true as an unnecessary boilerplate when you include certificates into the config.
Comment is about this UX exactly. But it's nit.
If we skip cert validation - then ssl: true without certs could mean - encrypt transmission but do not verify certs.
There was a problem hiding this comment.
Actually, there are small differences in behavior controlled by the ssl field.
It's true that if someone adds TLS certs to config, then we can infer that ssl: true.
On the other hand, if we are missing TLS certs, we might still want to use TLS against scylla that does not verify client certs.
Similar thing happens when we think about custom CAs. Setup with ssl: true, no TLS certs, no custom CA still uses TLS and validates TLS certs against system CAs.
We might want to think about making it so we always validate TLS certs against both system and custom CA (if specified), so that the CA case is more comprehensive, but for now we can't just remove the ssl: bool field.
There was a problem hiding this comment.
Or did you mean to just not require ss: true when any other TLS config field is specified, but keep this config option in general?
karol-kokoszka
left a comment
karol-kokoszka
left a comment
There was a problem hiding this comment.
not blocking PR - please update documentation with examples
"ssl: true/false" is just a nit.
PR is not just about TLS - it includes mTLS.
|
@karol-kokoszka I will add the docs in a followup PR today so that the CI is not wastefully rekicked. |
2 participants
This PR:
This PR extends db.cert with IP SANs for:
As all of those nodes use the same db.crt. In theory, SM doesn't validate first/second cluster nodes TLS certs, so their IPs are not needed there, but we plan to do it in the near future, so this is a preparation set towards that. It also aligns generation/usage of db.crt across all scylla nodes, which is nice.
This also fixes one additional issue that we had in our SSL_ENABLED=true test env for a long time - it wasn't possible to connect to first/second cluster with cqlsh, as our cqlshrc config does validate scylla TLS certs. This was not needed for any CI tests, but was annoying when debugging manually/with AI on a local test dev env.
Fixes https://scylladb.atlassian.net/browse/CLOUD-1187