feat(security): Fix redis start issue from #2863 #3115
Conversation
jim-wang-intel
added
enhancement
jim-wang-intel
requested review from
hutchic,
tonyespy,
bnevis-i and
lenny-goodell
jim-wang-intel
changed the title
|
Hi @tonyespy would you please help to generate a new updated PATCH file for the related snapcraft.yml changes? Thanks! |
jim-wang-intel
force-pushed
the
fix-redis-start-insecurely-2863
branch
from
6f35d1e
to
81e9efd
Compare
cmd/security-bootstrapper/entrypoint-scripts/redis_wait_install.sh
Outdated
Show resolved
Hide resolved
cmd/security-bootstrapper/entrypoint-scripts/redis_wait_install.sh
Outdated
Show resolved
Hide resolved
cmd/security-bootstrapper/entrypoint-scripts/redis_wait_install.sh
Outdated
Show resolved
Hide resolved
cmd/security-bootstrapper/res-bootstrap-redis/configuration.toml
Outdated
Show resolved
Hide resolved
snap/snapcraft.yaml
Outdated
| # This is a simple service which calls into vault to retrieve the Redis password and then | ||
| # generate Redis config file for Redis to start up with credentials and ACL rules. | ||
| # Redis should be start once the doneFile is created. Once the config file has been generated and | ||
| # verified authenticated connection, this service exits. In the Docker version, | ||
| # the customized redis' entrypoint.sh performs the similar actions as described above. |
There was a problem hiding this comment.
Might it not be more efficient for the snap to create a wrapper script for launching redis rather than creating a separate service?
There was a problem hiding this comment.
For that matter, the snap install could write the config file and the redis sevice could just start redis directly pointing at it... on second thought... the database password is created during secretstore-setup, which I think is dynamic, so that won't work
There was a problem hiding this comment.
Right, the key or major purpose of that bootstrap-redis is to "dynamically" generate the config file as the credentials are generated from secretstore-setup during the run time for the first time. Hence, you can NOT just do the snap install.
There was a problem hiding this comment.
@tonyespy Maybe a command-chain would be useful here?
There was a problem hiding this comment.
Jim is just converting an existing oneshot service in the snap which used to call security-bootstrap-redis. The redis service is already defined to start after this service, so this transition to using bootstrap doesn't really impact the service ordering.
Yes, we could re-implement this via command-chain, however I don't see any added benefit to doing so, unless I'm missing something?
jim-wang-intel
force-pushed
the
fix-redis-start-insecurely-2863
branch
from
b9f59c6
to
7720835
Compare
|
recheck |
jim-wang-intel
force-pushed
the
fix-redis-start-insecurely-2863
branch
from
7720835
to
851e414
Compare
|
|
||
| /* Redis ACL configuration | ||
| * | ||
| * The followings are contents excerpted from redis 6.0 conf documentation: |
There was a problem hiding this comment.
It'd be nice to have a pointer to the upstream documentation as well.
| # The ACL rules that describe what a user can do are the following: | ||
| # | ||
| # on Enable the user: it is possible to authenticate as this user. | ||
| # off Disable the user: it's no longer possible to authenticate |
There was a problem hiding this comment.
It might also be nice to just include the bits of configuration that we're using (i.e. we only use "on").
| # is a glob-style pattern like the one of KEYS. | ||
| # It is possible to specify multiple patterns. | ||
| # allkeys Alias for ~* | ||
| # resetkeys Flush the list of allowed keys patterns. |
There was a problem hiding this comment.
See previous comment, the conf file we create doesn't use allcommand, nocommands, ~<pattern>, allkeys, or resetkeys.
| # "nopass" status. After "resetpass" the user has no associated | ||
| # passwords and there is no way to authenticate without adding | ||
| # some password (or setting it as "nopass" later). | ||
| # reset Performs the following actions: resetpass, resetkeys, off, |
There was a problem hiding this comment.
Same as above, we don't use reset, resetpass, nopass, or <<password>
| # | ||
| # Now DEBUG was removed when alice had yet no commands in the set of allowed | ||
| # commands, later all the commands are added, so the user will be able to | ||
| # execute everything. |
There was a problem hiding this comment.
Same as above, maybe just explain the doc which applies to our conf (e.g. +@ALL -@dangerous)? Maybe even include our conf scheme and explain it?
user default on allkeys +@all -@dangerous > password
requirepass password
snap/snapcraft.yaml
Outdated
| # This is a simple service which calls into vault to retrieve the Redis password and then | ||
| # generate Redis config file for Redis to start up with credentials and ACL rules. | ||
| # Redis should be start once the doneFile is created. Once the config file has been generated and | ||
| # verified authenticated connection, this service exits. In the Docker version, | ||
| # the customized redis' entrypoint.sh performs the similar actions as described above. |
There was a problem hiding this comment.
Jim is just converting an existing oneshot service in the snap which used to call security-bootstrap-redis. The redis service is already defined to start after this service, so this transition to using bootstrap doesn't really impact the service ordering.
Yes, we could re-implement this via command-chain, however I don't see any added benefit to doing so, unless I'm missing something?
Now redis starts with conf file with credentials and thus insecure gap is removed - Refactor security-bootstrap-redis to absorbed into security-bootstrapper as one of command - Remove security-bootstrap-redis binary build - Redis db server starts with config file with credentials - Update snaps Closes: edgexfoundry#2863 Signed-off-by: Jim Wang <yutsung.jim.wang@intel.com>
Address Bryon's PR feedback Signed-off-by: Jim Wang <yutsung.jim.wang@intel.com>
Address Lenny's PR feedback about unit tests Signed-off-by: Jim Wang <yutsung.jim.wang@intel.com>
Change the redis.conf file permission to 0600 Make chown for redis' conf to redis:redis 999:1000 as part of redis' uid and gid creation Add doc's url and detailed explanation for redis' conf EdgeX is currently using Signed-off-by: Jim Wang <yutsung.jim.wang@intel.com>
There was a problem hiding this comment.
Wrt the implementation, this looks good. Kudos for switching over to ACLs. One of the conversations I had with the V2 API team was to consider that with ACLs and deterministic key names, we could segregate microservice access to only the keys they should have access to. If that does evolve, this will need a little rework. Another thing to note is that by removing the dangerous commands, the commands useful for debugging Redis apps have also been removed. I had tried to document tips to debug with security enabled in the various security .md files and I suggest pointing out enabling the dangerous commands while debugging may be useful.
Circling back to #2863, while using a config file conforms to the ADR and addresses the risk of Redis being unsecured on boot, I wonder if we're trading a timing risk for a clear password on disk risk. Another way #2863 could be addressed is starting Redis unsecured and pointing to an empty RDB file, dynamically setting the password/ACLs, then pointing back to the actual RDB file.
Thanks @andresrinivasan. A good thought about the debugging redis app. It is truly that you might lose the ability to run the dangerous command like
From file system mounting point of view, as long as you can mount it on the temp. file system like docker's tmfs, in which it also runs in memory (see docker's doc https://docs.docker.com/storage/tmpfs/) , I think the password written out should be ok. At least it is not as bad as of it today and just like Redis' data in memory. |
What is to stop someone, if they can get at the Redis password, from simply starting their own server to read the RDB files directly bypassing the provided Redis? The whole issue could be avoided if Redis simply persisted the ACL configuration. |
In addition to INFO, MONITOR, BGSAVE, and FLUSHDB are quite useful. I'm only suggesting we document this and document specifically how the developer overrides this. When I was trying to figure out how to integrate with Vault I consumed a lot of time from Bryon and Lenny because not enough was written down to help me figure out how to debug things. |
In the grand scheme of things, there's not much difference between a oneshot service, a wrapper script or a command chain. In this case, Jim just converted an existing one-shot service which already works "as is". Yes, he could certainly re-implement this as a redis wrapper script or command-chain executable, but as there's no real advantage to the other two approaches, I'm not sure I see your point? |
Ok. |
Remove the change ownership from golang code and only do the change ownership inside the docker's entrypoint script because snap doesn't work with chow to a non-existing userId. Signed-off-by: Jim Wang <yutsung.jim.wang@intel.com>
jim-wang-intel
dismissed stale reviews from lenny-goodell and bnevis-i
via
45ee40c
jim-wang-intel
force-pushed
the
fix-redis-start-insecurely-2863
branch
from
24b1dce
to
45ee40c
Compare
@andresrinivasan I created a note md file for the aspect of developers to use their own configuration file: Developer-notes.md. |
|
@jim-wang-intel here's the update for the snap patch... |
patch -p1 < ./0001-fix-snap-update-the-snap-optimization-patch.patch.txt patching file snap/local/patches/0001-optimize-build-for-pipeline-CI-check.patch Signed-off-by: Jim Wang <yutsung.jim.wang@intel.com>
jim-wang-intel
dismissed stale reviews from lenny-goodell and tonyespy
via
362659e
|
Kudos, SonarCloud Quality Gate passed!
|
Now redis starts with conf file with credentials and thus insecure gap is removed
Closes: #2863
Signed-off-by: Jim Wang yutsung.jim.wang@intel.com
PR Checklist
Please check if your PR fulfills the following requirements:
If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-go/blob/master/.github/Contributing.md.
What is the current behavior?
Redis server starts insecurely as it is not started in authenticated mode.
Issue Number: #2863
What is the new behavior?
Now Redis server starts with credentials in conf file and thus it is started securely.
Does this PR introduce a breaking change?
New Imports
Specific Instructions
Are there any specific instructions or things that should be known prior to reviewing?
Local docker test file:
Other information
TBD: need to update patch file for SNAP @tonyespy