Just a small heads-up: this will not go into v15 anymore, but rather v15.1, because I will start a release for v15 soon.

I'll review when DMS v15 is released. Presently trying to tackle various docs PRs within that window until then.

I am not sure if I understand you correctly, but using export makes a variable available to child processes. check-for-changes.sh is started by supervisord. So it's not a child of the start script.

check-for-changes.sh is started by supervisord. So it's not a child of the start script.

Ok, but the source /etc/dms-settings will be run after the start script has finished updating that file yes?

Any ENV not stored in that file will not be usable by check-for-changes.sh or elsewhere after __FILE is processed. Those would only be used via the startup script, so while export may not be necessary, if they're not part of the VARS associative map then they'll not be persisted.

Just highlighting this as a potential caveat where subtle bugs may later appear.

Anything within the container that can read /etc/dms-settings (0644 permissions) will have access to the file-based secret values centralized there.

This feature AFAIK is only to prevent secrets visibility from anyone with access to the Docker socket by sending a GET HTTP request, similar to how the Docker CLI works:

# Read the ENV associated to the running container:
docker inspect <container-name> | jq -r .[].Config.Env

# Shell into DMS as with an interactive TTY as root (implicitly starts a login shell `-l`):
# NOTE: Secrets would be visible in ENV loaded from `/etc/dms-settings` via `/root/.bashrc`
docker exec -it <container-name> bash

I think this feature makes more sense when the secret is read temporarily into memory to use it, but due to how DMS is managing the secrets currently, this is to effectively only defend against a separately compromised container inspecting metadata through the Docker socket.

I just want to clarify with the author (and any others interested in the feature) if that meets their actual expectations of implementing this feature, or if it falls short of the expected security benefit.

If @aartoni still wants to proceed with that security benefit clarified, then they just need to ensure the PR has a test-case that verifies the target ENV has the expected value resolved by the __FILE variant.

@polarathene I'm willing to proceed. I'll get in touch in a few days in case I get stuck.

Hello @polarathene! I got stuck while trying to address this comment.

It is not clear to me how to obtain the path for the copy of the configuration that I should be using in this case, here's what I have tried:

TEST_TMP_CONFIG=$(_duplicate_config_for_container . 'env_vars_from_files')

We have a variety of tests you can reference that do this :)

TEST_TMP_CONFIG should be already managed for you (duplicated for each container configured in setup_file()), so like the examples below, you can use mv or cp to place files where you need 👍

If you need to create files instead of cp/mv, you should be able to create files at the TEST_TMP_CONFIG location too. Just make sure you do this after calling _init_with_defaults first (configures TEST_TMP_CONFIG for the test) and before calling _common_container_setup (starts the container with TEST_TMP_CONFIG volume mounted).

Extra notes

• If you need to have files provided already, you can commit them to this directory which is the contents of TEST_TMP_CONFIG (duplicated per container setup).

• _init_with_defaults() duplicates the test config directory for you setting it to TEST_TMP_CONFIG:

  docker-mailserver/test/helper/setup.bash

  Lines 96 to 97 in ef66dd5

  	export TEST_TMP_CONFIG
  	TEST_TMP_CONFIG=$(_duplicate_config_for_container . "${CONTAINER_NAME}")

  TEST_TMP_CONFIG is then added as a volume mount for the container to use as /tmp/docker-mailserver (DMS Config Volume):

  docker-mailserver/test/helper/setup.bash

  Line 108 in ef66dd5

  export TEST_CONFIG_VOLUME="${TEST_TMP_CONFIG}:/tmp/docker-mailserver"

  docker-mailserver/test/helper/setup.bash

  Line 158 in ef66dd5

  --volume "${TEST_CONFIG_VOLUME}" \

• We also document here how you can create a separate TEST_TMP_CONFIG on demand when needed (see the example portion):

  docker-mailserver/test/helper/setup.bash

  Lines 77 to 92 in ef66dd5

  	# Common defaults appropriate for most tests.
  	#
  	# Override variables in test cases within a file when necessary:
  	# - Use `export <VARIABLE>` in `setup_file()` to overrides for all test cases.
  	# - Use `local <VARIABLE>` to override within a specific test case.
  	#
  	# ## Attenton
  	#
  	# The ENV `CONTAINER_NAME` must be set before this method is called. It only affects the
  	# `TEST_TMP_CONFIG` directory created, but will be used in `common_container_create()`
  	# and implicitly in other helper methods.
  	#
  	# ## Example
  	#
  	# For example, if you need an immutable config volume that can't be affected by other tests
  	# in the file, then use `local TEST_TMP_CONFIG=$(_duplicate_config_for_container . "${UNIQUE_ID_HERE}")`

• Automated removal happens here:

  docker-mailserver/Makefile

  Lines 27 to 33 in ef66dd5

  	# `docker ps`: Remove any lingering test containers
  	# `.gitignore`: Remove `test/duplicate_configs` and files copied via `make generate-accounts`
  	clean: ALWAYS_RUN
  	-@ while read -r LINE; do CONTAINERS+=("$${LINE}"); done < <(docker ps -qaf name='^(dms-test|mail)_.*') ; \
  	for CONTAINER in "$${CONTAINERS[@]}"; do docker rm -f "$${CONTAINER}"; done
  	-@ while read -r LINE; do [[ $${LINE} =~ test/.+ ]] && FILES+=("/mnt$${LINE#test}"); done < .gitignore ; \
  	docker run --rm -v "$(REPOSITORY_ROOT)/test/:/mnt" alpine ash -c "rm -rf $${FILES[@]}"

  Which will remove all the duplicated config folders tests created at test/duplicate_configs/:

  docker-mailserver/.gitignore

  Line 21 in ef66dd5

  test/duplicate_configs/

Thank you @polarathene for referencing the relevant code. I have implemented the tests the way you pointed out, I'd say that this is ready for the final review!

I'm sorry @polarathene, I have unwillingly dismissed your review, can you please contribute back the comments so that I can approve them? Or simply push without my help if you prefer :)

I'll review this on the weekend.

Documentation preview for this PR is ready! 🎉

Built with commit: 9ef06e6