THREESCALE-8404: Add TLS and ACL support for Redis #3572

jlledom · 2023-09-27T07:27:30Z

What this PR does / why we need it:

This implements support for ACL and TLS credentials for Redis from Porta.

After Sidekiq 7 upgrade, this PR has become pretty trivial: It only requires some minimal changes in the Redis config parser class to be able to read SSL params from yaml config files. It also loads a default CA cert from the /config folder if one is needed and no other certificate is provided.

All other changes are on tests. Also, all yaml example files have been updated.

Which issue(s) this PR fixes

THREESCALE-8404

Verification steps

This requires three steps:

Generate TLS certificates and keys for Redis.
Configure Redis to enable ACL and TLS, and use the generated keys.
Configure Porta.
3.1. TLS Client
3.2. Mutual TLS Client

Generate keys and certificates:

The first thing we need is a certification authority. For that, we need to create its key and certificate.

key: openssl genrsa -out ca-root-key.pem 4096
cert: openssl req -new -x509 -days 365 -key ca-root-key.pem -out ca-root-cert.pem

Then we'll have to create a key and certificate for the server, and sign it with our CA. Ensure setting localhost as CN, if that's the domain you're going to use to connect to Redis:

key: openssl genrsa -out redis.key 4096
cert request: openssl req -new -key redis.key -out redis.csr
signed cert: openssl x509 -req -days 365 -in redis.csr -CA ca-root-cert.pem -CAkey ca-root-key.pem -CAcreateserial -out redis.crt

Configure Redis:

First, we need a Redis configuration file which enables ACL and TLS, defines the users and sets the certificate the server will use and the CA the server will trust. This is the minimal redis.conf for that purpose:

port 6379
tls-port 26379
tls-cert-file /etc/redis.crt
tls-key-file /etc/redis.key
tls-ca-cert-file /etc/ca-root-cert.pem
tls-auth-clients optional
user default off
user porta on >sup3rS3cre1! ~* &* +@all
user apisonator on >secret#Passw0rd ~* &* +@all

Next step is to launch a container that has access to the certificates, keys and Redis config files created above. We create a volume for each file and modify the container start command to reference the configuration file we created. This is how I configured the redis pod in my docker-compose.yml. But the container can be launched by other ways:

  redis:
    image: redis:6.2-alpine
    container_name: redis-compose-ssl
    ports:
      - "26379:26379"
    volumes:
      - /home/jlledom/redis.conf:/etc/redis.conf:Z
      - /home/jlledom/redis.crt:/etc/redis.crt:z
      - /home/jlledom/redis.key:/etc/redis.key:z
      - /home/jlledom/ca-root-cert.pem:/etc/ca-root-cert.pem:z
    command: [redis-server, /etc/redis.conf]

The :z at the end of the volume definitions is required when using Selinux, for instance in Fedora.

Configure Porta:

Porta can be configured as a regular TLS client, when only the server needs a certificate, or a Mutual TLS Client, where both client and server must provide a certificate.

As TLS Client:

Update our config files to add a new base configuration:

redis-yml:

ssl: &ssl
  url: "<%= ENV.fetch('REDIS_URL', 'rediss://localhost:26379/5') %>"
  pool_size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>
  pool_timeout: 5 # this is in seconds
  namespace: "<%= ENV['REDIS_NAMESPACE'] %>"
  sentinels: "<%= ENV['REDIS_SENTINEL_HOSTS'] %>"
  role: <%= ENV['REDIS_SENTINEL_ROLE'] %>
  username: porta
  password: sup3rS3cre1!
  ssl_params:
    ca_file: /home/jlledom/ca-root-cert.pem

backend-redis-yml:

ssl: &ssl
  url: "<%= ENV.fetch('BACKEND_REDIS_URL', 'rediss://localhost:26379/6') %>"
  pool_size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>
  pool_timeout: 5 # this is in seconds
  sentinels: "<%= ENV['BACKEND_REDIS_SENTINEL_HOSTS'] %>"
  role: <%= ENV['BACKEND_REDIS_SENTINEL_ROLE'] %>
  username: porta
  password: sup3rS3cre1!
  ssl_params:
    ca_file: /home/jlledom/ca-root-cert.pem

