Introduce granular etcd permissions to access KVstoreMesh cached data #33082
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, the same etcd user (i.e., remote) is granted permissions to read the whole content of the clustermesh-apiserver's sidecar etcd instance, including also the data cached by kvstoremesh, when enabled. In an effort to harden the overall clustermesh posture, let's introduce a separate and dedicated user for local access, to ensure that remote clusters cannot access cached data, as it may include information that they would not normally have access to. Specifically, the remote user is intended to have access only to the information regarding the local cluster, while the local user can access cached data about remote clusters only. Still, for backward compatibility purposes, the remote user still retains access to cached data as well in this release. The reason being that there would otherwise be a time window upon upgrade in which Cilium Agents would lose access to the kvstoremesh data (especially in large clusters). Indeed, the new certificate would be mounted by the agents only upon rollout, but the configuration would be immediately reloaded (thus targeting the new, not yet mounted, certificate), hence breaking the access to the information cached by kvstoremesh. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Extend the helm chart to additionally generate the "local" certificate with the common name matching the newly introduced "local" etcd user, when kvstoremesh is enabled. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Let's additionally mount the kvstoremesh-specific certificate into cilium agents, so that it can be used to authenticate against the local etcd instance storing the cached data. The secret entry is always configured (although marked as optional), regardless of whether KVStoreMesh is actually enabled or not, so that it can be automatically mounted in case it gets subsequently enabled, without requiring a restart of the agents. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
9d2f5dd to
461da7f
Compare
|
/test |
|
ClusterMesh related workflows are green. Dropping the last commit, which will be introduced in v1.17 only. |
461da7f to
fecfd7b
Compare
|
/test |
1 similar comment
|
/test |
aanm
approved these changes
Jun 13, 2024
sayboras
approved these changes
Jun 13, 2024
marseel
reviewed
Jun 13, 2024
| rangeForKey(kvstore.HasClusterConfigPath), | ||
| rangeForPrefix(kvstore.CachePrefix), | ||
| rangeForPrefix(kvstore.ClusterConfigPrefix), | ||
| rangeForPrefix(kvstore.SyncedPrefix), |
There was a problem hiding this comment.
I would probably fix "Prefixes" so they are actually prefixes with "/" at the end, while "Path" are actual keys in Etcd (so I would change variable naming to Key)
For example, instead of:
SyncedPrefix = BaseKeyPrefix + "/synced"
have
SyncedPrefix = BaseKeyPrefix + "/synced/"
Then we could have rangeForKey function, without having rangeForPrefix, which is a bit confusing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently, the same etcd user (i.e., remote) is granted permissions to read the whole content of the clustermesh-apiserver's sidecar etcd instance, including also the data cached by kvstoremesh, when enabled. In an effort to harden the overall clustermesh posture, let's introduce a separate and dedicated user for local access, to ensure that remote clusters cannot access cached data, as it may include information that they would not normally have access to.
Specifically, the remote user is intended to have access only to the information regarding the local cluster, while the local user can access cached data about remote clusters only. Still, for backward compatibility purposes, the remote user still retains access to cached data as well in this release. The reason being that there would otherwise be a time window upon upgrade in which Cilium Agents would lose access to the kvstoremesh data (especially in large clusters). Indeed, the new certificate would be mounted by the agents only upon rollout, but the configuration would be immediately reloaded (thus targeting the new, not yet mounted, certificate), hence breaking the access to the information cached by kvstoremesh.
Please review commit by commit, and refer to the individual descriptions for additional details.