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

s3: provide a way to use key id and key secret directly #4175

Closed
Suor opened this issue Jul 7, 2020 · 7 comments · Fixed by #4224
Closed

s3: provide a way to use key id and key secret directly #4175

Suor opened this issue Jul 7, 2020 · 7 comments · Fixed by #4224
Labels
feature request Requesting a new feature p2-medium Medium priority, should be done, but less important

Comments

@Suor
Copy link
Contributor

Suor commented Jul 7, 2020

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:

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.

@triage-new-issues triage-new-issues bot added the triage Needs to be triaged label Jul 7, 2020
@efiop efiop added the feature request Requesting a new feature label Jul 7, 2020
@triage-new-issues triage-new-issues bot removed the triage Needs to be triaged label Jul 7, 2020
@efiop
Copy link
Contributor

efiop commented Jul 7, 2020

@Suor Have you considered changing the env for that call?

Also, I suppose your request is only about internal api, right?

@efiop efiop added the awaiting response we are waiting for your reply, please respond! :) label Jul 7, 2020
@Suor
Copy link
Contributor Author

Suor commented Jul 7, 2020

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.

@Suor
Copy link
Contributor Author

Suor commented Jul 7, 2020

Ideally I want key_id/key_secret pair be added to config. Any reason you want to avoid it?

@efiop
Copy link
Contributor

efiop commented Jul 7, 2020

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 🙂 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.

@Suor
Copy link
Contributor Author

Suor commented Jul 7, 2020

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.

@Suor
Copy link
Contributor Author

Suor commented Jul 7, 2020

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.

@efiop
Copy link
Contributor

efiop commented Jul 7, 2020

@Suor agreed. Let's do that.

@efiop efiop added p2-medium Medium priority, should be done, but less important and removed awaiting response we are waiting for your reply, please respond! :) labels Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Requesting a new feature p2-medium Medium priority, should be done, but less important
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants