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

Decrypt sensitive config parameters #90

Open
rndquu opened this issue Aug 15, 2024 · 13 comments
Open

Decrypt sensitive config parameters #90

rndquu opened this issue Aug 15, 2024 · 13 comments

Comments

@rndquu
Copy link
Member

rndquu commented Aug 15, 2024

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
Copy link
Member Author

rndquu commented Aug 15, 2024

@gentlementlegen Pls check the description

@0x4007
Copy link
Member

0x4007 commented Aug 16, 2024

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
Copy link
Member Author

rndquu commented Aug 16, 2024

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.

ubiquity/onboard.ubq.fi#22

@whilefoo
Copy link
Contributor

whilefoo commented Sep 5, 2024

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

@rndquu
Copy link
Member Author

rndquu commented Sep 6, 2024

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
Copy link
Contributor

whilefoo commented Sep 6, 2024

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
Copy link
Member Author

rndquu commented Sep 13, 2024

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
Copy link
Contributor

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
Copy link
Member

0x4007 commented Sep 13, 2024

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
Copy link
Member Author

rndquu commented Sep 18, 2024

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"
  1. 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
Copy link
Member

0x4007 commented Sep 30, 2024

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
Copy link
Member

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
Copy link
Member Author

rndquu commented Sep 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants