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

GitHub Storage: session string exposed #12

Open
Keyrxng opened this issue Oct 23, 2024 · 14 comments
Open

GitHub Storage: session string exposed #12

Keyrxng opened this issue Oct 23, 2024 · 14 comments

Comments

@Keyrxng
Copy link
Collaborator

Keyrxng commented Oct 23, 2024

Relates to #3

The config repo is now public and GitHub layer storage is supposed to push into that repo.

This plugin has one ultra sensitive env var which is the MTProtoAPI Session String.

If that is stored in a public repo as raw text the TG can be hijacked easily.

  1. Add encryption/decryption
  2. Find another way to store just that var
  3. Adjust the storage layer solution that has been presented as-is

original context

@Keyrxng
Copy link
Collaborator Author

Keyrxng commented Oct 23, 2024

This is an urgent task as it's a blocker now for #3 and estimate it to be about 4 hours+ of work depending on the approach to be implemented

Any longer than two days without direction or input and I'm going to disable the GitHub storage adapter and return to using the Supabase one so I can move that PR forward.

@Keyrxng
Copy link
Collaborator Author

Keyrxng commented Oct 23, 2024

rfc @0x4007

@0x4007
Copy link
Member

0x4007 commented Oct 26, 2024

We can just use our existing X25519 encryption @rndquu rfc

@Keyrxng
Copy link
Collaborator Author

Keyrxng commented Oct 27, 2024

You mentioned during review that we'll make Global Storage private and handle it that way but yes we could use the same encryption we use in other projects if that's the method to implement.

@rndquu
Copy link
Member

rndquu commented Oct 29, 2024

@Keyrxng

Trying to understand how the whole plugin works.

  1. A new telegram bot (let's say ubiquity-os-telegram-bot) is created
  2. Contributor opens ubiquity-os-telegram-bot and prints /start
  3. ubiquity-os-telegram-bot receives a webhook from the user where we save username and user id to some storage (supabase/github json/whatever...)
  4. Contributor solves a github issue, issue is closed as "completed", ubiquity-os github app receives a webhook
  5. ubiquity-os github app retrieves contributor's telegram username with user id and (using ubiquity-os-telegram-bot credentials) sends a direct message in telegram to a user that issue has been resolved

Does it work this way?

@Keyrxng
Copy link
Collaborator Author

Keyrxng commented Oct 29, 2024

  1. kernel-telegram receives payloads from GitHub/UbiquityOS when the payment comment is posted or edited. We use the BotFather API to send a message to the user.
  2. kernel-telegram receives a telegram payload when a user calls /register keyrxng, /subscribe, etc. With these payloads we don't have the kernel' auth token yml config settings for the plugin nothing like that. It effectively runs on env and whatever TypeBox defaults we use. So to use Octokit we need an access token, the app provides that. For storage, app was required for private repo writes. If storage is public, use encryption no problem (but doesn't solve for other plugins with sensitive data to store). Private storage is the better approach long term.

  • user opens TG and calls /register keyrxng: TG sends our worker a bot_command payload, we call rest.getByUsername and write to SB or GH.
  • on TG, user calls /subscribe: selects a trigger which runs on GitHub webhooks, we update storage.
  • when said webhook fires on GH (issues.unassigned, requested_review, etc...) UbiquityOS kernel sends it to the telegram-kernel worker (because it's a BotFather feature), it pulls from DB for username match and which triggers are subscribed to

@Keyrxng
Copy link
Collaborator Author

Keyrxng commented Oct 29, 2024

The session string is for the MTProto API which is responsible for the workrooms feature, it doesn't affect notifications as that's a Bot API feature.

  1. Interface to come to allow plugins to ping this worker and send an arbitrary msg from any of our plugins. Imagine Security Monitoring implemented, we use these DMs to notify team etc.
  2. Plugin devs are quota'd for rate limit; we build analytics into plugin-template feed it back to the kernel, pass it off to a dedicated worker which tracks usage in real-time across all incoming plugins/org/developer etc and we are able to message all parties if something is wrong or about to break.
  3. Able to post .fatal() errors from the logger straight into a dedicated core team TG channel

There are a lot more applications for this but after #13 is fully installed which will be later today I'll make a task and aim to really improve the docs etc for this plugin as it is a little complex

@Keyrxng
Copy link
Collaborator Author

Keyrxng commented Oct 29, 2024

The session string solve is easy:

  1. We encrypt with same approach we use elsewhere, I'll write another setup CLI for this process.
  2. We use a private storage location, so a private repo. <-- this solves for more than just my use-case and I can't see the downsides of it personally.

@rndquu
Copy link
Member

rndquu commented Oct 29, 2024

@Keyrxng

  1. ubiquity-os-kernel-telegram is a telegram bot, not a github app, correct?
  2. ubiquity-os-kernel-telegram is also able to receive webhooks from https://github.com/ubiquity-os/ubiquity-os-kernel, correct?
  3. Is the flow in the following picture correct?
flow

@Keyrxng
Copy link
Collaborator Author

Keyrxng commented Oct 29, 2024

  1. It's not a GitHub app but requires usage of one for an access token (and writes to private storage repos later)
  2. It's a kernel plugin in the traditional sense yes, same as start-stop or any other.
  3. Yeah that's it. Kernel receives and forwards as normal; all logic along that path has full kernel input. The clear difference is; this plugin receives payloads from a source other than the kernel and along that path it has zero kernel input (yml config settings, etc...), which is why we need a token and smart usage of TypeBox schema defaults.

Separate from that you need to also consider that the Telegram side has two "branches" we'll call them:

  • MTProto API: personal account to create workrooms, it can DM without a previous connection. It runs via actions only and has nothing to do with the BotFather. It needs the session string.
  • Bot API: you know this well already but it runs via the worker; could likely use it within actions but not the other way around

@rndquu
Copy link
Member

rndquu commented Oct 29, 2024

MTProto API: personal account to create workrooms, it can DM without a previous connection. It runs via actions only and has nothing to do with the BotFather. It needs the session string.

Why can't it be run in a cloudflare worker?

@rndquu
Copy link
Member

rndquu commented Oct 29, 2024

As far as I understand from the docs current plugin needs to be defined 2 times in the config:

  1. As a cloudflare worker (for sending messages via the telegram bot)
  2. As a github action (solely for the "workroom" feature when a new telegram group is created for each new github issue)

Correct?

@Keyrxng
Copy link
Collaborator Author

Keyrxng commented Oct 29, 2024

Basically yes.

  1. The worker receives payloads from both the kernel so it can parse comments, see who got disqualified etc and telegram so it can handle slash commands etc via telegram. We can send messages, remove from chat, handle forums all of that.
  2. As an action solely for the workroom feature (and any future MTProto requiring features)

Why can't it be run in a cloudflare worker?

The packages are not built for that env exactly, I did try alt libs and to find a way but I gathered we'd need something like Heroku or whatever to host the server


This originally dispatched it's own workflows and required defined only once through the worker url. We needed the kernel' APP_ID and APP_PRIVATE_KEY, this was rejected during review deemed unsafe to expose them but they have been exposed via org secrets now. So I think we should go back to self-dispatching

@rndquu
Copy link
Member

rndquu commented Dec 11, 2024

So I think we should go back to #20

@Keyrxng So can we close current issue as "not planned" in favor of #20?

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

3 participants