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

Fix signature and add plugin github token #168

Merged

Conversation

whilefoo
Copy link
Contributor

Resolves #162

This PR solves two issues:

  1. The Action SDK was using a wrong token to return data to the kernel
  2. The signature was not working on the Action plugin SDK

1. issue

The kernel was using the authToken passed from the kernel which is org-bound installation token that only has permissions for the organization which the event was triggered from. To trigger a repository dispatch from the plugin's repo where the Action is ran we need the plugin's repo token which is automatically provided in the Action runtime in secrets.GITHUB_TOKEN.
The solution is to use an env variable PLUGIN_GITHUB_TOKEN to provide the token from secrets.GITHUB_TOKEN

2. issue

Inputs are signed by making an object containing all inputs, JSON.stringify it and generating a signature from that string.

This is fine for Worker plugins because that same object that was used to make a signature is sent over the wire to the plugin. The Javascript engine then parses the request's body and makes an essentially identical object with identical order of keys, so when the SDK verified the signature, it also stringified the object which resulted in the same string so the verification was successful.

This however was not the case with Action plugins. Github stores the inputs in github.context.payload.inputs and has its own way of parsing the inputs making the order of keys in the object different so when the SDK stringified the object, it made a different string and when it tried to verify the signature of course it wasn't valid because the string was not the same.

I resolved this by also producing an object with the same order of keys in the SDK and then verifying it with the signature. I also had to modify the tests to make inputs the same way the kernel does (in the same order)

@gentlementlegen
Copy link
Member

Hard to test locally because when I do I end up using the Hono server which bypasses the issue. Otherwise fine with the changes, if you can fix Knip and Type Checks.

@whilefoo
Copy link
Contributor Author

I've just remember to turn createPlugin into a sync function instead of async because there's no need for async and that fixes the export default issue.

As for the Knip I'm not sure why it's saying the CloudflareKv is unresolved import, it works fine on my end
The type check is also related to CloudflareKv but if I run it on my end it's doesn't show any errors.
On another note I think we should disable Sonar for tests because it's quite annoying

@0x4007
Copy link
Member

0x4007 commented Oct 26, 2024

@gentlementlegen On you to process this. Nobody knows the kernel as much as you two do

@gentlementlegen gentlementlegen self-requested a review October 27, 2024 12:23
@gentlementlegen gentlementlegen removed their assignment Oct 27, 2024
@gentlementlegen
Copy link
Member

@whilefoo We decided to disable Cloudflare KV so I think you should use EmptyStore or KvStore interface (CloudflareKv is supposed to be gone?)

@whilefoo
Copy link
Contributor Author

@whilefoo We decided to disable Cloudflare KV so I think you should use EmptyStore or KvStore interface (CloudflareKv is supposed to be gone?)

I'm guessing this was done temporarily until we figure out a solution?

@0x4007
Copy link
Member

0x4007 commented Oct 27, 2024

@UbiquityOS why was kv disabled?

@0x4007
Copy link
Member

0x4007 commented Oct 27, 2024

We don't use sequential outputs as inputs to other plugins so instead we made all plugins in a chain run concurrently. We can re add once we actually need to chain outputs as inputs to other plugins.

Copy link

KV was disabled primarily because KV reads incur quota costs, making it financially inefficient for our current needs. Although KV provides a lightweight solution for persisting data between worker runs, the cost associated with frequent reads became a significant concern. This led us to consider alternative storage options such as Supabase and Redis, which may offer more cost-effective and scalable solutions for data persistence without the overhead of KV read quotas. Additionally, the priority to ship version 2 (v2) took precedence, delaying the migration to a more sustainable storage solution.

@Keyrxng
Copy link
Contributor

Keyrxng commented Oct 27, 2024

I had my doubts on the effectiveness of the embeddings search stuff as we didn't really start small and iterate it felt like a jump into the deep end with little proof it'd perform at all never mind well but it really does hold it's own when it works doesn't it. Hats off to you shiv (can't tag fsr)

@gentlementlegen
Copy link
Member

Funny to see the answer the bot gives. The immediate reason is that we actually hardly ever chained plugins therefore never needed the output of the previous one for the next one, effectively making storing the outputs of the values in KV useless. Until this is needed, we can save on KV usage by disabling it. I encapsulated the class so getting KV back should just be a matter of changing the class that is used.

@gentlementlegen gentlementlegen merged commit a47ea4b into ubiquity-os:development Oct 28, 2024
3 checks passed
@gentlementlegen
Copy link
Member

gentlementlegen commented Oct 28, 2024

@whilefoo So now we have a redundant PLUGIN_GITHUB_TOKEN that is the same as GITHUB_TOKEN?

Also it seems that the fix didn't resolve the invalid signature: https://github.com/ubiquity-os-marketplace/daemon-disqualifier/actions/runs/11547073306/job/32136379674

@whilefoo
Copy link
Contributor Author

@whilefoo So now we have a redundant PLUGIN_GITHUB_TOKEN that is the same as GITHUB_TOKEN?

I chose the name PLUGIN_GITHUB_TOKEN because I thought it would be more descriptive of which token is this

Also it seems that the fix didn't resolve the invalid signature: https://github.com/ubiquity-os-marketplace/daemon-disqualifier/actions/runs/11547073306/job/32136379674

My test was successful: https://github.com/ubiquity-whilefoo/test-action-signature/actions/runs/11550317959/job/32145049979

I think the problem is in the public key, is it set to the beta bot?

@0x4007
Copy link
Member

0x4007 commented Oct 28, 2024

Well we only have two right? Prod and dev bot.

We need support for both it seems. What's the solution?

@gentlementlegen
Copy link
Member

Plugins should properly use environments within their secrets to have different values set for development and production most likely.

@0x4007
Copy link
Member

0x4007 commented Oct 28, 2024

Can someone make a spec?

@gentlementlegen
Copy link
Member

@whilefoo I don't know if I am doing something wrong again but my workers wok perfectly and the same within actions breaks:
https://github.com/Meniole/daemon-pricing/actions/runs/11567826807/job/32198771226
Yet I am pretty sure the two of them have the same public key, and the kernel used is the same. I'll keep investigating.

@whilefoo
Copy link
Contributor Author

I'll clone your repo and try

@gentlementlegen
Copy link
Member

@whilefoo I found out that Action workflows did not forward the KERNEL_PUBLIC_KEY which meant they used the embedded one. That's something we might wanna address throughout our plugins.

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

Successfully merging this pull request may close these issues.

Wrong auth token
4 participants