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

Passing an initialized auth backend? #157

Open
xarses opened this issue Sep 16, 2024 · 7 comments
Open

Passing an initialized auth backend? #157

xarses opened this issue Sep 16, 2024 · 7 comments

Comments

@xarses
Copy link
Contributor

xarses commented Sep 16, 2024

I'm working on adding in a custom auth backend for me to use with AWS Elastic Container Registry (ECR).

Currently the process for adding a new backend appears to be to create a parameterless descendent of oras.auth.base.AuthBackend and add it the mapping of auth backends.

Seems fine. However ECR credentials are specific to each registry, so ideally we'd prepare the auth client by constructing it with the account already known. However this pattern would require that the registry could be passed an instantiated auth instance. I could also skim the account number out of the request in authenticate_request, but that feels a little backwards.

@vsoch
Copy link
Contributor

vsoch commented Sep 16, 2024

Can you walk me through the desired interaction you'd like the user to take, and we can think about it?

@xarses
Copy link
Contributor Author

xarses commented Sep 16, 2024

I'm not sure it matters too much. My current flow is to look at the registry URI and split the desired parts out of the URL and construct the registry client and return it, I cache the resulting registry to avoid requesting new credentials when processing images (I work with a lot of images). In this case, I've implemented the Registry being able to accept a constructed auth_backend instance

class ECRAuth(oras.auth.BasicAuth):
    def __init__(self, account: str):
        self.account = account
        super().__init__()
        self.update_auth()
...

def __init_ecr_registry(registry: str):
    account, _, _, region, *_ = registry.split(".")
    reg = Registry(auth_backend=ECRAuth(account))
    return reg

Writing this out, it would be nearly indifferent between this method, and siphoning the account out of a request requiring auth. The only differences I can think of is that in this flow, I construct the initial auth and designate the account to the instance. This allows for the auth flow with ECR via tha boto3 client to be performed at instantiation which tends to improve understanding what is going on when this leg of the auth fails.

It further locks the instance to the given account and would prevent the instance from accidentally switching between registries for which the auth was currently valid (if locked it would raise an auth failure) If we silently took the account from the request uri, then it would be possible for the client to attempt to switch the auth to another registry (if it was coded to refresh on a 401, which I wanted to support in this client). Such a call to boto3 might be at additional risk of failing if the caller doesn't have access to that registry (I currently control this when I determine which Registry I want to use from my cache of constructed registries)

@vsoch
Copy link
Contributor

vsoch commented Sep 17, 2024

Could that not be accomplished with a registry init function that allows for kwargs that are passed to the auth backend? Or for example have some parameter that can hold "registry" as you have it above (or a similar name) that is passed to the backend to parse, or just ignored.

@xarses
Copy link
Contributor Author

xarses commented Sep 17, 2024

If I'm following, you're proposing something like

class Registry:
    def __init__(self, ..., auth_backend, auth_kwargs):
        ...
        self.auth = oras.auth.get_auth_backend(
            auth_backend, self.session, **auth_kwargs
        )

@vsoch
Copy link
Contributor

vsoch commented Sep 17, 2024

yes! That would definitely work - that's even better than what I had in mind to have auth_kwargs as separate from others. I would default to None and then do:

class Registry:
    def __init__(self, ..., auth_backend, auth_kwargs=None):
        ...
        auth_kwargs = auth_kwargs or {}
        self.auth = oras.auth.get_auth_backend(
            auth_backend, self.session, **auth_kwargs
        )

@xarses
Copy link
Contributor Author

xarses commented Sep 17, 2024

Sounds good, this method would still require that the extension adds its self to the oras.auth.auth_backends map. Are you open to allowing auth_backend being passed as a subclass of oras.auth.base.AuthBackend or a str and having oras.auth.get_auth_backend(...) handle the two?

@vsoch
Copy link
Contributor

vsoch commented Sep 17, 2024

Do you mean passing the auth backend directly instead of calling get_auth_backend? Yes that would be OK - likely we can have:

  • auth_backend (the instance, defaults to None)
  • auth_backend_name (the current auth_backend, string name that calls get_auth_backend.
  • A check that after those two, we have self.auth defined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants