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

chore(redis) enable ssl #45

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

ADD-SP
Copy link
Contributor

@ADD-SP ADD-SP commented Mar 31, 2022

Kong CE will soon be testing the SSL connection between the plugin and Redis, and we should upgrade gojira to allow developers to run tests locally.

Also, we should ensure that both repositories use the same root certificate to issue certificates for the Redis server.

Related PR: Kong/kong#8662, Kong/kong-pongo#270

@ADD-SP ADD-SP marked this pull request as draft April 2, 2022 05:43
ADD-SP added a commit to Kong/kong that referenced this pull request Apr 7, 2022
### Summary

This commit allows the SSL connection between the `rate-limiting` plugin and `Redis` to be tested.

### Full changelog

* Add relevant testing strategies to `spec/03-plugins/23-rate-limiting/05-integration_spec.lua`.
* Update `.github/workflows/build_and_test.yml` to enable SSL for Redis in CI
* Create folder `spec/fixtures/redis` and add the following files
     * `ca.key`: Private key for the root certificate
     * `ca.crt`: Root Certificate
     * `server.key`: Private key for Redis server certificate
     * `server.crt`: Redis server certificate
     * `docker-entrypoint.sh`: To override the default

These certificates should be added to [Kong-Pongo](https://github.com/Kong/kong-pongo) and [gojira](https://github.com/Kong/gojira) to make it easy for developers to run tests locally.

Related PR: Kong/gojira#45, Kong/kong-pongo#270
@ADD-SP ADD-SP marked this pull request as ready for review April 7, 2022 03:29
@ADD-SP ADD-SP marked this pull request as draft April 7, 2022 07:24
@ADD-SP ADD-SP marked this pull request as ready for review April 11, 2022 04:37
@ADD-SP ADD-SP requested a review from mikefero April 19, 2022 02:53
Copy link
Contributor

@mikefero mikefero left a comment

Choose a reason for hiding this comment

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

First pass on this addition; this should really be an extra instead adding into the current docker-compose file and certs should be generated instead of placed into the source tree.

Please make sure you test this feature using --redis-cluster as these changes may impact the redis-cluster.sh script operation for the Docker container.

@@ -0,0 +1,28 @@
-----BEGIN CERTIFICATE-----
Copy link
Contributor

Choose a reason for hiding this comment

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

These certs should be generated and for consistency follow the same pattern that the hybrid extra uses:

  • Generate the keys if they do not exists
  • Expire them after 365 days (the current certs expires Aug 1 01:03:35 3021 GMT which is excessive for generated certs)
  • Update .gitignore to ignore the generated certs

--tls-key-file /usr/local/etc/redis/server.key
--tls-cluster no
--tls-replication no
--tls-auth-clients no
healthcheck:
test: ["CMD", "redis-cli", "ping"]
interval: 5s
Copy link
Contributor

Choose a reason for hiding this comment

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

We should isolate all these docker-compose changes into its own extra (e.g. redis-ssl) and then eggs can be applied to update the redis section of the docker-compose. This will also isolate the generation of the certs into extras/redis-ssl/keys directory.

@@ -102,7 +102,10 @@ cat << EOF
KONG_TEST_PG_HOST: ${KONG_TEST_PG_HOST:-db}
KONG_TEST_PG_DATABASE: ${KONG_TEST_PG_DATABASE:-kong_tests}
KONG_TEST_CASSANDRA_CONTACT_POINTS: ${KONG_TEST_CASSANDRA_CONTACT_POINTS:-db}
KONG_SPEC_REDIS_HOST: ${KONG_SPEC_REDIS_HOST:-redis}
Copy link
Contributor

Choose a reason for hiding this comment

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

If updates are being made to the spec/helpers.lua file in Kong Gateway to add _TEST_ for these variables we can't simply remove this one as it would break older versions of Kong Gateway.

@@ -233,13 +236,14 @@ fi
if [[ -n $GOJIRA_REDIS ]]; then
cat << EOF
redis:
image: redis:${REDIS_VERSION:-5.0.4-alpine}
image: redis:${REDIS_VERSION:-6.2.6-alpine}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the default on the 6.0 line; it can still be overridden with an env variable)

Suggested change
image: redis:${REDIS_VERSION:-6.2.6-alpine}
image: redis:${REDIS_VERSION:-6.0.16-alpine}

fegDVhkf7H/d8NOjHVBfQugXjBLWohyO+x3y0+KQY5RqHrwCSfjfUOLtN4XWoDe7
sZCUU/nqLyOhGc+FKEpz5v9Bm8eaGoR5kh+srMr5Y3OtXLXi/xd1+IE9a2gWpzpE
643zPI3Q/EFpuG6DQ1e1Qcze0VVP8Cwj0eYyvBJDad+8
-----END CERTIFICATE-----
Copy link
Contributor

Choose a reason for hiding this comment

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

The CN test-redis.example.com should be changed to something like kong_redis_ssl; this is to stay consistent with hybrid CN naming.

KONG_SPEC_TEST_REDIS_HOST: ${KONG_SPEC_TEST_REDIS_HOST:-redis}
KONG_SPEC_TEST_REDIS_PORT: ${KONG_SPEC_TEST_REDIS_PORT:-6379}
KONG_SPEC_TEST_REDIS_SSL_PORT: ${KONG_SPEC_TEST_REDIS_SSL_PORT:-6380}
KONG_SPEC_TEST_REDIS_SSL_SNI: ${KONG_SPEC_TESR_REDIS_SSL_SNI:-test-redis.example.com}
Copy link
Contributor

Choose a reason for hiding this comment

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

Environment default is misspelled and test-redis.exaple.com isn't consistent with other CN naming for this environment (see comment in server.crt).

Suggested change
KONG_SPEC_TEST_REDIS_SSL_SNI: ${KONG_SPEC_TESR_REDIS_SSL_SNI:-test-redis.example.com}
KONG_SPEC_TEST_REDIS_SSL_SNI: ${KONG_SPEC_TEST_REDIS_SSL_SNI:-kong_redis_ssl}

KONG_SPEC_TEST_REDIS_HOST: ${KONG_SPEC_TEST_REDIS_HOST:-localhost}
KONG_SPEC_TEST_REDIS_PORT: ${KONG_SPEC_TESR_REDIS_PORTT:-6379}
KONG_SPEC_TEST_REDIS_SSL_PORT: ${KONG_SPEC_TESR_REDIS_SSL_PORT:-6380}
KONG_SPEC_TEST_REDIS_SSL_SNI: ${KONG_SPEC_TESR_REDIS_SSL_SNI:-test-redis.example.com}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
KONG_SPEC_TEST_REDIS_SSL_SNI: ${KONG_SPEC_TESR_REDIS_SSL_SNI:-test-redis.example.com}
KONG_SPEC_TEST_REDIS_SSL_SNI: ${KONG_SPEC_TEST_REDIS_SSL_SNI:-kong_redis_ssl}

@@ -262,7 +266,7 @@ EOF
fi
cat << EOF
volumes:
- ${DOCKER_CTX}/redis-cluster.sh:/usr/local/bin/redis-cluster.sh
- ${DOCKER_CTX}/redis/cluster.sh:/usr/local/bin/redis-cluster.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

File was not moved, this will not work as currently implemented.

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 this pull request may close these issues.

2 participants