Skip to content
This repository has been archived by the owner on Dec 26, 2023. It is now read-only.

feat(api): encrypt sensitive variables #669

Closed

Conversation

ismailrei
Copy link

@ismailrei ismailrei commented Dec 14, 2023

Save sensitive variables encrypted in Database

@leg100
Copy link
Owner

leg100 commented Dec 15, 2023

Hello @ismailrei. Thanks for the contribution.

If the concern here is that sensitive variables are encrypted in the database, then I think it is better implemented in the "database layer", i.e. ./internal/variable/db.go. I think this will make the code easier to understand.

Let me know what you think.

@ismailrei
Copy link
Author

Hello @leg100. Thank you for the Answer.
Yes right and i did already think about it :). IMHO, we must encrypt as early as possible and decrypt as late as possible. And moreover, we need to decrypt the values only at the level of the ListEffectiveVariables function. Hence the idea of adding this logic to the service & variable files.

Regards :)

@leg100
Copy link
Owner

leg100 commented Dec 18, 2023

we must encrypt as early as possible and decrypt as late as possible

I appreciate the sentiment, however the complexity this PR introduces is not worth the trade-off. If you can find a succinct approach then by all means do so.

@ismailrei
Copy link
Author

For me implementing the decryption process in the db.go file can be counterproductive in terms of performance, this is why I did it in the ListEffectiveVariable function only used by the agents :)

In your code you hide the sensitive value when you call the ListVariables and ListVariablesSet functions:

https://github.com/leg100/otf/blob/master/internal/variable/tfe.go#L483-L485

If we decrypt the sensitive variables each time these functions are called, we’ll decrease the performance even though these sensitive data are not used.

BTW passing secured data in clear from function to function is not really recommended as it can possibly be displayed somewhere by inadvertence (logs, sentry errors…).

What do you think about that? I'm open to hearing if you have other suggestions 🙂

@leg100 leg100 closed this Dec 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants