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

Added an encryption mechanism for API keys, Fixed the config syncing problem #229

Merged
merged 12 commits into from
Nov 4, 2021
Merged

Conversation

m-javani
Copy link
Contributor

@m-javani m-javani commented Oct 18, 2021

Pull Request Details

This pull request includes solutions for issues #1 and #175.

Two issues are addressed in this PR.

Issue #175

The configs stored in the settings database and the environment key/values were not always in sync.

The reason is that when the application starts, it does not check or pick the environment variables if the application has been initialized previously.

so, because we have multiple sources of truth and do not check the changes in environment variables, the changes won't affect the code. as a workaround, I added a simple logic to compare the values every time the app starts.

note: the changes to the environment variables won't take effect until the next application restart.
solution: the better approach is to remove the API keys from the settings, and use a hot reload config reader package like Viper.

Issue #1

the keys are stored in the database in plain text.

the solution: I added AES Encryption/Decryption utility for encrypting the keys when storing and decrypting them when reading.

note: we need to add a secret key(PROXEUS_ENCRYPTION_SECRET_KEY = 32 characters long string) to the environment variables for this. so the docs should be updated to let the platform maintainer knows about it.

@loleg loleg self-requested a review October 18, 2021 11:55
@loleg
Copy link
Contributor

loleg commented Oct 18, 2021

Thanks for the clean and clear PR. Tests are not passing due to probably just a need to update the go mods. The error due to which test-go is failing is this:

go: downloading github.com/jmespath/go-jmespath v0.4.0
../go/pkg/mod/github.com/huin/[email protected]/goupnp.go:24:2: missing go.sum entry for module providing package golang.org/x/net/html/charset (imported by github.com/onsi/gomega/matchers); to add:
	go get github.com/onsi/gomega/[email protected]
../go/pkg/mod/golang.org/x/[email protected]/acme/autocert/autocert.go:35:2: missing go.sum entry for module providing package golang.org/x/net/idna (imported by golang.org/x/crypto/acme/autocert); to add:
	go get golang.org/x/crypto/acme/[email protected]
../go/pkg/mod/github.com/dop251/[email protected]/string_unicode.go:16:2: missing go.sum entry for module providing package golang.org/x/text/cases (imported by github.com/dop251/goja); to add:
	go get github.com/dop251/[email protected]
../go/pkg/mod/github.com/dop251/[email protected]/builtin_string.go:11:2: missing go.sum entry for module providing package golang.org/x/text/collate (imported by github.com/dop251/goja); to add:
	go get github.com/dop251/[email protected]
../go/pkg/mod/github.com/dop251/[email protected]/builtin_string.go:12:2: missing go.sum entry for module providing package golang.org/x/text/language (imported by github.com/dop251/goja); to add:
	go get github.com/dop251/[email protected]
../go/pkg/mod/github.com/ethereum/[email protected]/accounts/scwallet/securechannel.go:32:2: missing go.sum entry for module providing package golang.org/x/text/unicode/norm (imported by github.com/dop251/goja); to add:
	go get github.com/dop251/[email protected]
make: *** [Makefile:149: test] Error 1

@m-javani
Copy link
Contributor Author

m-javani commented Nov 2, 2021

fixed the dependency and go validation problems. the CircleCI pipeline passed successfully.

Makefile Outdated
@@ -29,6 +29,7 @@ export [email protected]
export PROXEUS_DATA_DIR?=./data
export PROXEUS_DATABASE_ENGINE?=storm
export PROXEUS_DATABASE_URI?=mongodb://localhost:27017
export PROXEUS_ENCRYPTION_SECRET_KEY=734yvc2093dbc2vgdi93ljwwncshhd29
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change to something like "PleAsE_chAnGe_me_32_Characters++" to make it clear that this is only a demo key. Also without ?= it will not get replaced by an environment variable.

@loleg loleg merged commit 2e20bb0 into ProxeusApp:master Nov 4, 2021
@loleg
Copy link
Contributor

loleg commented Nov 4, 2021

We still need to add the new environment variable to documentation, however as tests are passing and this looks now all fine to me, I'm accepting the PR - with many thanks 🎉

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.

2 participants