Skip to content

Conversation

@nicklaswj
Copy link

This PR adds the methods EncryptedTable::init_headless_with_zk_config and EncryptedTable::init_with_zk_config, to make it possible to initialize a EncryptedTable with a custom ZeroKmsConfig.

We need this since we load the cipherstash credentials/client_key externally and wont have them in the environment.

@nicklaswj nicklaswj requested a review from coderdan November 7, 2024 13:37
Self::init_headless_with_zk_config(zerokms_config).await
}

pub async fn init_headless_with_zk_config(
Copy link
Contributor

@CDThomas CDThomas Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion: mind spelling out zerokms here instead of abbreviating? This is more consistent with other areas of the code.

Suggested change
pub async fn init_headless_with_zk_config(
pub async fn init_headless_with_zerokms_config(

})
}

pub async fn init_with_zk_config(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same suggestion here: mind spelling out zerokms instead of abbreviating?

Suggested change
pub async fn init_with_zk_config(
pub async fn init_with_zerokms_config(

Copy link
Contributor

@CDThomas CDThomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nicklaswj. I left a couple of minor comments around naming, but this looks good to me otherwise.

We need this since we load the cipherstash credentials/client_key externally and wont have them in the environment.

Just to clarify, the use case is to avoid keeping secrets in the environment and load them from somewhere like AWS Secrets Manager? This looks very reasonable to me. Considering that it's best practice to keep secrets out of the environment in AWS Lambda in particular, maybe it's worth noting this as the preferred approach in our documentation as well.

What do you think @coderdan?

@nicklaswj
Copy link
Author

Thanks @nicklaswj. I left a couple of minor comments around naming, but this looks good to me otherwise.

Fixed.

Just to clarify, the use case is to avoid keeping secrets in the environment and load them from somewhere like AWS Secrets Manager?

It's a bit more complex than that. But basically we have a AWS authorizer lambda that retrieves different cipherstash credentials depending on who is logged in.

@nicklaswj nicklaswj force-pushed the init-with-config branch 3 times, most recently from 4cb62b4 to dc8f7b0 Compare November 11, 2024 09:14
@nicklaswj
Copy link
Author

I have rebased and signed the commits

@nicklaswj
Copy link
Author

Even though I can, I don't really feel comfortable just pressing the merge button. Can I get a confirmation that this PR can be merged ?


impl EncryptedTable<Headless> {
pub async fn init_headless() -> Result<Self, InitError> {
info!("Initializing...");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this was removed?

@coderdan coderdan merged commit e6192db into main Nov 12, 2024
3 checks passed
@coderdan coderdan deleted the init-with-config branch November 12, 2024 22:40
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

Successfully merging this pull request may close these issues.

4 participants