Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore: ADR for creation and distribution of secrets #141
chore: ADR for creation and distribution of secrets #141
Changes from all commits
ca809e4
e489e41
cedfed9
bb35bc8
b1f94c6
3fd1813
4ebca87
e36916a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an OK posture to take?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an OK position to take if one is actually building a production system based on EdgeX. However, as a middleware it is impossible for EdgeX to know the environment into which it is being deployed, nor does EdgeX mandate an underlying infrastructure. This statement basically declares the host OS as out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added language to describe what these terms mean.
Please elaborate.
Yes, it is a broad problem to solve in a generic way across the industry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The document has a priority list of how to create / distribute secrets and an audit in time of now. Do we need a process in place such that going forward a secret doesn't get hard coded (PR template updated, scheduled audits, tooling to detect passwords being commited etc)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am open to suggestions, but I won't want to propose anything that I'm unwilling to do myself. (e.g. watch every PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative to a file system and something kept in memory with optional persistence, Redis Labs uses Redis instances for this purpose. The runtime overhead of another Redis instance is a couple of MB and storing a few key/value pairs won't change that. Simple shell scripts can return the values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the Redis Labs approach is good, many open source projects can only consume data from files, making it the lowest-common denominator. I will try to fit this into the description of secretsloc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andresrinivasan The way I have explained this is that SECRETSLOC is the equivalent of
169.254.169.254
in the public cloud -- it delivers bootstrapping secrets.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be deprecated since we are using Redis from now on by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to tell what lines were highlighted, but:
Postgres: Kong only works with Postgres and Cassandra not Redis so Postgres has to stay.
MongoDB: We reverted the PR to remove PW generation for MongoDB just in case someone wants to keep using it, so I think this should stay as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this ADR mention that similar support for user-managed secrets should be added to the device SDKs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No opinion. Hard to write about when no design has been proposed. @lenny-intel do you input on the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some naive questions:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
Yes, the backend is Vault. It is not possible to store all secrets in Vault, but as many as possible should be stored in Vault.
As a counter example, the access token needed to connect to Vault itself cannot be stored in Vault. As a fallback mechanism when security is not enabled but a registry is used, secrets will be fetched from Consul. IMHO this is worse than just leaving them on the file system, as Consul is a networked service. However, security disabled in security disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do a survey of existing code? There are quite a few instances of username/password being defined in existing EdgeX services:
https://github.com/edgexfoundry/device-mqtt-go/blob/master/cmd/res/configuration.toml#L60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I am not sure that we should do it RIGHT NOW. Besides, there is thorn in the side of EdgeX--
EDGEX_SECURITY_SECRET_STORE=false
--which if set essentially mandates this practice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be deprecated i guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in Hanoi release, at least. Maybe Ireland.