Note how the ssl_params section is not always required. We are using it here because the server is using a certificate signed by an unknown authority we've just created, so we need to explicitly tell Porta to trust that authority. If the server were using a certificate signed by any of the well-known CAs, the ssl_params section could be omitted.

Another possible situation is when the server is using a self-signed certificate. When this happens, there's no trusted CA to add to ssl_params, so the only way to go is to skip certificate validation. Unfortunately we don't support this yet because the redis-client gem still haven't added support for verify modes (see: redis-rb/redis-client#133) so we can't set SSL_VERIFY_NONE to the client. Anyway this is only useful for development purposes, we'll have to use our own CA for development for the moment.

As Mutual TLS Cliet:

Generate a key and a singed certificate for Porta:

key: openssl genrsa -out porta.key 4096
cert request: openssl req -new -key porta.key -out porta.csr
signed cert: openssl x509 -req -days 365 -in porta.csr -CA ca-root-cert.pem -CAkey ca-root-key.pem -CAcreateserial -out porta.crt

Update our config files to add a new base configuration:

redis-yml:

ssl: &ssl
  url: "<%= ENV.fetch('REDIS_URL', 'rediss://localhost:26379/5') %>"
  pool_size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>
  pool_timeout: 5 # this is in seconds
  namespace: "<%= ENV['REDIS_NAMESPACE'] %>"
  sentinels: "<%= ENV['REDIS_SENTINEL_HOSTS'] %>"
  role: <%= ENV['REDIS_SENTINEL_ROLE'] %>
  username: porta
  password: sup3rS3cre1!
  ssl_params:
    ca_file: /home/jlledom/ca-root-cert.pem
    cert: /home/jlledom/porta.crt
    key: /home/jlledom/porta.key

backend-redis-yml:

ssl: &ssl
  url: "<%= ENV.fetch('BACKEND_REDIS_URL', 'rediss://localhost:26379/6') %>"
  pool_size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>
  pool_timeout: 5 # this is in seconds
  sentinels: "<%= ENV['BACKEND_REDIS_SENTINEL_HOSTS'] %>"
  role: <%= ENV['BACKEND_REDIS_SENTINEL_ROLE'] %>
  username: porta
  password: sup3rS3cre1!
  ssl_params:
    ca_file: /home/jlledom/ca-root-cert.pem
    cert: /home/jlledom/porta.crt
    key: /home/jlledom/porta.key

As mentioned above, the ca_file field is only required when the server isn't using a certificate signed by one of the well-known CAs.

Enable the TLS Client:

You can switch between TLS and non-TLS client by updating the development environment to use the new base, on both files:

development:
  <<: *ssl

At this point it should work. You can verify the connection using RedisInsight or trying with invalid certs or keys from the porta side, to verify it works fine.

config/initializers/sidekiq_hacks.rb

jlledom · 2023-09-28T14:41:11Z

@akostadinov @mayorova I added some comments

jlledom · 2023-10-03T15:25:59Z

I updated the PR description to add the use case when Porta doesn't provide a certificate.

github-actions · 2023-10-19T03:34:11Z

This PR is stale because it has not received activity for more than 14 days. Remove stale label or comment or this will be closed in 7 days.

app/lib/three_scale/redis_config.rb

lib/tasks/boot.rake

config/examples/redis.yml

akostadinov · 2023-10-24T16:23:32Z

openshift/system/config/redis.yml

+  password: <%= ENV['REDIS_ACL_PASSWORD'] %>
+  # == TLS Params ==
+  ssl_params:
+    ca_file: <%= ENV['REDIS_TLS_CA_FILE'] %>


I thought I commented somewhere on this. I would like first to know how will customers be providing their CA (perhaps depends on operator) before finalizing this variable. To make sure that this is the expected way it should work. At the very least I think we should have a default location to search for this so that users can only mount a secret and then the files are present, porta to use them.

The point about default location applies to the cert and public key variables as well.

Yes, you mentioned that in the Apisonator PR. If not provided, Redis will verify the server certificate against all trusted certificates in the OS default ssl certs folder. In Fedora that's /etc/ssl/certs.

I would say the ca_path param would work out of the box (See hiredis-client) to provide an alternative directory to /etc/ssl/certs. I can add a new env variable for it, so let the clients use ca_path or ca_file as they want.

