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: RP for plugin-tee-verifiable-log-api #1260

Conversation

gene-zhan
Copy link

Relates to:

  1. Run verifiable Eliza in the TEE.
  2. Verifiable log for Eliza in the TEE
  3. RP for plugin-tee-verifiable-log #1259

Risks

Low

Background

What does this PR do?

This PR builds upon plugin-tee-verifiable-log by modifying the direct client to add remote attestation and query interfaces for verifiable logs.

To better understand what verifiable logs are and why we implemented this feature, it is necessary to refer to the preceding PR that introduces plugin-tee-verifiable-log and provides the context for its development.

What kind of change is this?

  • Feature
  • TEE plugin
  • Verifiable feature

Documentation changes needed?

Yes, we will add documentation about api detail.

Testing

Where should a reviewer start?

  1. Understand the existing plugin-tee

    1. We rely on Eliza's existing plugin-tee and use its key derivation interface.
  2. To better understand what verifiable logs are and why we implemented this feature, it is necessary to refer to the preceding PR that introduces plugin-tee-verifiable-log and provides the context for its development.

  3. Understand what plugin-tee-verifiable-log does

    1. Derive a key pair for verifiable logs: It uses the TEE to derive a key pair specifically for signing verifiable logs.
    2. Remote attestation: The public key of verifiable logs is embedded in the remote attestation report, making it accessible for external verification.
    3. Sign logs: This plugin accepts logs passed in from external modules (e.g., tweets sent and received by a Twitter client, or executed actions), signs them with the TEE-derived key pair, and stores them in the database.
    4. Verification: External entities can use the remotely attested public key to verify these logs, ensuring that certain actions were indeed performed by TEE Eliza.

Detailed testing steps

It have completed the integration tests and can run the pnpm test file in the test directory.

@HashWarlock HashWarlock self-requested a review December 20, 2024 04:19
@odilitime
Copy link
Collaborator

is this a duplicate of #1259 ?

@odilitime odilitime changed the base branch from main to develop December 20, 2024 04:30
@odilitime odilitime added the Plugin_new Mark PRs that are a new plugin label Dec 20, 2024
@@ -43,3 +43,42 @@ curl -X GET "http://localhost:3000/fine-tune/8566c47a-ada8-441c-95bc-7bb07656c4c
-H "Content-Type: application/json" \
-H "Authorization: Bearer jvBpxrTNqGqhnfQhSEqCdsG6aTSP8IBL".
```


## Verifiable Attestations
Copy link
Collaborator

@HashWarlock HashWarlock Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this!

I think the easiest way to insert into documentation is to add this as a section called Enable Verifiable Logs for the Eliza in TEE Doc docs/docs/advanced/eliza-in-tee.md. Then add a reference to your discord or discord user to contact about the implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can work on this together for formatting & further enhancements/tutorials down the road. Separate into to two Plugin Sections:

  • TEE Plugin
    • Core Components
  • TEE Verifiable Log Plugin
    • Core Components
  • Tutorial
    • Enable Verifiable Log
  • Conclusion
    • Mention contributors for implementation and who to reach out to learn more about Verifiable Logs in TEE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, let's work on this together. that’s exactly the idea behind building this plugin.

Let me explain further:

@HashWarlock @odilitime

@HashWarlock
Copy link
Collaborator

LGTM, outside of the 1 suggested edit. I can help document tomorrow and run a test to fix any bugs i may encounter

@odilitime odilitime changed the title RP for plugin-tee-verifiable-log-api feat: RP for plugin-tee-verifiable-log-api Dec 20, 2024
@@ -69,6 +71,9 @@ export class DirectClient {
const apiRouter = createApiRouter(this.agents, this);
this.app.use(apiRouter);

const apiLogRouter = createVerifiableLogApiRouter(this.agents, this);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If VERIFIABLE_LOGGING is not enabled, will this fail to start the agent? or will it just fail the API call?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fail the api call

@HashWarlock
Copy link
Collaborator

HashWarlock commented Dec 22, 2024

@odilitime @gene-zhan what are the @gene-sf changes? Is that some automation? I'm approving this PR now

@HashWarlock HashWarlock self-requested a review December 22, 2024 01:32
HashWarlock
HashWarlock previously approved these changes Dec 22, 2024
@gene-zhan
Copy link
Author

gene-zhan commented Dec 22, 2024

@odilitime @gene-zhan what are the @gene-sf changes? Is that some automation? I'm approving this PR now

sorry,I have configured two GitHub accounts locally. The problem with @gene-sf is that I forgot to switch the configuration file when submitting the code.

I have completed the following operations:

  • According to your review, I have fixed the code.
  • Updated the readme that is easier to understand.
  • Merged the latest version of main code.

@shakkernerd shakkernerd deleted the branch elizaOS:develop December 22, 2024 07:02
@gene-zhan
Copy link
Author

@shakkernerd I noticed that PRs #1331 and #1333 were reopened and this PR closed, which has left me a bit confused about the current direction. Should I merge my changes into the develop branch, or is there a different branch we should treat as the standard? I’ve submitted #1369. both cpppppp7 and I have been working diligently to make progress on this code contribution. I’d appreciate any clarification to ensure we align our efforts effectively. 😊

@shakkernerd
Copy link
Member

Hi @gene-zhan Thanks for bringing this to my attention.
PRs that were closed about 5hours ago were due to develop branch getting accidentally deleted.
And yes, the develop remains the standard to open PRs to.

Good to hear about your active contributions.
Apologies for the confusion.

@shakkernerd shakkernerd reopened this Dec 22, 2024
@gene-zhan
Copy link
Author

@shakkernerd Thank you for your response! I accidentally submitted duplicate PRs (#1259 #1260 #1331, #1333, and #1369). I recommend prioritizing the merge of #1369, as this branch already includes the latest develop code. The other branches contain the same code but currently have conflicts that need to be resolved. Merging #1369 would help reduce additional workload. Thank you again for your understanding and support! 😊

@gene-zhan gene-zhan dismissed HashWarlock’s stale review December 23, 2024 05:09

The merge-base changed after approval.

@gene-zhan
Copy link
Author

@shakkernerd @odilitime @HashWarlock How can I help facilitate the merging of this PR?

@shakkernerd
Copy link
Member

shakkernerd commented Dec 23, 2024

Hi @gene-zhan which PRs do you need attention on and which ones should be closed?

@gene-zhan
Copy link
Author

@shakkernerd

I would like to request the following actions:

  1. Close the following pull requests (PRs):
  1. Merge PR feat: RP for plugin-tee-verifiable-log #1369
    Please proceed with merging PR feat: RP for plugin-tee-verifiable-log #1369 into the develop branch as it contains the necessary updates/fixes we need.
    Let me know if there are any issues or additional steps required. Thank you!

@shakkernerd
Copy link
Member

Okay, thanks for listing them out. I will get to it.

@shakkernerd
Copy link
Member

Closing in favor of #1369

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Plugin_new Mark PRs that are a new plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants