Repository: iterative/dvc
Pull request: S3: Use key_id and key_secret directly
Fixes https://github.com/iterative/dvc/issues/4175

@efiop 




Problem statement:
s3: provide a way to use key id and key secret directly
For now the only way to pass credentials to S3 is using credentials file and the env var. This is very inconvenient when trying to use dvc as a library. For now I hacked it like:

```python
def pull(repo):
    # In memory config update to avoid validation
    repo.config["core"]["remote"] = "vdefault"
    repo.config["remote"]["vdefault"] = {"url": ..., "key_id": ..., "key_secret": ...}

    fix_s3()
    repo.pull()


from funcy import once, monkey, cached_property, wrap_prop  # noqa

@once
def fix_s3():
    import threading
    from dvc.remote.s3 import S3RemoteTree

    @monkey(S3RemoteTree, "s3")
    @wrap_prop(threading.Lock())
    @cached_property
    def s3(self):
        import boto3

        session = boto3.session.Session(
            profile_name=self.profile,
            region_name=self.region,
            # Pass these two from config to session
            aws_access_key_id=self.config.get("key_id"),
            aws_secret_access_key=self.config.get("key_secret"),
        )

        return session.client("s3", endpoint_url=self.endpoint_url, use_ssl=self.use_ssl)
```

Would be nice to have it inside `dvc.remote.S3RemoteTree`.

Hints from issue discussion:
@Suor Have you considered changing the env for that call?

Also, I suppose your request is only about internal api, right?
Changing env for the process will leak it to other threads and also may leak to subsequent calls. Starting a new process is a solution, but a heavy one.

I am using `repo.cloud.pull()`, but `repo.pull()` should work the same.
Ideally I want key_id/key_secret pair be added to config. Any reason you want to avoid it?
> Ideally I want key_id/key_secret pair be added to config. Any reason you want to avoid it?

Same reason as with any plain-text passwords :slightly_smiling_face: We could make it api-only, but feels like a hack, still. I guess the only current nice way to do it is to just use a tempfile, dump keys into and and then use it as credentailpath in the config.
That would work too. However, if we want api to be usable we should stop doing things like that) Non-saveable config option is fine with me too. Hack or not. 
And BTW, we already have it for oss, gdrive and ssh. Maybe it's ok to save password/key to local config. People are storing this anyway in some `~/.aws/credentials`, which is a [blessed way](https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html). 
@Suor agreed. Let's do that.

Commit message: remove redundant default

Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>

Patch to review:
diff --git a/dvc/config.py b/dvc/config.py
index a43cb16f2f..058a698970 100644
--- a/dvc/config.py
+++ b/dvc/config.py
@@ -149,6 +149,8 @@ class RelPath(str):
                     "profile": str,
                     "credentialpath": str,
                     "endpointurl": str,
+                    "s3_key_id": str,
+                    "s3_key_secret": str,
                     Optional("listobjects", default=False): Bool,
                     Optional("use_ssl", default=True): Bool,
                     "sse": str,
diff --git a/dvc/tree/s3.py b/dvc/tree/s3.py
index 17d58301f1..574a47d48b 100644
--- a/dvc/tree/s3.py
+++ b/dvc/tree/s3.py
@@ -54,6 +54,9 @@ def __init__(self, repo, config):
 
         self._append_aws_grants_to_extra_args(config)
 
+        self.key_id = config.get("s3_key_id")
+        self.key_secret = config.get("s3_key_secret")
+
         shared_creds = config.get("credentialpath")
         if shared_creds:
             os.environ.setdefault("AWS_SHARED_CREDENTIALS_FILE", shared_creds)
@@ -63,9 +66,14 @@ def __init__(self, repo, config):
     def s3(self):
         import boto3
 
-        session = boto3.session.Session(
-            profile_name=self.profile, region_name=self.region
-        )
+        session_opts = dict(profile_name=self.profile, region_name=self.region)
+
+        if self.key_id:
+            session_opts["aws_access_key_id"] = self.key_id
+        if self.key_secret:
+            session_opts["aws_secret_access_key"] = self.key_secret
+
+        session = boto3.session.Session(**session_opts)
 
         return session.client(
             "s3", endpoint_url=self.endpoint_url, use_ssl=self.use_ssl
diff --git a/tests/unit/remote/test_s3.py b/tests/unit/remote/test_s3.py
index 570371c0c7..4ecddeefec 100644
--- a/tests/unit/remote/test_s3.py
+++ b/tests/unit/remote/test_s3.py
@@ -6,6 +6,8 @@
 bucket_name = "bucket-name"
 prefix = "some/prefix"
 url = f"s3://{bucket_name}/{prefix}"
+key_id = "key_id"
+key_secret = "key_secret"
 
 
 @pytest.fixture(autouse=True)
@@ -57,3 +59,11 @@ def test_grants_mutually_exclusive_acl_error(dvc, grants):
 def test_sse_kms_key_id(dvc):
     tree = S3RemoteTree(dvc, {"url": url, "sse_kms_key_id": "key"})
     assert tree.extra_args["SSEKMSKeyId"] == "key"
+
+
+def test_key_id_and_secret(dvc):
+    tree = S3RemoteTree(
+        dvc, {"url": url, "s3_key_id": key_id, "s3_key_secret": key_secret}
+    )
+    assert tree.key_id == key_id
+    assert tree.key_secret == key_secret


Code review comments:
diff_hunk: @@ -6,6 +6,8 @@
 bucket_name = "bucket-name"
 prefix = "some/prefix"
 url = f"s3://{bucket_name}/{prefix}"
+key_id = "key_id"
+key_secret = "key_secret"
path: tests/unit/remote/test_s3.py
text: @user1:
a bit worried of some nasty bug like that :smiley: 
```suggestion
key_id = "key-id"
key_secret = "key-secret"
```