mirrors: allow username/password as environment variables (#46549)

`spack mirror add` and `set` now have flags `--oci-password-variable`, `--oci-password-variable`, `--s3-access-key-id-variable`, `--s3-access-key-secret-variable`, `--s3-access-token-variable`, which allows users to specify an environment variable in which a username or password is stored.

Storing plain text passwords in config files is considered deprecated.

The schema for mirrors.yaml has changed, notably the `access_pair` list is generally replaced with a dictionary of `{id: ..., secret_variable: ...}` or `{id_variable: ..., secret_variable: ...}`.

9 files changed, 520 insertions, 57 deletions

diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index 2f74f38dac..dcc82130e7 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -1182,6 +1182,9 @@ class Uploader:
         self.tmpdir: str
         self.executor: concurrent.futures.Executor
 
+        # Verify if the mirror meets the requirements to push
+        self.mirror.ensure_mirror_usable("push")
+
     def __enter__(self):
         self._tmpdir = tempfile.TemporaryDirectory(dir=spack.stage.get_stage_root())
         self._executor = spack.util.parallel.make_concurrent_executor()
diff --git a/lib/spack/spack/cmd/common/arguments.py b/lib/spack/spack/cmd/common/arguments.py
index 6a4a43e9e9..7ddafd4d14 100644
--- a/lib/spack/spack/cmd/common/arguments.py
+++ b/lib/spack/spack/cmd/common/arguments.py
@@ -581,23 +581,51 @@ def add_concretizer_args(subparser):
 
 
 def add_connection_args(subparser, add_help):
-    subparser.add_argument(
-        "--s3-access-key-id", help="ID string to use to connect to this S3 mirror"
+    def add_argument_string_or_variable(parser, arg: str, *, deprecate_str: bool = True, **kwargs):
+        group = parser.add_mutually_exclusive_group()
+        group.add_argument(arg, **kwargs)
+        # Update help string
+        if "help" in kwargs:
+            kwargs["help"] = "environment variable containing " + kwargs["help"]
+        group.add_argument(arg + "-variable", **kwargs)
+
+    s3_connection_parser = subparser.add_argument_group("S3 Connection")
+
+    add_argument_string_or_variable(
+        s3_connection_parser,
+        "--s3-access-key-id",
+        help="ID string to use to connect to this S3 mirror",
     )
-    subparser.add_argument(
-        "--s3-access-key-secret", help="secret string to use to connect to this S3 mirror"
+    add_argument_string_or_variable(
+        s3_connection_parser,
+        "--s3-access-key-secret",
+        help="secret string to use to connect to this S3 mirror",
     )
-    subparser.add_argument(
-        "--s3-access-token", help="access token to use to connect to this S3 mirror"
+    add_argument_string_or_variable(
+        s3_connection_parser,
+        "--s3-access-token",
+        help="access token to use to connect to this S3 mirror",
     )
-    subparser.add_argument(
+    s3_connection_parser.add_argument(
         "--s3-profile", help="S3 profile name to use to connect to this S3 mirror", default=None
     )
-    subparser.add_argument(
+    s3_connection_parser.add_argument(
         "--s3-endpoint-url", help="endpoint URL to use to connect to this S3 mirror"
     )
-    subparser.add_argument("--oci-username", help="username to use to connect to this OCI mirror")
-    subparser.add_argument("--oci-password", help="password to use to connect to this OCI mirror")
+
+    oci_connection_parser = subparser.add_argument_group("OCI Connection")
+
+    add_argument_string_or_variable(
+        oci_connection_parser,
+        "--oci-username",
+        deprecate_str=False,
+        help="username to use to connect to this OCI mirror",
+    )
+    add_argument_string_or_variable(
+        oci_connection_parser,
+        "--oci-password",
+        help="password to use to connect to this OCI mirror",
+    )
 
 
 def use_buildcache(cli_arg_value):
diff --git a/lib/spack/spack/cmd/mirror.py b/lib/spack/spack/cmd/mirror.py
index af6a45e399..ede0429497 100644
--- a/lib/spack/spack/cmd/mirror.py
+++ b/lib/spack/spack/cmd/mirror.py
@@ -231,31 +231,133 @@ def setup_parser(subparser):
     )
 
 
+def _configure_access_pair(
+    args, id_tok, id_variable_tok, secret_tok, secret_variable_tok, default=None
+):
+    """Configure the access_pair options"""
+
+    # Check if any of the arguments are set to update this access_pair.
+    # If none are set, then skip computing the new access pair
+    args_id = getattr(args, id_tok)
+    args_id_variable = getattr(args, id_variable_tok)
+    args_secret = getattr(args, secret_tok)
+    args_secret_variable = getattr(args, secret_variable_tok)
+    if not any([args_id, args_id_variable, args_secret, args_secret_variable]):
+        return None
+
+    def _default_value(id_):
+        if isinstance(default, list):
+            return default[0] if id_ == "id" else default[1]
+        elif isinstance(default, dict):
+            return default.get(id_)
+        else:
+            return None
+
+    def _default_variable(id_):
+        if isinstance(default, dict):
+            return default.get(id_ + "_variable")
+        else:
+            return None
+
+    id_ = None
+    id_variable = None
+    secret = None
+    secret_variable = None
+
+    # Get the value/default value if the argument of the inverse
+    if not args_id_variable:
+        id_ = getattr(args, id_tok) or _default_value("id")
+    if not args_id:
+        id_variable = getattr(args, id_variable_tok) or _default_variable("id")
+    if not args_secret_variable:
+        secret = getattr(args, secret_tok) or _default_value("secret")
+    if not args_secret:
+        secret_variable = getattr(args, secret_variable_tok) or _default_variable("secret")
+
+    if (id_ or id_variable) and (secret or secret_variable):
+        if secret:
+            if not id_:
+                raise SpackError("Cannot add mirror with a variable id and text secret")
+
+            return [id_, secret]
+        else:
+            return dict(
+                [
+                    (("id", id_) if id_ else ("id_variable", id_variable)),
+                    ("secret_variable", secret_variable),
+                ]
+            )
+    else:
+        if id_ or id_variable or secret or secret_variable is not None:
+            id_arg_tok = id_tok.replace("_", "-")
+            secret_arg_tok = secret_tok.replace("_", "-")
+            tty.warn(
+                "Expected both parts of the access pair to be specified. "
+                f"(i.e. --{id_arg_tok} and --{secret_arg_tok})"
+            )
+
+        return None
+
+
 def mirror_add(args):
     """add a mirror to Spack"""
     if (
         args.s3_access_key_id
         or args.s3_access_key_secret
         or args.s3_access_token
+        or args.s3_access_key_id_variable
+        or args.s3_access_key_secret_variable
+        or args.s3_access_token_variable
         or args.s3_profile
         or args.s3_endpoint_url
         or args.type
         or args.oci_username
         or args.oci_password
+        or args.oci_username_variable
+        or args.oci_password_variable
         or args.autopush
         or args.signed is not None
     ):
         connection = {"url": args.url}
-        if args.s3_access_key_id and args.s3_access_key_secret:
-            connection["access_pair"] = [args.s3_access_key_id, args.s3_access_key_secret]
+        # S3 Connection
+        if args.s3_access_key_secret:
+            tty.warn(
+                "Configuring mirror secrets as plain text with --s3-access-key-secret is "
+                "deprecated. Use --s3-access-key-secret-variable instead"
+            )
+        if args.oci_password:
+            tty.warn(
+                "Configuring mirror secrets as plain text with --oci-password is deprecated. "
+                "Use --oci-password-variable instead"
+            )
+        access_pair = _configure_access_pair(
+            args,
+            "s3_access_key_id",
+            "s3_access_key_id_variable",
+            "s3_access_key_secret",
+            "s3_access_key_secret_variable",
+        )
+        if access_pair:
+            connection["access_pair"] = access_pair
+
         if args.s3_access_token:
             connection["access_token"] = args.s3_access_token
+        elif args.s3_access_token_variable:
+            connection["access_token_variable"] = args.s3_access_token_variable
+
         if args.s3_profile:
             connection["profile"] = args.s3_profile
+
         if args.s3_endpoint_url:
             connection["endpoint_url"] = args.s3_endpoint_url
-        if args.oci_username and args.oci_password:
-            connection["access_pair"] = [args.oci_username, args.oci_password]
+
+        # OCI Connection
+        access_pair = _configure_access_pair(
+            args, "oci_username", "oci_username_variable", "oci_password", "oci_password_variable"
+        )
+        if access_pair:
+            connection["access_pair"] = access_pair
+
         if args.type:
             connection["binary"] = "binary" in args.type
             connection["source"] = "source" in args.type
@@ -285,16 +387,35 @@ def _configure_mirror(args):
     changes = {}
     if args.url:
         changes["url"] = args.url
-    if args.s3_access_key_id and args.s3_access_key_secret:
-        changes["access_pair"] = [args.s3_access_key_id, args.s3_access_key_secret]
+
+    default_access_pair = entry._get_value("access_pair", direction or "fetch")
+    # TODO: Init access_pair args with the fetch/push/base values in the current mirror state
+    access_pair = _configure_access_pair(
+        args,
+        "s3_access_key_id",
+        "s3_access_key_id_variable",
+        "s3_access_key_secret",
+        "s3_access_key_secret_variable",
+        default=default_access_pair,
+    )
+    if access_pair:
+        changes["access_pair"] = access_pair
     if args.s3_access_token:
         changes["access_token"] = args.s3_access_token
     if args.s3_profile:
         changes["profile"] = args.s3_profile
     if args.s3_endpoint_url:
         changes["endpoint_url"] = args.s3_endpoint_url
-    if args.oci_username and args.oci_password:
-        changes["access_pair"] = [args.oci_username, args.oci_password]
+    access_pair = _configure_access_pair(
+        args,
+        "oci_username",
+        "oci_username_variable",
+        "oci_password",
+        "oci_password_variable",
+        default=default_access_pair,
+    )
+    if access_pair:
+        changes["access_pair"] = access_pair
     if getattr(args, "signed", None) is not None:
         changes["signed"] = args.signed
     if getattr(args, "autopush", None) is not None:
diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py
index b320671361..328a456fc3 100644
--- a/lib/spack/spack/mirror.py
+++ b/lib/spack/spack/mirror.py
@@ -18,7 +18,7 @@ import os.path
 import sys
 import traceback
 import urllib.parse
-from typing import List, Optional, Union
+from typing import Any, Dict, Optional, Tuple, Union
 
 import llnl.url
 import llnl.util.symlink
@@ -153,8 +153,66 @@ class Mirror:
         """Get the valid, canonicalized fetch URL"""
         return self.get_url("push")
 
+    def ensure_mirror_usable(self, direction: str = "push"):
+        access_pair = self._get_value("access_pair", direction)
+        access_token_variable = self._get_value("access_token_variable", direction)
+
+        errors = []
+
+        # Verify that the credentials that are variables expand
+        if access_pair and isinstance(access_pair, dict):
+            if "id_variable" in access_pair and access_pair["id_variable"] not in os.environ:
+                errors.append(f"id_variable {access_pair['id_variable']} not set in environment")
+            if "secret_variable" in access_pair:
+                if access_pair["secret_variable"] not in os.environ:
+                    errors.append(
+                        f"environment variable `{access_pair['secret_variable']}` "
+                        "(secret_variable) not set"
+                    )
+
+        if access_token_variable:
+            if access_token_variable not in os.environ:
+                errors.append(
+                    f"environment variable `{access_pair['access_token_variable']}` "
+                    "(access_token_variable) not set"
+                )
+
+        if errors:
+            msg = f"invalid {direction} configuration for mirror {self.name}: "
+            msg += "\n    ".join(errors)
+            raise spack.mirror.MirrorError(msg)
+
     def _update_connection_dict(self, current_data: dict, new_data: dict, top_level: bool):
-        keys = ["url", "access_pair", "access_token", "profile", "endpoint_url"]
+        # Only allow one to exist in the config
+        if "access_token" in current_data and "access_token_variable" in new_data:
+            current_data.pop("access_token")
+        elif "access_token_variable" in current_data and "access_token" in new_data:
+            current_data.pop("access_token_variable")
+
+        # If updating to a new access_pair that is the deprecated list, warn
+        warn_deprecated_access_pair = False
+        if "access_pair" in new_data:
+            warn_deprecated_access_pair = isinstance(new_data["access_pair"], list)
+        # If the not updating the current access_pair, and it is the deprecated list, warn
+        elif "access_pair" in current_data:
+            warn_deprecated_access_pair = isinstance(current_data["access_pair"], list)
+
+        if warn_deprecated_access_pair:
+            tty.warn(
+                f"in mirror {self.name}: support for plain text secrets in config files "
+                "(access_pair: [id, secret]) is deprecated and will be removed in a future Spack "
+                "version. Use environment variables instead (access_pair: "
+                "{id: ..., secret_variable: ...})"
+            )
+
+        keys = [
+            "url",
+            "access_pair",
+            "access_token",
+            "access_token_variable",
+            "profile",
+            "endpoint_url",
+        ]
         if top_level:
             keys += ["binary", "source", "signed", "autopush"]
         changed = False
@@ -270,11 +328,53 @@ class Mirror:
 
         return _url_or_path_to_url(url)
 
-    def get_access_token(self, direction: str) -> Optional[str]:
-        return self._get_value("access_token", direction)
+    def get_credentials(self, direction: str) -> Dict[str, Any]:
+        """Get the mirror credentials from the mirror config
+
+        Args:
+            direction: fetch or push mirror config
+
+        Returns:
+            Dictionary from credential type string to value
 
-    def get_access_pair(self, direction: str) -> Optional[List]:
-        return self._get_value("access_pair", direction)
+            Credential Type Map:
+                access_token -> str
+                access_pair  -> tuple(str,str)
+                profile      -> str
+        """
+        creddict: Dict[str, Any] = {}
+        access_token = self.get_access_token(direction)
+        if access_token:
+            creddict["access_token"] = access_token
+
+        access_pair = self.get_access_pair(direction)
+        if access_pair:
+            creddict.update({"access_pair": access_pair})
+
+        profile = self.get_profile(direction)
+        if profile:
+            creddict["profile"] = profile
+
+        return creddict
+
+    def get_access_token(self, direction: str) -> Optional[str]:
+        tok = self._get_value("access_token_variable", direction)
+        if tok:
+            return os.environ.get(tok)
+        else:
+            return self._get_value("access_token", direction)
+        return None
+
+    def get_access_pair(self, direction: str) -> Optional[Tuple[str, str]]:
+        pair = self._get_value("access_pair", direction)
+        if isinstance(pair, (tuple, list)) and len(pair) == 2:
+            return (pair[0], pair[1]) if all(pair) else None
+        elif isinstance(pair, dict):
+            id_ = os.environ.get(pair["id_variable"]) if "id_variable" in pair else pair["id"]
+            secret = os.environ.get(pair["secret_variable"])
+            return (id_, secret) if id_ and secret else None
+        else:
+            return None
 
     def get_profile(self, direction: str) -> Optional[str]:
         return self._get_value("profile", direction)
diff --git a/lib/spack/spack/oci/opener.py b/lib/spack/spack/oci/opener.py
index 906d5d2b92..2f9e83f5be 100644
--- a/lib/spack/spack/oci/opener.py
+++ b/lib/spack/spack/oci/opener.py
@@ -377,9 +377,10 @@ def credentials_from_mirrors(
         # Prefer push credentials over fetch. Unlikely that those are different
         # but our config format allows it.
         for direction in ("push", "fetch"):
-            pair = mirror.get_access_pair(direction)
-            if pair is None:
+            pair = mirror.get_credentials(direction).get("access_pair")
+            if not pair:
                 continue
+
             url = mirror.get_url(direction)
             if not url.startswith("oci://"):
                 continue
diff --git a/lib/spack/spack/schema/mirrors.py b/lib/spack/spack/schema/mirrors.py
index 9a56f1a7e2..8a61fd5b53 100644
--- a/lib/spack/spack/schema/mirrors.py
+++ b/lib/spack/spack/schema/mirrors.py
@@ -15,14 +15,42 @@ connection = {
     "url": {"type": "string"},
     # todo: replace this with named keys "username" / "password" or "id" / "secret"
     "access_pair": {
-        "type": "array",
-        "items": {"type": ["string", "null"], "minItems": 2, "maxItems": 2},
+        "oneOf": [
+            {
+                "type": "array",
+                "items": {"minItems": 2, "maxItems": 2, "type": ["string", "null"]},
+            },  # deprecated
+            {
+                "type": "object",
+                "required": ["secret_variable"],
+                # Only allow id or id_variable to be set, not both
+                "oneOf": [{"required": ["id"]}, {"required": ["id_variable"]}],
+                "properties": {
+                    "id": {"type": "string"},
+                    "id_variable": {"type": "string"},
+                    "secret_variable": {"type": "string"},
+                },
+            },
+        ]
     },
-    "access_token": {"type": ["string", "null"]},
     "profile": {"type": ["string", "null"]},
     "endpoint_url": {"type": ["string", "null"]},
+    "access_token": {"type": ["string", "null"]},  # deprecated
+    "access_token_variable": {"type": ["string", "null"]},
 }
 
+connection_ext = {
+    "deprecatedProperties": [
+        {
+            "names": ["access_token"],
+            "message": "Use of plain text `access_token` in mirror config is deprecated, use "
+            "environment variables instead (access_token_variable)",
+            "error": False,
+        }
+    ]
+}
+
+
 #: Mirror connection inside pull/push keys
 fetch_and_push = {
     "anyOf": [
@@ -31,6 +59,7 @@ fetch_and_push = {
             "type": "object",
             "additionalProperties": False,
             "properties": {**connection},  # type: ignore
+            **connection_ext,  # type: ignore
         },
     ]
 }
@@ -49,6 +78,7 @@ mirror_entry = {
         "autopush": {"type": "boolean"},
         **connection,  # type: ignore
     },
+    **connection_ext,  # type: ignore
 }
 
 #: Properties for inclusion in other schemas
@@ -70,3 +100,28 @@ schema = {
     "additionalProperties": False,
     "properties": properties,
 }
+
+
+def update(data):
+    import jsonschema
+
+    errors = []
+
+    def check_access_pair(name, section):
+        if not section or not isinstance(section, dict):
+            return
+
+        if "access_token" in section and "access_token_variable" in section:
+            errors.append(
+                f'{name}: mirror credential "access_token" conflicts with "access_token_variable"'
+            )
+
+    # Check all of the sections
+    for name, section in data.items():
+        check_access_pair(name, section)
+        if isinstance(section, dict):
+            check_access_pair(name, section.get("fetch"))
+            check_access_pair(name, section.get("push"))
+
+    if errors:
+        raise jsonschema.ValidationError("\n".join(errors))
diff --git a/lib/spack/spack/test/cmd/mirror.py b/lib/spack/spack/test/cmd/mirror.py
index 2a67bc2e14..ee827c0554 100644
--- a/lib/spack/spack/test/cmd/mirror.py
+++ b/lib/spack/spack/test/cmd/mirror.py
@@ -17,6 +17,7 @@ import spack.util.url as url_util
 import spack.version
 from spack.main import SpackCommand, SpackCommandError
 
+config = SpackCommand("config")
 mirror = SpackCommand("mirror")
 env = SpackCommand("env")
 add = SpackCommand("add")
@@ -181,20 +182,122 @@ def test_mirror_crud(mutable_config, capsys):
         output = mirror("remove", "mirror")
         assert "Removed mirror" in output
 
-        # Test S3 connection info id/key
-        mirror(
-            "add",
-            "--s3-access-key-id",
-            "foo",
-            "--s3-access-key-secret",
-            "bar",
-            "mirror",
-            "s3://spack-public",
-        )
+        # Test S3 connection info token as variable
+        mirror("add", "--s3-access-token-variable", "aaaaaazzzzz", "mirror", "s3://spack-public")
 
         output = mirror("remove", "mirror")
         assert "Removed mirror" in output
 
+        def do_add_set_seturl_access_pair(
+            id_arg, secret_arg, mirror_name="mirror", mirror_url="s3://spack-public"
+        ):
+            # Test S3 connection info id/key
+            output = mirror("add", id_arg, "foo", secret_arg, "bar", mirror_name, mirror_url)
+            if "variable" not in secret_arg:
+                assert (
+                    f"Configuring mirror secrets as plain text with {secret_arg} is deprecated. "
+                    in output
+                )
+
+            output = config("blame", "mirrors")
+            assert all([x in output for x in ("foo", "bar", mirror_name, mirror_url)])
+            # Mirror access_pair deprecation warning should not be in blame output
+            assert "support for plain text secrets" not in output
+
+            output = mirror("set", id_arg, "foo_set", secret_arg, "bar_set", mirror_name)
+            if "variable" not in secret_arg:
+                assert "support for plain text secrets" in output
+            output = config("blame", "mirrors")
+            assert all([x in output for x in ("foo_set", "bar_set", mirror_name, mirror_url)])
+            if "variable" not in secret_arg:
+                output = mirror(
+                    "set", id_arg, "foo_set", secret_arg + "-variable", "bar_set_var", mirror_name
+                )
+                assert "support for plain text secrets" not in output
+                output = config("blame", "mirrors")
+                assert all(
+                    [x in output for x in ("foo_set", "bar_set_var", mirror_name, mirror_url)]
+                )
+
+            output = mirror(
+                "set-url",
+                id_arg,
+                "foo_set_url",
+                secret_arg,
+                "bar_set_url",
+                "--push",
+                mirror_name,
+                mirror_url + "-push",
+            )
+            output = config("blame", "mirrors")
+            assert all(
+                [
+                    x in output
+                    for x in ("foo_set_url", "bar_set_url", mirror_name, mirror_url + "-push")
+                ]
+            )
+
+            output = mirror("set", id_arg, "a", mirror_name)
+            assert "No changes made to mirror" not in output
+
+            output = mirror("set", secret_arg, "b", mirror_name)
+            assert "No changes made to mirror" not in output
+
+            output = mirror("set-url", id_arg, "c", mirror_name, mirror_url)
+            assert "No changes made to mirror" not in output
+
+            output = mirror("set-url", secret_arg, "d", mirror_name, mirror_url)
+            assert "No changes made to mirror" not in output
+
+            output = mirror("remove", mirror_name)
+            assert "Removed mirror" in output
+
+            output = mirror("add", id_arg, "foo", mirror_name, mirror_url)
+            assert "Expected both parts of the access pair to be specified. " in output
+
+            output = mirror("set-url", id_arg, "bar", mirror_name, mirror_url)
+            assert "Expected both parts of the access pair to be specified. " in output
+
+            output = mirror("set", id_arg, "bar", mirror_name)
+            assert "Expected both parts of the access pair to be specified. " in output
+
+            output = mirror("remove", mirror_name)
+            assert "Removed mirror" in output
+
+            output = mirror("add", secret_arg, "bar", mirror_name, mirror_url)
+            assert "Expected both parts of the access pair to be specified. " in output
+
+            output = mirror("set-url", secret_arg, "bar", mirror_name, mirror_url)
+            assert "Expected both parts of the access pair to be specified. " in output
+
+            output = mirror("set", secret_arg, "bar", mirror_name)
+            assert "Expected both parts of the access pair to be specified. " in output
+
+            output = mirror("remove", mirror_name)
+            assert "Removed mirror" in output
+
+            output = mirror("list")
+            assert "No mirrors configured" in output
+
+        do_add_set_seturl_access_pair("--s3-access-key-id", "--s3-access-key-secret")
+        do_add_set_seturl_access_pair("--s3-access-key-id", "--s3-access-key-secret-variable")
+        do_add_set_seturl_access_pair(
+            "--s3-access-key-id-variable", "--s3-access-key-secret-variable"
+        )
+        with pytest.raises(
+            spack.error.SpackError, match="Cannot add mirror with a variable id and text secret"
+        ):
+            do_add_set_seturl_access_pair("--s3-access-key-id-variable", "--s3-access-key-secret")
+
+        # Test OCI connection info user/password
+        do_add_set_seturl_access_pair("--oci-username", "--oci-password")
+        do_add_set_seturl_access_pair("--oci-username", "--oci-password-variable")
+        do_add_set_seturl_access_pair("--oci-username-variable", "--oci-password-variable")
+        with pytest.raises(
+            spack.error.SpackError, match="Cannot add mirror with a variable id and text secret"
+        ):
+            do_add_set_seturl_access_pair("--s3-access-key-id-variable", "--s3-access-key-secret")
+
         # Test S3 connection info with endpoint URL
         mirror(
             "add",
@@ -218,6 +321,9 @@ def test_mirror_crud(mutable_config, capsys):
         output = mirror("remove", "mirror")
         assert "Removed mirror" in output
 
+        output = mirror("list")
+        assert "No mirrors configured" in output
+
 
 def test_mirror_nonexisting(mutable_config):
     with pytest.raises(SpackCommandError):
diff --git a/lib/spack/spack/test/mirror.py b/lib/spack/spack/test/mirror.py
index b62d5a3e41..ce104259fc 100644
--- a/lib/spack/spack/test/mirror.py
+++ b/lib/spack/spack/test/mirror.py
@@ -329,9 +329,9 @@ def test_update_4():
 
 
 @pytest.mark.parametrize("direction", ["fetch", "push"])
-def test_update_connection_params(direction):
+def test_update_connection_params(direction, tmpdir, monkeypatch):
     """Test whether new connection params expand the mirror config to a dict."""
-    m = spack.mirror.Mirror("https://example.com")
+    m = spack.mirror.Mirror("https://example.com", "example")
 
     assert m.update(
         {
@@ -354,12 +354,64 @@ def test_update_connection_params(direction):
             "endpoint_url": "https://example.com",
         },
     }
-
-    assert m.get_access_pair(direction) == ["username", "password"]
+    assert m.get_access_pair(direction) == ("username", "password")
     assert m.get_access_token(direction) == "token"
     assert m.get_profile(direction) == "profile"
     assert m.get_endpoint_url(direction) == "https://example.com"
 
+    # Expand environment variables
+    os.environ["_SPACK_TEST_PAIR_USERNAME"] = "expanded_username"
+    os.environ["_SPACK_TEST_PAIR_PASSWORD"] = "expanded_password"
+    os.environ["_SPACK_TEST_TOKEN"] = "expanded_token"
+
+    assert m.update(
+        {
+            "access_pair": {
+                "id_variable": "_SPACK_TEST_PAIR_USERNAME",
+                "secret_variable": "_SPACK_TEST_PAIR_PASSWORD",
+            }
+        },
+        direction,
+    )
+
+    assert m.to_dict() == {
+        "url": "https://example.com",
+        direction: {
+            "url": "http://example.org",
+            "access_pair": {
+                "id_variable": "_SPACK_TEST_PAIR_USERNAME",
+                "secret_variable": "_SPACK_TEST_PAIR_PASSWORD",
+            },
+            "access_token": "token",
+            "profile": "profile",
+            "endpoint_url": "https://example.com",
+        },
+    }
+
+    assert m.get_access_pair(direction) == ("expanded_username", "expanded_password")
+
+    assert m.update(
+        {
+            "access_pair": {"id": "username", "secret_variable": "_SPACK_TEST_PAIR_PASSWORD"},
+            "access_token_variable": "_SPACK_TEST_TOKEN",
+        },
+        direction,
+    )
+
+    assert m.to_dict() == {
+        "url": "https://example.com",
+        direction: {
+            "url": "http://example.org",
+            "access_pair": {"id": "username", "secret_variable": "_SPACK_TEST_PAIR_PASSWORD"},
+            "access_token_variable": "_SPACK_TEST_TOKEN",
+            "profile": "profile",
+            "endpoint_url": "https://example.com",
+        },
+    }
+
+    assert m.get_access_pair(direction) == ("username", "expanded_password")
+    assert m.get_access_token(direction) == "expanded_token"
+
 
 def test_mirror_name_or_url_dir_parsing(tmp_path):
     curdir = tmp_path / "mirror"
diff --git a/lib/spack/spack/util/s3.py b/lib/spack/spack/util/s3.py
index 5457abdca5..700db07135 100644
--- a/lib/spack/spack/util/s3.py
+++ b/lib/spack/spack/util/s3.py
@@ -94,20 +94,17 @@ def get_mirror_s3_connection_info(mirror, method):
 
     # access token
     if isinstance(mirror, Mirror):
-        access_token = mirror.get_access_token(method)
-        if access_token:
-            s3_connection["aws_session_token"] = access_token
-
-        # access pair
-        access_pair = mirror.get_access_pair(method)
-        if access_pair and access_pair[0] and access_pair[1]:
-            s3_connection["aws_access_key_id"] = access_pair[0]
-            s3_connection["aws_secret_access_key"] = access_pair[1]
-
-        # profile
-        profile = mirror.get_profile(method)
-        if profile:
-            s3_connection["profile_name"] = profile
+        credentials = mirror.get_credentials(method)
+        if credentials:
+            if "access_token" in credentials:
+                s3_connection["aws_session_token"] = credentials["access_token"]
+
+            if "access_pair" in credentials:
+                s3_connection["aws_access_key_id"] = credentials["access_pair"][0]
+                s3_connection["aws_secret_access_key"] = credentials["access_pair"][1]
+
+            if "profile" in credentials:
+                s3_connection["profile_name"] = credentials["profile"]
 
         # endpoint url
         endpoint_url = mirror.get_endpoint_url(method) or os.environ.get("S3_ENDPOINT_URL")