feat(engine): implement hashicorp vault engine #29

akhildevelops · 2024-10-15T04:05:58Z

This PR provides support to connect to Hashicorp Vault as backend.

Features:

Setup Vault as docker container for development.
Feature Flags for connecting to different backends: aes, aws, vault.
Connect to Vault for generating keys, encrypt, decrypt data.
Get DB Error messages instead of overriding with DatabaseError::Others variant

P.S: cargo run will fail as feature flags aren't provided. Instead run with either cargo run --features=<aes/aws/vault>

Fixes: juspay/hyperswitch#6226

…ce into vault

dracarys18

@akhildevelops Please mention me once the comments are resolved and encryption/decryption part is implemented for Data encryption

Cargo.toml

.gitignore

dracarys18 · 2024-10-15T09:22:03Z

docker-compose.yml

If we are creating a docker-compose file we need to create a fully fledged one

Is this what you're expecting https://github.com/juspay/hyperswitch-encryption-service/blob/f4ee38855ac85a72e1047e85b2279ec9afa655da/docker/development/docker-compose.yml

src/config.rs

dracarys18 · 2024-10-15T09:25:26Z

src/config.rs

+        {
+            // This function doesn't return result therefore unwrapping.
+            // TODO: Make it customresult return type
+            let client = init_vault(self.vault_config, self.vault_token).unwrap();


The panic part can happen inside the init_vault function

I see there are many expect methods used through out the code that can also panic. Rewired unwrap to expect.

src/crypto.rs

src/crypto/vault.rs

dracarys18 · 2024-10-15T09:31:05Z

src/errors/app.rs

-    InternalServerError(&'static str),
+    #[error("Internal Server Error Occurred - {prefix} {}",message.as_ref().unwrap_or(&"".to_string()))]
+    InternalServerError {
+        prefix: &'static str,


What is prefix here? I think we can just use message alone

I observe there are many static lifetime strings used for error messages. I can simply transform it to String type but every time memory will be allocated while creating error messages at different code points.

Can we just use the message?

akhildevelops · 2024-10-16T07:23:04Z

Cool, @dracarys18 will ping you oince changes are implemented.

akhildevelops · 2024-10-16T12:22:34Z

Hi @dracarys18, I'm done with my changes and also fixes #30

I might not be very active for next couple of days. I'll try to respond for any comments as and when I'm free.

akhildevelops · 2024-10-16T13:20:54Z

Setup vault, postgres and run migrations through

docker compose --file docker/development/docker-compose.yml --profile vault_only up

Run server with vault as backend through cargo run --no-default-features --features=vault
and running with AES backend cargo run

dracarys18 · 2024-10-22T11:59:09Z

@akhildevelops Hey why do we need changes in CI?

akhildevelops · 2024-10-22T14:00:22Z

This PR fails with current ci configuration for cargo check --all-features --all-targets variant because there will be a clash in creating higher level types/objects (Ex: KeyManagerClient type fails).

➜  hyperswitch-encryption-service git:(vault) cargo hack check --all-features --all-targets
info: running `cargo check --all-features --all-targets` on cripta (1/1)
   Compiling cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
error[E0428]: the name `KeyManagerClient` is defined multiple times
   --> src/crypto.rs:143:1
    |
140 | pub type KeyManagerClient = EncryptionClient<AwsKmsClient>;
    | ----------------------------------------------------------- previous definition of the type `KeyManagerClient` here
...
143 | pub type KeyManagerClient = EncryptionClient<GcmAes256>;
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `KeyManagerClient` redefined here
    |
    = note: `KeyManagerClient` must be defined only once in the type namespace of this module

As it will open up for all features

#[cfg(feature = "aws")]
pub type KeyManagerClient = EncryptionClient<AwsKmsClient>;


#[cfg(feature = "aes")]
pub type KeyManagerClient = EncryptionClient<GcmAes256>;


#[cfg(feature = "vault")]
pub type KeyManagerClient = EncryptionClient<Vault>;

https://github.com/akhildevelops/hyperswitch-encryption-service/blob/21aa1967c856215dcdf69ba0067dddd8c4132254/src/crypto.rs#L139-L145

It isn't a problem in the current main:63c6794 because of simple aws or !aws.

dracarys18 · 2024-10-22T15:09:25Z

@akhildevelops You can try something like conditional compilation

#[cfg(all(feature = "aws", not(feature = "aes")))]
pub type KeyManagerClient = EncryptionClient<AwsKmsClient>;

akhildevelops · 2024-10-22T15:19:57Z

This will be bloated as and when multiple backends are added

#[cfg(all(feature = "aws", not(feature = "aes"),not(feature = "vault")))]
pub type KeyManagerClient = EncryptionClient<AwsKmsClient>;

dracarys18 · 2024-10-22T16:22:31Z

Really don't wanna do dynamic dispatch but we can do this for readability. Change the current EncryptionClient<T> to this

type Backend = dyn KeyManagement + Send + Sync;

pub struct KeyManagerClient {
    client: Arc<Backend>,
}

impl KeyManagerClient {
    pub fn new(client: Arc<Backend>) -> Self {
        Self { client }
    }
}

impl KeyManagerClient {
    pub fn client(&self) -> &Arc<Backend> {
        &self.client
    }
}

impl Deref for KeyManagerClient {
    type Target = Arc<Backend>;
    fn deref(&self) -> &Self::Target {
        self.client()
    }
}

and this in config.rs

impl Secrets {
    pub async fn create_keymanager_client(self) -> KeyManagerClient {
        #[cfg(feature = "aws")]
        {
            let client = AwsKmsClient::new(&self.kms_config).await;
            EncryptionClient::new(client)
        }

        #[cfg(not(feature = "aws"))]
        {
            let client = self.master_key;
            KeyManagerClient::new(std::sync::Arc::new(client))
        }
    }
}

gorakhnathy7 · 2024-10-24T10:44:27Z

Hey @akhildevelops Kindly make the changes so that the PR can be merged within time.

akhildevelops · 2024-10-24T14:08:23Z

Even with Dynamic trait objects how can the application decide to choose between vault or aes ? if not(feature="aws") is the only way to decide on aes or vault..

// Is activated for both vault and aes
// Will not work if vault is the backend
#[cfg(not(feature = "aws"))]
        {
            let client = self.master_key;
            KeyManagerClient::new(std::sync::Arc::new(client))
        }

I would ask you to check the ci.yml that doesn't have any harcoded way to run compilation checks for different backends.

Below is the cargo hack command runs checks for each individual backend and every compilation check is successful

➜  hyperswitch-encryption-service git:(vault) cargo hack check --each-feature --exclude-no-default-features --exclude-all-features --exclude-features mtls --all-targets
info: running `cargo check --all-targets --no-default-features --features aes` on cripta (1/5)
    Checking cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.78s

info: running `cargo check --all-targets --no-default-features --features aws` on cripta (2/5)
    Checking cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.91s

info: running `cargo check --all-targets --no-default-features --features default` on cripta (3/5)
    Checking cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.33s

info: running `cargo check --all-targets --no-default-features --features release` on cripta (4/5)
    Checking cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.91s

info: running `cargo check --all-targets --no-default-features --features vault` on cripta (5/5)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.18s
###########################

➜  hyperswitch-encryption-service git:(vault) cargo hack clippy --each-feature --exclude-no-default-features --exclude-all-features --exclude-features mtls --all-targets
info: running `cargo clippy --all-targets --no-default-features --features aes` on cripta (1/5)
   Compiling cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.09s

info: running `cargo clippy --all-targets --no-default-features --features aws` on cripta (2/5)
   Compiling cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.82s

info: running `cargo clippy --all-targets --no-default-features --features default` on cripta (3/5)
   Compiling cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.81s

info: running `cargo clippy --all-targets --no-default-features --features release` on cripta (4/5)
   Compiling cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.18s

info: running `cargo clippy --all-targets --no-default-features --features vault` on cripta (5/5)
   Compiling cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.77s

dracarys18 · 2024-10-25T05:21:28Z

Even with Dynamic trait objects how can the application decide to choose between vault or aes ? if not(feature="aws") is the only way to decide on aes or vault..

// Is activated for both vault and aes
// Will not work if vault is the backend
#[cfg(not(feature = "aws"))]
        {
            let client = self.master_key;
            KeyManagerClient::new(std::sync::Arc::new(client))
        }

I would ask you to check the ci.yml that doesn't have any harcoded way to run compilation checks for different backends.

Below is the cargo hack command runs checks for each individual backend and every compilation check is successful

➜  hyperswitch-encryption-service git:(vault) cargo hack check --each-feature --exclude-no-default-features --exclude-all-features --exclude-features mtls --all-targets
info: running `cargo check --all-targets --no-default-features --features aes` on cripta (1/5)
    Checking cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.78s

info: running `cargo check --all-targets --no-default-features --features aws` on cripta (2/5)
    Checking cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.91s

info: running `cargo check --all-targets --no-default-features --features default` on cripta (3/5)
    Checking cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.33s

info: running `cargo check --all-targets --no-default-features --features release` on cripta (4/5)
    Checking cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.91s

info: running `cargo check --all-targets --no-default-features --features vault` on cripta (5/5)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.18s
###########################

➜  hyperswitch-encryption-service git:(vault) cargo hack clippy --each-feature --exclude-no-default-features --exclude-all-features --exclude-features mtls --all-targets
info: running `cargo clippy --all-targets --no-default-features --features aes` on cripta (1/5)
   Compiling cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.09s

info: running `cargo clippy --all-targets --no-default-features --features aws` on cripta (2/5)
   Compiling cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.82s

info: running `cargo clippy --all-targets --no-default-features --features default` on cripta (3/5)
   Compiling cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.81s

info: running `cargo clippy --all-targets --no-default-features --features release` on cripta (4/5)
   Compiling cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.18s

info: running `cargo clippy --all-targets --no-default-features --features vault` on cripta (5/5)
   Compiling cripta v0.1.0 (/home/akhil/practice/hyperswitch-encryption-service)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 3.77s

With dynamic dispatch we don't have to write that line anymore. @akhildevelops If you are not able to do the change let me know I will do it

akhildevelops · 2024-10-25T15:10:40Z

@dracarys18 please go ahead

dracarys18 · 2024-10-29T10:37:57Z

Hi @akhildevelops I have made the changes I will thoroughly test this once and merge this PR! Thanks a lot for the contribution!!

akhildevelops · 2024-10-29T16:12:05Z

@dracarys18 Thanks for the appreciation!!

If I'm correct, you're trying to avoid compilation complexities by including all features and choose feature during runtime. I have a suggestions, please discard if it doesn't help or you're ok with current design:

Remove features and pass backend as argument to binary i.e, cargo run -- --backend aws and then initialise KeyManagerClient.

This pattern helps to run any backend without recompilation considering current design of trait objects.

dracarys18 · 2024-10-29T16:22:27Z

@akhildevelops So once you compile for AWS it doesn't make sense to switch to vault at runtime, Because all the keys would be encrypted with AWS, That's why I kept it separate.

dracarys18 · 2024-11-12T06:17:58Z

I have removed the README.md changes since we will add another SETUP.md to add the documentation for the setup

dracarys18

PR looks good to me! Thanks for the PR @akhildevelops , great work!

ArjunKarthik · 2024-11-12T06:34:08Z

src/config.rs

+                None,
+            )
+            .await
+            .expect("Failed why decrypted vault encrypted secret")


"Failed while decrypting vault encrypted secret"

ArjunKarthik

LGTM

akhildevelops added 7 commits October 12, 2024 11:00

services for development

0b9272a

feature to aes

5760ca9

vault

a709933

Send back db error context to application

11a6e05

fixes for aes

f235c19

to run with aes

d84da02

Merge branch 'main' of github.com:juspay/hyperswitch-encryption-servi…

4dda651

…ce into vault

akhildevelops mentioned this pull request Oct 15, 2024

[FEAT] add support for Hashicorp Vault Engine juspay/hyperswitch#6226

Closed

dracarys18 changed the title ~~Hashicorp Vault as Backend to Cripta~~ feat(engine): implement hashicorp vault engine Oct 15, 2024

dracarys18 suggested changes Oct 15, 2024

View reviewed changes

gorakhnathy7 assigned akhildevelops Oct 15, 2024

gorakhnathy7 added the Hacktoberfest label Oct 15, 2024

gorakhnathy7 requested a review from NishantJoshi00 October 15, 2024 22:40

akhildevelops added 6 commits October 16, 2024 06:40

skip formatting

9413220

push them to hosts global gitignore file

f460b57

default feature to aes and readme update

2bd7ade

setup all services in docker compose

171b78b

docker compose comments

dbfac62

initialize vault

f4ee388

akhildevelops added 3 commits October 16, 2024 16:01

Errors propgate directly from vault

c3e4350

vault_only profile sets up services except the cript service

841f691

remove spaces

93ce106

akhildevelops mentioned this pull request Oct 16, 2024

Data Key versioning isn't working for multiple create requests #30

Closed

Increment version

02c6f6d

akhildevelops added 3 commits October 16, 2024 17:58

small adjustments

6d2eca0

default to zero as it gets incremented later

b144ee8

fixes versioning

ecc49b6

gorakhnathy7 requested a review from dracarys18 October 22, 2024 11:48

all-targets

fd1bc56

dracarys18 added 4 commits October 29, 2024 15:52

feat: dynamically enable encryption backend

f491887

fix: dont change ci

bfc5d8e

fix: clippy

a4a52e8

chore:fmt

0d3a305

gorakhnathy7 added the hacktoberfest-accepted label Oct 31, 2024

feat: cleanups

a5561ae

dracarys18 previously approved these changes Nov 12, 2024

View reviewed changes

dracarys18 requested review from ArjunKarthik and removed request for NishantJoshi00 November 12, 2024 06:19

ArjunKarthik reviewed Nov 12, 2024

View reviewed changes

fix: typo

e66a3e1

dracarys18 dismissed their stale review via e66a3e1 November 12, 2024 06:48

ArjunKarthik approved these changes Nov 12, 2024

View reviewed changes

dracarys18 approved these changes Nov 12, 2024

View reviewed changes

dracarys18 merged commit b54bf8f into juspay:main Nov 12, 2024
2 checks passed

feat(engine): implement hashicorp vault engine #29

feat(engine): implement hashicorp vault engine #29

Uh oh!

Conversation

akhildevelops commented Oct 15, 2024

Uh oh!

dracarys18 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhildevelops commented Oct 16, 2024

Uh oh!

akhildevelops commented Oct 16, 2024

Uh oh!

akhildevelops commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dracarys18 commented Oct 22, 2024

Uh oh!

akhildevelops commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dracarys18 commented Oct 22, 2024

Uh oh!

akhildevelops commented Oct 22, 2024

Uh oh!

dracarys18 commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gorakhnathy7 commented Oct 24, 2024

Uh oh!

akhildevelops commented Oct 24, 2024

Uh oh!

dracarys18 commented Oct 25, 2024

Uh oh!

akhildevelops commented Oct 25, 2024

Uh oh!

dracarys18 commented Oct 29, 2024

Uh oh!

akhildevelops commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dracarys18 commented Oct 29, 2024

Uh oh!

dracarys18 commented Nov 12, 2024

Uh oh!

dracarys18 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArjunKarthik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akhildevelops commented Oct 16, 2024 •

edited

Loading

akhildevelops commented Oct 22, 2024 •

edited

Loading

dracarys18 commented Oct 22, 2024 •

edited

Loading

akhildevelops commented Oct 29, 2024 •

edited

Loading