ubiquity-os / ubiquity-os-kernel

1 stars 19 forks source link

Decrypt sensitive config parameters #90

Open rndquu opened 3 months ago

rndquu commented 3 months ago

Right now there are 2 ways to hide sensitive bot's config parameters:

  1. Make a config repository private (organization collaborators can still see the values though)
  2. Fork plugin, self host and set sensitive parameters in env variables (bad UX for partners)

Sensitive config parameters could be encrypted via our own x25519_PUBLIC_KEY (the same one we use for encrypting partners' private keys) and the kernel could then decrypt them.

So as a part of this issue the kernel should be able to decrypt config parameters on initial config parsing.

The expected flow for parsing a single config param:

  1. Try to decrypt the parameter.
  2. If it is decrypted then use the decrypted parameter.
  3. If there's a decryption error then assume the parameter is not encrypted and use a raw value.

Notice that there is a difference between decrypting:

  1. Unencrypted param
  2. Encrypted param with another PK

I hope https://doc.libsodium.org/ distinguishes those 2 errors because in the 2nd case it will be a very subtle bug and the kernel should ideally throw an error like The config parameter encryption is invalid so that partner knew he did something wrong on encrypting a param.

P.S. Handy tool: https://keygen.ubq.fi/

rndquu commented 3 months ago

@gentlementlegen Pls check the description

0x4007 commented 2 months ago
Error: incorrect key pair for the given ciphertext

I believe this is the error thrown for

  1. Encrypted param with another PK

Another idea for a future vision of mine (perhaps out of scope for this task) is to abstract the GitHub UI with a Ubiquity app that looks like the following:

  1. Login with GitHub
  2. Setup wizard with some text input fields for the secrets
  3. Save button

And behind the scenes we handle the forking on behalf of the user and populate the secrets.

This approach can generally allow us to build on top of GitHub as our backend while still being able to control the end user experience.

rndquu commented 2 months ago

Another idea for a future vision of mine (perhaps out of scope for this task) is to abstract the GitHub UI with a Ubiquity app > that looks like the following:

  1. Login with GitHub
  2. Setup wizard with some text input fields for the secrets
  3. Save button

This is basically https://github.com/ubiquity/onboard.ubq.fi v2 where users should be able to edit their configs via UI.

https://github.com/ubiquity/onboard.ubq.fi/issues/22

whilefoo commented 2 months ago

So you propose to try to decrypt every parameter in the config instead of creating a special prefix like _UBIQUITY_ENCRYPTED_PARAM=?

rndquu commented 2 months ago

So you propose to try to decrypt every parameter in the config instead of creating a special prefix like _UBIQUITY_ENCRYPTED_PARAM=?

Yes

I don't fully understand how the _UBIQUITY_ENCRYPTED_PARAM prefix may be used. For example, if partner wants to encrypt the evmPrivateEncrypted param then how to tell the kernel that param is encrypted? Either via some custom prefix, like evmPrivateEncrypted_UBIQUITY_ENCRYPTED_PARAM or via introducing a new param option so the config could look like:

plugins:
  issues.closed:
    - uses:
      - plugin: rndquu/conversation-rewards@fix/nonce-generation
        with:
          evmPrivateEncrypted: 
            value: "wOzNgt-yKT6oFlOVz5wrBLUSYxAbKGE9Co-yvT8f9lePsx7wJwPVugS9186zdhr1T4UpkpXvq9ii5M-nWfrydMnllSkowH4LirRZsHbvRVSvDoH_uh80p6HpwqDSG3g4Nwx5q0GD3H-ne4vwXMuwWAHd"
            encrypted: true

I wanted things to be backwards compatible. But if decrypting every param takes time (or there are other issues with this approach) then we could stick to the encrypted option param or suffixes.

whilefoo commented 2 months ago

like this

plugins:
  issues.closed:
    - uses:
      - plugin: rndquu/conversation-rewards@fix/nonce-generation
        with:
          evmPrivateKey: "_UBIQUITY_ENCRYPTED_PARAM=wOzNgt-yKT6oFlOVz5wrBLUSYxAbKGE9Co-yvT8f9lePsx7wJwPVugS9186zdhr1T4UpkpXvq9ii5M-nWfrydMnllSkowH4LirRZsHbvRVSvDoH_uh80p6HpwqDSG3g4Nwx5q0GD3H-ne4vwXMuwWAHd"
rndquu commented 2 months ago

like this

plugins:
  issues.closed:
    - uses:
      - plugin: rndquu/conversation-rewards@fix/nonce-generation
        with:
          evmPrivateKey: "_UBIQUITY_ENCRYPTED_PARAM=wOzNgt-yKT6oFlOVz5wrBLUSYxAbKGE9Co-yvT8f9lePsx7wJwPVugS9186zdhr1T4UpkpXvq9ii5M-nWfrydMnllSkowH4LirRZsHbvRVSvDoH_uh80p6HpwqDSG3g4Nwx5q0GD3H-ne4vwXMuwWAHd"

For me this way of telling the kernel that param is encrypted is more straightforward:

plugins:
  issues.closed:
    - uses:
      - plugin: rndquu/conversation-rewards@fix/nonce-generation
        with:
          evmPrivateEncrypted: 
            value: "wOzNgt-yKT6oFlOVz5wrBLUSYxAbKGE9Co-yvT8f9lePsx7wJwPVugS9186zdhr1T4UpkpXvq9ii5M-nWfrydMnllSkowH4LirRZsHbvRVSvDoH_uh80p6HpwqDSG3g4Nwx5q0GD3H-ne4vwXMuwWAHd"
            encrypted: true
whilefoo commented 2 months ago

I agree your approach looks good but what if someone creates a plugin with the same structure. Maybe we can change encrypted to ubiquityEncrypted?

0x4007 commented 2 months ago

I wanted things to be backwards compatible.

We shouldn't put effort into this until its necessary. We don't have active partners, however now at conferences I've been more focused on meeting prospective partners and investors.

Even if we get active partners (theres a couple interested in the pipeline) we can version lock plugins and things, right? Not sure if we can do versioned deployments of the kernel but that would be the final step to making things totally version-able and we no longer have to worry at all about backwards compatibility.

For us internally I think we should always be running the latest to catch problems faster.

rndquu commented 1 month ago

I agree your approach looks good but what if someone creates a plugin with the same structure. Maybe we can change encrypted to ubiquityEncrypted?

Yes, plugin developer may create a param with the encrypted name.

To sum up there are at least 3 options:

  1. Try to decrypt everything (as described in the issue description)
  2. Use prefix, example:
    plugins:
    issues.closed:
    - uses:
      - plugin: rndquu/conversation-rewards@fix/nonce-generation
        with:
          evmPrivateKey: "_UBIQUITY_ENCRYPTED_PARAM=wOzNgt-yKT6oFlOVz5wrBLUSYxAbKGE9Co-yvT8f9lePsx7wJwPVugS9186zdhr1T4UpkpXvq9ii5M-nWfrydMnllSkowH4LirRZsHbvRVSvDoH_uh80p6HpwqDSG3g4Nwx5q0GD3H-ne4vwXMuwWAHd"
  3. Use a separate param with "unique" name, example:
    plugins:
    issues.closed:
    - uses:
      - plugin: rndquu/conversation-rewards@fix/nonce-generation
        with:
          evmPrivateEncrypted: 
            value: "wOzNgt-yKT6oFlOVz5wrBLUSYxAbKGE9Co-yvT8f9lePsx7wJwPVugS9186zdhr1T4UpkpXvq9ii5M-nWfrydMnllSkowH4LirRZsHbvRVSvDoH_uh80p6HpwqDSG3g4Nwx5q0GD3H-ne4vwXMuwWAHd"
            ubiquityEncrypted: true
0x4007 commented 1 month ago

We could also reserve _ or some other special symbol 🔒 as a prefix for encrypted properties. The emoji isn't very aesthetic but it is functional and expressive.

plugins:
  issues.closed:
    - uses:
      - plugin: rndquu/conversation-rewards@fix/nonce-generation
        with:
          🔒evmPrivateKey: "wOzNgt-yKT6oFlOVz5wrBLUSYxAbKGE9Co-yvT8f9lePsx7wJwPVugS9186zdhr1T4UpkpXvq9ii5M-nWfrydMnllSkowH4LirRZsHbvRVSvDoH_uh80p6HpwqDSG3g4Nwx5q0GD3H-ne4vwXMuwWAHd"

I tested all three while drafting my comment and I think that the lock emoji might be best. It might even be a strength that it renders in color (on macOS it is gold) so that it is easily noticeable.

image

So in conclusion it appears that the task is to read every config property, and if it includes the lock symbol, to attempt to decrypt it.

RFC @rndquu @whilefoo @gentlementlegen

gentlementlegen commented 1 month ago

I would be worried that we'd have issues on encode and decode configurations, I do not know if Unicode characters are properly handled by the library we are using, so it should be tested.

rndquu commented 1 month ago

Although locked emoji does look good I anticipate issues in unexpected places. I think with decrypting sensitive config values we should be as explicit as possible.