ubiquity-os / ubiquity-os-kernel

1 stars 19 forks source link

Fix signature and add plugin github token #168

Closed whilefoo closed 2 weeks ago

whilefoo commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

gentlementlegen commented 2 weeks ago

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

whilefoo commented 2 weeks ago

@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 commented 2 weeks ago

@Ubiquityos why was kv disabled?

0x4007 commented 2 weeks ago

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.

ubiquity-os-beta[bot] commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 2 weeks ago

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

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

gentlementlegen commented 2 weeks ago

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

0x4007 commented 2 weeks ago

Can someone make a spec?

gentlementlegen commented 2 weeks ago

@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 commented 2 weeks ago

I'll clone your repo and try

gentlementlegen commented 2 weeks ago

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