AFAIK there's no equivalent config param for client certs and keys folder, but I don't think it's really needed, in OpenShift you can mount folders as well as single files.

I pushed some changes to allow looking for trusted CA certs through ca_path. However, you can't just store your cert file under ca_path, that doesn't work (check redis/redis#7392). You need to manually install your certificate in your OS using the official way. For Fedora, that's update-ca-trust. And, since this mechanism essentially stores the cert in the proper format in the default folder (/etc/ssl/certs for Fedora), there's no difference between providing or not the ca_path param.

I also tried setting ca_path to a directory under my $HOME which is a symbolic link to /etc/ssl/certs, and it worked fine.

Summarizing, I don't think ca_path is much useful but since it's an officially accepted parameter and I already added the code, I think it's OK to provide it, It might be useful to somebody.

I think we are not on the same page here. I'm asking what will be the documented way to mount the certs/key in OpenShift. I know how to create a ca path (sorry for writing ca dir previously) with openssl-rehash. But probably that wont be the easiest option to mange as an openshift secret. Also the documented way will show what default location to use.

IMO if we provide both ca_file and ca_path then we are providing all available options for the operator team to define where to place the files or how to mount the secrets.

Sounds good to. Probably filename should be ca_redis.pem or something like that.

Do we have documented that users can edit this secret?

Sounds good to. Probably filename should be ca_redis.pem or something like that.

I think ca_cert.pem is better because this doesn't need to be exclusive for Redis. We provide a way for the user to define in which CAs they want porta to trust, it can be for Redis or anything else in the future. ca_cert.pem doesn't need to be a single certificate, a certificate bundle is also accepted.

Do we have documented that users can edit this secret?

This is a new feature, not documented.

I added this in 81762db.

I finally took config/ as the default path for the certificate. It turns out than EXTRA_CONFIGS_DIR is just a workaround to create symbolic links in config/ (check the code). So whatever secret the user mounts on EXTRA_CONFIGS_DIR will be available from config/.

ok, nice. Maybe test that this works, will also help write instructions how to do by customers.

I updated the Jira issue description with a "To Document" section

akostadinov · 2023-10-24T16:25:47Z

openshift/system/config/redis.yml

+    cert: <%= ENV['REDIS_TLS_CERT'] %>
+    key: <%= ENV['REDIS_TLS_KEY'] %>


Suggested change

cert: <%= ENV['REDIS_TLS_CERT'] %>

key: <%= ENV['REDIS_TLS_KEY'] %>

cert: <%= ENV['REDIS_USER_CERT'] %>

key: <%= ENV['REDIS_PRIVATE_KEY'] %>

This naming makes it more clear about the intended purpose of these variables. TLS is redundant in this case I believe. What else can certificate be about? I'm not super much against the TLS part but it's nicer without I believe.

I like the original names (i.e. REDIS_TLS_CERT and REDIS_TLS_KEY), because they match how Redis itself calls them, see https://redis.io/docs/management/security/encryption/#running-manually

Also, isn't it called "client certificate" rather than "user"?... But maybe I'm misinterpreting what ssl_params.cert represents 😬

they match how Redis itself calls them, see https://redis.io/docs/management/security/encryption/#running-manually

They match the server parameters names, but we are the client here.

Also, isn't it called "client certificate" rather than "user"?... But maybe I'm misinterpreting what ssl_params.cert represents 😬

As Porta is the client, cert is the client certificate.

What about these options?

REDIS_CA_FILE, REDIS_CERT and REDIS_KEY

To match params names

REDIS_CLIENT_CA_FILE, REDIS_CLIENT_CERT and REDIS_CLIENT_KEY

To note it's the client, and match params names

What about these options?

REDIS_CA_FILE, REDIS_CERT and REDIS_KEY

To match params names

REDIS_CLIENT_CA_FILE, REDIS_CLIENT_CERT and REDIS_CLIENT_KEY

To note it's the client, and match params names

I think CLIENT_CA_FILE is not necessarily accurate, it's not client or server, it's just CA, isn't it? 🙃

REDIS_CA_FILE and REDIS_CERT look good, but REDIS_KEY IMO is too vague.

So, I'd be sticking with the original options. Optionally, _TLS_ could be replaced with _SSL_, but probably not good, because TLS term kind of supersedes SSL.

I think CLIENT_CA_FILE is not necessarily accurate, it's not client or server, it's just CA, isn't it? 🙃

Right!! 😅

REDIS_CA_FILE and REDIS_CERT look good, but REDIS_KEY IMO is too vague.

So, I'd be sticking with the original options.

What do you mean? Do you have an example of variable names?

What do you mean? Do you have an example of variable names?

I mean that I wouldn't change what you have now :)

ssl_params: ca_file: <%= ENV['REDIS_TLS_CA_FILE'] %> ca_path: <%= ENV['REDIS_TLS_CA_PATH'] %> cert: <%= ENV['REDIS_TLS_CERT'] %> key: <%= ENV['REDIS_TLS_KEY'] %>

True, "client certificate". For me TLS is redundant as I said but I can live with it. More important for me is to have CLIENT_CERT and PRIVATE_KEY. This makes things clear beyond doubt.

We don't have to follow redis naming conventions rather we better establish a clear configuration naming conventions for porta.

I finally set them to

ssl_params: ca_file: <%= ENV['REDIS_CA_FILE'] %> ca_path: <%= ENV['REDIS_CA_PATH'] %> cert: <%= ENV['REDIS_CLIENT_CERT'] %> key: <%= ENV['REDIS_PRIVATE_KEY'] %>

I hope you both agree on these.

I'm fine with these.

mayorova · 2023-10-26T11:09:58Z

config/examples/redis.yml

@@ -5,6 +5,15 @@ base: &default
  sentinels: "<%= ENV['REDIS_SENTINEL_HOSTS'] %>"
  name: <%= ENV['REDIS_SENTINEL_NAME'] %>
  role: <%= ENV['REDIS_SENTINEL_ROLE'] %>
+  # == ACL Params ==
+  username: <%= ENV['REDIS_USERNAME'] %>
+  password: <%= ENV['REDIS_PASSWORD'] %>


I suggest to wrap username-password, and not sure if any other vars into "", as having special characters in the password (following the example in README) seems to be breaking YAML:

Uncaught exception: YAML syntax error occurred while parsing /home/dmayorov/Projects/3scale/porta/config/redis.yml. Please note that YAML must be consistently indented using spaces. Tabs are not allowed. Error: (<unknown>): did not find expected comment or line break while scanning a block scalar at line 11 column 13

UPDATE: the culprit seems to be the character > that I accidentally made part of the password, but still :) users may choose any special characters for their password, and we need to avoid breaking YAML...

