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

feat(backend): Ensure validity of OAuth credentials during graph execution #8191

Conversation

Pwuts
Copy link
Member

@Pwuts Pwuts commented Sep 26, 2024

Changes 🏗️

backend

  • Added Pydantic model (de)serialization support to @expose decorator

backend.executor

  • Change credential injection mechanism to acquire credentials just before execution
    • Also locks the credentials for the duration of the execution

backend.server

  • Add IntegrationCredentialsManager to handle and synchronize credentials-related operations
  • Make backend.server.integrations module with router, creds_manager, and utils in it

autogpt_libs

  • Add mutexes to SupabaseIntegrationCredentialsStore to ensure thread-safety
  • Rewrite KeyedMutex into RedisKeyedMutex and move to autogpt_libs.utils.synchronize

@github-actions github-actions bot added platform/backend AutoGPT Platform - Back end size/m labels Sep 26, 2024
Copy link

netlify bot commented Sep 26, 2024

Deploy Preview for auto-gpt-docs ready!

Name Link
🔨 Latest commit 519e396
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/670803c8d89969000804860f
😎 Deploy Preview https://deploy-preview-8191--auto-gpt-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

- Move `KeyedMutex` to `autogpt_libs.utils.synchronize`
- Add locks to transactions in `SupabaseIntegrationCredentialsStore`
@github-actions github-actions bot added size/l conflicts Automatically applied to PRs with merge conflicts and removed size/m labels Sep 27, 2024
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

…entials JIT

Also:
- Add exposed `acquire_credentials`, `release_credentials` methods to `AgentServer`
- Add Pydantic model auto-deserialization support to `@expose` decorator
- Make `ExecutionManager.agent_server_client` cached
@github-actions github-actions bot added size/xl and removed size/l labels Sep 27, 2024
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Sep 27, 2024
Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@Pwuts Pwuts marked this pull request as ready for review September 27, 2024 22:33
@Pwuts Pwuts requested a review from a team as a code owner September 27, 2024 22:33
@Pwuts Pwuts requested review from aarushik93, majdyz and ntindle and removed request for a team September 27, 2024 22:33
@Pwuts Pwuts force-pushed the reinier/open-1891-ensure-oauth-credentials-are-fresh-before-using-them-in branch from d48fbfe to fda2cf6 Compare September 27, 2024 22:48
@Pwuts Pwuts force-pushed the reinier/open-1891-ensure-oauth-credentials-are-fresh-before-using-them-in branch from fda2cf6 to 5f3ef42 Compare September 27, 2024 23:00
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Sep 29, 2024
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Oct 3, 2024
Copy link
Contributor

github-actions bot commented Oct 3, 2024

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

majdyz
majdyz previously approved these changes Oct 7, 2024
Copy link
Contributor

github-actions bot commented Oct 9, 2024

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Oct 9, 2024
@Pwuts Pwuts requested a review from majdyz October 9, 2024 15:48
@ntindle
Copy link
Member

ntindle commented Oct 9, 2024

Why are we locking the credential for the duration of the execution? Is it a write-only lock? Seems like we would basically be forcing one block using a credential at a time when blocks themselves can't modify the credentials.

I'm not convinced that this is desired behavior as I may want to update my credentials without stopping my agents

@Pwuts
Copy link
Member Author

Pwuts commented Oct 10, 2024

@ntindle because refreshing OAuth credentials causes the old tokens to become invalid. If we would allow refreshing tokens while they are in use, that would break the ongoing execution.

Also, IntegrationCredentialsManager.get(..) will refresh OAuth credentials as needed, so it isn't a read-only operation.

@Pwuts Pwuts requested a review from majdyz October 10, 2024 13:55
@ntindle
Copy link
Member

ntindle commented Oct 10, 2024

my understanding of in-use is for the tiny time to pull the credential and send the request. the implementations definition is when a block that uses that credential is running

If I have five blocks that all use that same credential and I want to update it, when will it be updated? When the next block starts running? What if I have multiple agents using that credential? we could be basically locking a credential forever without the ability to update it

@Pwuts
Copy link
Member Author

Pwuts commented Oct 10, 2024

my understanding of in-use is for the tiny time to pull the credential and send the request. the implementations definition is when a block that uses that credential is running

If I have five blocks that all use that same credential and I want to update it, when will it be updated? When the next block starts running? What if I have multiple agents using that credential? we could be basically locking a credential forever without the ability to update it

Your concerns are valid, but I think they are addressed in the current implementation:

  • Credentials are locked while in use, which is while a node that uses them is running. I am yet to see a case where this causes issues, e.g. a long-running block that needs credentials. The total running duration of the parent graph is not a factor.
    • Because getting credentials can result in a refresh (= invalidation + replacement) of the stored credentials, getting is an operation that potentially requires read/write access.
    • Checking whether a token has to be refreshed is subject to an additional refresh scoped lock to prevent unnecessary sequential refreshes when multiple executions try to access the same credentials simultaneously.
  • We MUST lock credentials while in use to prevent them from being invalidated while they are in use, e.g. because they are being refreshed by a different part of the system.
  • I implemented a two-tier locking mechanism in which updating gets priority over getting credentials. This is to prevent a long queue of waiting get requests from blocking essential credential refreshes or user-initiated updates.

It is possible to implement a reader/writer locking system where either multiple readers or a single writer can have simultaneous access, but this would add a lot of complexity to the mechanism. I'll look into it if the current ("simple") mechanism causes too much latency, which I don't expect.

@ntindle
Copy link
Member

ntindle commented Oct 10, 2024

Note that when this merges, it was discussed to update this to use a more complex locking mechanism, but that was seen as unlikely to be needed for now and highly likely to significantly increase complexity. If we need to do that, we can later on

@Pwuts Pwuts merged commit 992989e into master Oct 10, 2024
14 checks passed
@Pwuts Pwuts deleted the reinier/open-1891-ensure-oauth-credentials-are-fresh-before-using-them-in branch October 10, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Ensure OAuth credentials are fresh before using them in graph execution
4 participants