kuzzleio / kuzzle-vault

Securely share secrets for your application with JSON and cryptography
Apache License 2.0
7 stars 2 forks source link

Objects containing numbers or boolean properties are not encrypted #15

Closed itishermann closed 1 year ago

itishermann commented 2 years ago

Expected Behavior

All the properties of all types of objects to be encrypted should be encrypted.

Current Behavior

If a property of the object to be encrypted is not a string, the property doest not get encrypted and it's not added to the encrypted object.

Possible Solution

In the function encryptObject, the function should check if the property is something else than a string. if it is, cast the property to string and add the type at the end of the string. This way, the property will be encrypted as a string and when decrypting, the type can be restored.

For example:

// Before encryption
{
  "name": "John Cena",
  "age": 30,
  "active": true
}

// What gets encrypted
{
  "name": "John Cena",
  "age": "30+number",
  "active": "true+boolean"
}

So the encryptObject would look like this:

encryptObject (secrets: any): any {
  const encryptedSecrets: any = {};

  for (const key of Object.keys(secrets)) {
    const value: string|any = secrets[key];

    if (value && typeof value === 'object' && !Array.isArray(value)) {
      encryptedSecrets[key] = this.encryptObject(value);
    }
    else if (typeof value === 'string') {
      encryptedSecrets[key] = this.encryptString(value);
    } 
    else if (['boolean','number'].includes(typeof value)) {
      const toBeEncrypted = `${value.toString()}+${typeof value}`;
      encryptedSecrets[key] = this.encryptString(toBeEncrypted);
    }
  }

  return encryptedSecrets;
}

ant the decyptString would look like this:

decryptString (encrypted: string): string {
  const [ encryptedData, ivHex ] = encrypted.split('.');

  if (encryptedData.length === 0) {
    throw new Error(`Invalid encrypted string format "${encryptedData}.${ivHex}"`);
  }

  if (ivHex.length !== 32) {
    throw new Error(`Invalid IV size. (${ivHex.length}, expected 32)`);
  }

  const iv = Buffer.from(ivHex, 'hex');
  const decipher = crypto.createDecipheriv('aes-256-cbc', this.vaultKeyHash, iv);

  try {
    const deciphered: string = decipher.update(encryptedData, 'hex', 'utf8') + decipher.final('utf8');

    const splited = deciphered.split('+');

    if(splited.length > 1){

      const originalType = splited.pop();

      if(originalType === 'number'){
        return Number(splited.join('+'))
      };

      if(originalType === 'boolean'){
        return Boolean(splited.join('+'))
      };

    }

    return deciphered;

  }
  catch (error) {
    if (error.message.includes('bad decrypt')) {
      throw new Error('Cannot decrypt encrypted value with the provided key');
    }

    throw new Error(`Encrypted input value format is not a valid: ${error.message}`);
  }
}

Steps to Reproduce

  1. Create a secret.json file containg the following content:
    {
    "name": "John Cena",
    "age": 30,
    "active": true
    }
  2. run kourou vault:encrypt secrets.json --vault-key password

Context (Environment)

Kuzzle version: 2.18.1 Node.js version: 16.14.2 SDK version: - Kourous version: 0.22.0

Shiranuit commented 2 years ago

Hi, it is an intended behaviour for the method encryptObject to only encrypt keys that have a string value as described in the comment of the function encryptObject.

Since it's not a bug I will remove the bug tag from this issue and replace it with enhancement, but encrypting boolean might not be useful since there is only two values true or false making it easy to guess and test, as for the numbers maybe it might be useful in some cases but I can't see which ones right now, but an easy work around is simply to make it a string and do a parseInt in your application for now.

Since there is an easy work around and no real use case for now it's not a priority for us to change this behaviour right now, but feel free to make a Pull Request 🙂

Aschen commented 2 years ago

@itishermann I'm not convinced about having the type in the string (e.g. true+boolean) because then it will became complicated to handle string with+ inside.

The Vault is meant to be used to encrypt secrets like authentication credentials and those secrets are mainly strings. You can have numbers as secrets (e.g. RSA keys are big prime numbers) but usually those numbers are represented as a base 64 string.

If you really want to encrypt something different than a string then you could add the type in the encrypted string;

For example: f700cac98100f1266536553f3181ada6.65dfa691071a81f3214be3836bbb9fa1.integer

To avoid breaking change, encrypted string with only two part (actual representation) should be considered as string