Actually it might be even better to #to_json them. Otherwise a password like my nice password with " sign will break the quoting. While #to_json should handle all cases.

Then #presence should be used on the values to check them as they will never be nil I assume.

Fixed:

# == ACL Params == username: <%= ENV['REDIS_USERNAME']&.to_json %> password: <%= ENV['REDIS_PASSWORD']&.to_json %>

This works fine for nil, > and ", and I assume any other character.

Actually nil.to_json results in null so I think the & is redundant.

I tried removing the & and it still works. So you're right. But I'm still not sure, because:

$ bundle exec irb irb(main):001:0> nil.to_json Traceback (most recent call last): ... NoMethodError (undefined method `to_json' for nil:NilClass) irb(main):002:0> nil&.to_json => nil

But

$ bundle exec rails c Loading development environment (Rails 6.0.6.1) [1] pry(main)> nil.to_json => "null"

So the & is only redundant assuming Rails is loaded the moment the yaml is read., which I think we can assume it'll be always true. But I don't like Rails returning a string "null", and sending that string to Redis, rather than a plain Ruby nil which we know for sure it won't cause problems on the Redis gems.

So I'd prefer to keep the &.

This is a template file which should result in valid YAML which in turn would be loaded by Rails. One option would result in:

field:

The other:

field: null

Both result in equivalent YAML, nothing to do with rails.

🐚 pry [1] pry(main)> require 'yaml' => true [2] pry(main)> YAML.load "field: " => {"field"=>nil} [3] pry(main)> YAML.load "field: null" => {"field"=>nil}

One other concern though is that either way (using & or not) an empty environment variable will result in the empty string "". While your original version would treat and empty value for the variable as nil. I can't immediately say whether this is a problem or not.

Fair enough, I removed the &

FYI in case it matters, to have the original behavior where unset variable is equivalent to the empty one, we could use ENV['REDIS_PASSWORD'].presence.to_json

mayorova · 2023-10-26T14:03:45Z

Except for the comment I left here: #3572 (comment) everything seems to be working well! Nice 👏

github-actions · 2023-11-14T03:34:05Z

This PR is stale because it has not received activity for more than 14 days. Remove stale label or comment or this will be closed in 7 days.

app/lib/three_scale/redis_config.rb

akostadinov

Except for the renaming comment, all looks great!

app/lib/three_scale/redis_config.rb

github-actions · 2023-12-06T03:34:11Z

This PR is stale because it has not received activity for more than 14 days. Remove stale label or comment or this will be closed in 7 days.

github-actions · 2024-04-11T03:34:06Z

This PR is stale because it has not received activity for more than 30 days. Remove stale label or comment or this will be closed in 15 days.

akostadinov · 2024-04-26T06:19:36Z

Another possible situation is when the server is using a self-signed certificate. When this happens, there's no trusted CA to add to ssl_params, so the only way to go is to skip certificate validation.

With self-signed certificate, you should be able to use the certificate itself as CA too. Should work OOB or maybe you need also to mark it as CA when creating.

akostadinov

Looks great! Just a few minor comments.

akostadinov · 2024-04-26T06:29:14Z

app/lib/three_scale/redis_config.rb

+    def load_default_ca_cert(conf)
+      return if conf.dig(:ssl_params, :ca_file).present? || conf.dig(:ssl_params, :ca_path).present?
+
+      cert_path = Rails.root.join('config/ca_cert.pem').to_s


Another question. If customer has put this file into the OCP secret, will it end up and be found here automatically?

It's not automatic unless implemented by the operator. Otherwise the user must edit the yamls to mount the certificate at /opt/system/config or /opt/system-extra-configs.

For me that's the whole point of he earlier discussion - when the file is added to the secret, to be used automatically without further configuration. Otherwise it is unnecessary complicated for the users.

What could we do from porta?

We need to coordinate with operator folks how these can be mounted. For CA and cert files, we can put them in config maps I guess and have them mounted to default locations. For private keys though we need to use secrets and idk what is the best way to handle these.

Why do we need to load a default CA cert in the first place? 🤔

I'd say - if nothing is provided in :ca_file or :ca_path explicitly, so be it? Does checking config/ca_cert.pem help in any way?...

If there's no :ca_file or :ca_path it checks config/ca_cert.pem to load that one. If it doesn't exist, then it loads the system cert bundle in /etc.

The idea behind config/ca_cert.pem is to provide a way for the user to use its own certificate effortlessly.

But I think the effort is pretty much the same in both cases, just that the user will always need to set an env var for :ca_file or :ca_path. But at least it's clear where the cert should be mounted (wherever these env vars point to).

I am not sure we need to define some porta-specific CA cert location 🤷
But whatever, if you think it's useful, it's OK for me.

I've been discussing with Eguz on how to integrate this TLS anc ACL with the operator, and we came to these two Jira issues:

https://issues.redhat.com/browse/THREESCALE-11020
https://issues.redhat.com/browse/THREESCALE-11021

Basically, porta and apisonator should limit to read the environment variables and it would be the operator who mounts the certificates and set the environment variables according to some new YAML keywords.

I made some changes to adapt to the new approach.

akostadinov · 2024-04-26T06:39:29Z

test/unit/three_scale/redis_config_test.rb

+      FakeFS.with_fresh do
+        FakeFS::FileSystem.clone(file_fixture_path)
+        FakeFS::FileSystem.clone(Rails.root.join('config'))
+        FileUtils.cp file_fixture('ca_cert.pem'), Rails.root.join('config')


you can just create an empty file instead of keeping a fixture in the repo

FileUtils.touch

Fixed fcc87b2

akostadinov · 2024-04-26T06:39:35Z

test/unit/three_scale/redis_config_test.rb

+      FakeFS.with_fresh do
+        FakeFS::FileSystem.clone(file_fixture_path)
+        FakeFS::FileSystem.clone(Rails.root.join('config'))
+        FileUtils.cp file_fixture('ca_cert.pem'), Rails.root.join('config')


you can just create an empty file instead of keeping a fixture in the repo

FileUtils.touch

Fixed fcc87b2

mayorova · 2024-05-01T10:43:32Z

config/examples/redis.yml

+  username: <%= ENV['REDIS_USERNAME'].to_json %>
+  password: <%= ENV['REDIS_PASSWORD'].to_json %>
+  # == TLS Params ==
+  ssl: <%= ENV['REDIS_SSL'] %>


I'm a bit confused by this setting... redis-client docs say:

ssl: Whether to connect using SSL or not.

But I am not sure how it is actually used 🤔

If I set it to false but use rediss:// in the URL, it still connects via SSL. So, is it really needed? 🙃 Or does it only apply when the URL is provided in a different format? e.g. host:port with no protocol.

Also, if this value is expected to be a boolean, I think we need to cast the env var value (which is considered a string).

I think for most of the boolean values we are using this pattern:

ssl: <%= ENV.fetch('REDIS_SSL', '0') == '1' %>

See settings.yml

So, it needs to be set as REDIS_SSL=1 to be true.

I don't like it too much, but we have it everywhere. An alternative would be to set REDIS_SSL=true but then change the config to

ssl: <%= ActiveModel::Type::Boolean.new.cast(ENV['REDIS_SSL']) || false %>

We discussed this with @akostadinov here: #3107 (comment)

REDIS_SSL is optional, because the gem is able to infer it correctly in most cases. But it doesn't infer it correctly for the sentinels + tls scenario. That scenario requires setting it manually.

I'll check the boolean format issue.

Solved parsing REDIS_SSL 3e537ba

mayorova · 2024-05-01T14:02:31Z

config/examples/redis.yml

@@ -3,8 +3,20 @@ base: &default
  size: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>
  pool_timeout: 5 # this is in seconds
  sentinels: "<%= ENV['REDIS_SENTINEL_HOSTS'] %>"
+  sentinel_username: <%= ENV['REDIS_SENTINEL_USERNAME'].to_json %>


I haven't tested these, but I guess it should just work, as we just pass the config to the redis client.

I tried locally and it works

mayorova

Works for me with the following configs:

REDIS_SSL=1

REDIS_USERNAME=porta
REDIS_PASSWORD=portapassword
REDIS_CA_FILE=/path/to/ca-root-cert.pem
REDIS_CLIENT_CERT=/path/to/porta.crt
REDIS_PRIVATE_KEY=/path/to/porta.key

REDIS_URL=redis://redis-master
REDIS_SENTINEL_HOSTS=localhost:56380,localhost:56381,localhost:56382
REDIS_SENTINEL_NAME=redis-master
REDIS_SENTINEL_ROLE=master
REDIS_SENTINEL_USERNAME=sentuser
REDIS_SENTINEL_PASSWORD=sentpass


BACKEND_REDIS_SSL=1

BACKEND_REDIS_USERNAME=apisonator
BACKEND_REDIS_PASSWORD=apisonatorpassword
BACKEND_REDIS_CA_FILE=/path/to/ca-root-cert.pem
BACKEND_REDIS_CLIENT_CERT=/path/to/apisonator.crt
BACKEND_REDIS_PRIVATE_KEY=/path/to/apisonator.key

BACKEND_REDIS_URL=redis://redis-master
BACKEND_REDIS_SENTINEL_HOSTS=localhost:56380,localhost:56381,localhost:56382
BACKEND_REDIS_SENTINEL_NAME=redis-master
BACKEND_REDIS_SENTINEL_ROLE=master
BACKEND_REDIS_SENTINEL_USERNAME=sentuser
BACKEND_REDIS_SENTINEL_PASSWORD=sentpass

Great job 👍

akostadinov · 2024-05-07T12:24:16Z

config/examples/backend_redis.yml

+    ca_file: <%= ENV['BACKEND_REDIS_CA_FILE'] %>
+    cert: <%= ENV['BACKEND_REDIS_CLIENT_CERT'] %>
+    key: <%= ENV['BACKEND_REDIS_PRIVATE_KEY'] %>


If my assumption is correct, one can enable ssl with just CA when client auth is not needed. So we should still allow cert and key to be omitted.

Also I believe we should allow CA_FILE to be nil when the default operating system CA is suitable for the connection.

Suggested change

ca_file: <%= ENV['BACKEND_REDIS_CA_FILE'] %>

cert: <%= ENV['BACKEND_REDIS_CLIENT_CERT'] %>

key: <%= ENV['BACKEND_REDIS_PRIVATE_KEY'] %>

ca_file: <%= ENV['BACKEND_REDIS_CA_FILE'].to_json %>

cert: <%= ENV['BACKEND_REDIS_CLIENT_CERT'].to_json %>

key: <%= ENV['BACKEND_REDIS_PRIVATE_KEY'].to_json %>

Yes, they all can be nil. No problem

akostadinov · 2024-05-07T12:27:41Z

config/examples/redis.yml

+  ssl_params:
+    ca_file: <%= ENV['REDIS_CA_FILE'] %>
+    cert: <%= ENV['REDIS_CLIENT_CERT'] %>
+    key: <%= ENV['REDIS_PRIVATE_KEY'] %>


Suggested change

ssl_params:

ca_file: <%= ENV['REDIS_CA_FILE'] %>

cert: <%= ENV['REDIS_CLIENT_CERT'] %>

key: <%= ENV['REDIS_PRIVATE_KEY'] %>

ssl_params:

ca_file: <%= ENV['REDIS_CA_FILE'].to_json %>

cert: <%= ENV['REDIS_CLIENT_CERT'].to_json %>

key: <%= ENV['REDIS_PRIVATE_KEY'].to_json %>

wait, ignore this, it will result in nil anyway, correct? So no need to apply this. Will it handle spaces in path without to_json?

What's the call to to_json for anyway?

to_json will escape the values if needed

akostadinov · 2024-05-07T12:29:04Z

openshift/system/config/backend_redis.yml

it's fine if you leave it for another time, but would be really nice to get rid of these openshift specific files, is there anything different than the default config files?

it's fine if you leave it for another time, but would be really nice to get rid of these openshift specific files, is there anything different than the default config files?

@akostadinov let's cover this in #3766

Once this PR is merged, I'll incorporate the changes there.

test/unit/three_scale/redis_config_test.rb

akostadinov · 2024-05-07T12:40:05Z

config/examples/redis.yml

+  username: <%= ENV['REDIS_USERNAME'].to_json %>
+  password: <%= ENV['REDIS_PASSWORD'].to_json %>
+  # == TLS Params ==
+  ssl: <%= ENV.fetch('REDIS_SSL', '0') == '1' %>


I'm a little concerned that one may set CA without updating this value. But maybe unnecessary complication to try to prevent this.

In that situation the Redis client will infer ssl: true. This variable is optional in almost all cases, it's only mandatory when connecting to TLS protected sentinels.

Check: redis/redis-rb#1249

I'm just wondering, when this is set to false hopefully it wont enforce no SSL

I'm just wondering, when this is set to false hopefully it wont enforce no SSL

I believe it will ONLY enable SSL when REDIS_SSL=1, it will be disabled with any other value.

I believe it will ONLY enable SSL when REDIS_SSL=1, it will be disabled with any other value.

True

I'm just wondering, when this is set to false hopefully it wont enforce no SSL

No, you can ignore the value or set it to 0, false or whatever and it still connects to redis if you provide a certificate. The scenario you mention would only fail for sentinels + TLS, where REDIS_SSL=1 is mandatory.

To accept `ssl_params` and load a default CA cert if provided

And update example files

jlledom self-assigned this Sep 27, 2023

jlledom marked this pull request as ready for review September 27, 2023 07:40

jlledom requested a review from a team September 27, 2023 07:41

jlledom commented Sep 27, 2023

View reviewed changes

config/initializers/sidekiq_hacks.rb Outdated Show resolved Hide resolved

config/initializers/sidekiq_hacks.rb Outdated Show resolved Hide resolved

jlledom marked this pull request as draft October 4, 2023 14:55

github-actions bot added the Stale label Oct 19, 2023

jlledom added on hold Waiting for or blocked by something else. and removed Stale labels Oct 19, 2023

jlledom force-pushed the THREESCALE-8404-redis-acl-tls branch from b394b48 to 1da3043 Compare October 24, 2023 15:11

jlledom marked this pull request as ready for review October 24, 2023 15:16

jlledom removed the on hold Waiting for or blocked by something else. label Oct 24, 2023

jlledom commented Oct 24, 2023

View reviewed changes

app/lib/three_scale/redis_config.rb Show resolved Hide resolved

lib/tasks/boot.rake Outdated Show resolved Hide resolved

jlledom requested review from mayorova and akostadinov October 24, 2023 15:58

mayorova reviewed Oct 24, 2023

View reviewed changes

config/examples/redis.yml Outdated Show resolved Hide resolved

akostadinov reviewed Oct 24, 2023

View reviewed changes

mayorova reviewed Oct 26, 2023

View reviewed changes

github-actions bot added the Stale label Nov 14, 2023

jlledom removed the Stale label Nov 14, 2023

akostadinov reviewed Nov 17, 2023

View reviewed changes

app/lib/three_scale/redis_config.rb Outdated Show resolved Hide resolved

jlledom requested review from mayorova and akostadinov November 20, 2023 09:24

akostadinov previously approved these changes Nov 21, 2023

View reviewed changes

app/lib/three_scale/redis_config.rb Outdated Show resolved Hide resolved

jlledom removed the Stale label Feb 13, 2024

jlledom mentioned this pull request Mar 15, 2024

THREESCALE-9596: Upgrade to Sidekiq 7 #3723

Merged

github-actions bot added the Stale label Apr 11, 2024

jlledom removed the Stale label Apr 11, 2024

jlledom removed the on hold Waiting for or blocked by something else. label Apr 24, 2024

jlledom force-pushed the THREESCALE-8404-redis-acl-tls branch 3 times, most recently from a3f0372 to bd3df29 Compare April 25, 2024 10:52

jlledom requested a review from akostadinov April 25, 2024 11:15

akostadinov reviewed Apr 26, 2024

View reviewed changes

jlledom requested a review from akostadinov April 26, 2024 11:15

mayorova reviewed May 1, 2024

View reviewed changes

jlledom requested a review from mayorova May 6, 2024 11:36

mayorova previously approved these changes May 7, 2024

View reviewed changes

akostadinov reviewed May 7, 2024

View reviewed changes

test/unit/three_scale/redis_config_test.rb Outdated Show resolved Hide resolved

akostadinov reviewed May 7, 2024

View reviewed changes

jlledom dismissed mayorova’s stale review via 1e2d428 May 7, 2024 15:29

mayorova approved these changes May 7, 2024

View reviewed changes

akostadinov approved these changes May 7, 2024

View reviewed changes

jlledom added 3 commits May 7, 2024 17:54

Update redis config parser

4a2d498

To accept `ssl_params` and load a default CA cert if provided

Add new ENV variables

37d3229

And update example files

Add and update tests

7a35601

jlledom force-pushed the THREESCALE-8404-redis-acl-tls branch from 1e2d428 to 7a35601 Compare May 7, 2024 15:55

jlledom merged commit 0f621cc into 3scale:master May 7, 2024
17 of 21 checks passed

		cert: <%= ENV['REDIS_TLS_CERT'] %>
		key: <%= ENV['REDIS_TLS_KEY'] %>

THREESCALE-8404: Add TLS and ACL support for Redis #3572

THREESCALE-8404: Add TLS and ACL support for Redis #3572

Conversation

jlledom commented Sep 27, 2023 • edited Loading

Generate keys and certificates:

Configure Redis:

Configure Porta:

As TLS Client:

As Mutual TLS Cliet:

Enable the TLS Client:

jlledom commented Sep 28, 2023

jlledom commented Oct 3, 2023

github-actions bot commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlledom Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova commented Oct 26, 2023

github-actions bot commented Nov 14, 2023

akostadinov left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 6, 2023

github-actions bot commented Apr 11, 2024

akostadinov commented Apr 26, 2024

akostadinov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlledom May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlledom commented Sep 27, 2023 •

edited

Loading

jlledom Nov 20, 2023 •

edited

Loading

mayorova Oct 26, 2023 •

edited

Loading

jlledom May 2, 2024 •

edited

Loading

mayorova May 1, 2024 •

edited

Loading

jlledom May 7, 2024 •

edited

Loading