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

Key Rotation breaks appending new values to existing sealed secrets #185

Closed
chrisharm opened this issue Jul 16, 2019 · 4 comments
Closed
Assignees
Labels
Milestone

Comments

@chrisharm
Copy link
Contributor

With new new key rotation feature introduced in #143, the ability to append new values to an existing sealed secret is broken. Each time the controller restarts, a new key is generated and set as the active key. It looks like the unseal process expects all of the keys in a SealedSecret object to be encrypted using the same key.

Steps to Reproduce:

  1. Encrypt foo=bar:
    kubectl create secret generic mysecret --dry-run --from-literal=foo=bar -o yaml | kubeseal --format yaml

Output:

apiVersion: bitnami.com/v1alpha1
kind: SealedSecret
metadata:
  creationTimestamp: null
  name: mysecret
  namespace: default
spec:
  encryptedData:
    foo: AgDSe+W6X+6Tk5WfwmMPFrlspMNOypWGY7uNX2JoRlC5/lGieYlUm0LTuFmTXZXwC+YzpjixHDjTkLGIgXw2uQj/BFZAae6rTrP78auQKZWA6FvQAdfPrX7PL9vNCJrlHLCcMf+03giJp5N9fDnjJf9Apxof+YFU6n4dy7EBn0AhIBJ0Zk7cJ30BHT4qUdrNf9W0u9ME4CmeaR7kwj7iGBlstNvxvd8yGO9g608jhclPzkugG30BvbkzeKqyuDTH0qUlFf5aGCZtHI6zZhz0GOLLkKLwZEHM/BkOsydm4jYJPXFatU5uFwXpsof+PfqWSRuhH/gqDcfYNjRPXR2CuFC7Tiy7WOdPTqINmw1cu26yHR6wsDRY+6o7kjHdfn9zB9UGhWBBiItU9vMuwhvte4yBkAvMVH67CYUUAgduCHpE870mVupgS1xyze2gVOTxpoG4x3OmGrGThHWQwnSIHcgB8dJKXiFudOT8B+U6PqOjOPpyyGjnC0WeM99Ets56TUNIRpQ7M1m5clp7Zz+8MltqdO4KgHph6H2hNfN0tLwohhcodZE+qhjYSSzDqA/3sjU/xUASBJfQH8in4WhRi0hk9toWjBb0pMex1eeYTNcM/LMvsOjRnFUqqQy/GpBXx4B78HEZ/tNBXwR69Jdw5E2B2B1eUFRu1eYD9lviGcZjhzCWDu1W3Q0NMqImYZqIRPYf6T0=
  1. Restart sealed-secret-controller pod:
    kubectl delete pod --namespace kube-system --selector=name=sealed-secrets-controller

  2. Generate a new secret for foo2=bar2
    kubectl create secret generic mysecret --dry-run --from-literal=foo2=bar2 -o yaml | kubeseal --format yaml

Output:

apiVersion: bitnami.com/v1alpha1
kind: SealedSecret
metadata:
  creationTimestamp: null
  name: mysecret
  namespace: default
spec:
  encryptedData:
    foo2: AgBuE1IsRoIJwjuLIp0w15Y1016ZU7KMgptdpgObNGJhmvPw8b6zshBhJ8N48eP/M6FHLPFhqPMj9ZR14tgA2aCjufLc6xBfAoQ9OGVZqepMQWI4jkjcJGtSlEZKy0FMpEaR0oF+cr/YCa7aiWBbdhjT+nBQLPxStH7L1RYJ5gQt/9tGiyCEJes8QcDLh5Fa78WLkMnaxcLO2JvhAh84vpdVea8dqA5U3l0bol0cN5pTZe+21xQZw6LXrvNdy+sGoyT0ytoOJx6xZdQQjz+EWUb+LgfLD2ddGY4SSHxUOTgZ9t3UvCpx6R4I2Gfe9nO2X7irvjZ2yEuBYMTxanGrlAoWlGXmVmC5U8t2BtVdEhnJ0BQsT2JbaTmXuiiggWzz6teyOKIne8Cxp5tojW+yNxEFO0Z0nXqnbVWpXoXy3dRExlAyHUYHr1NZ/b9lQpBuO64N+3KP/xWaM0RiB7brA46vAiCdgdKIGIxPcDUEPEuaw2yEN8mTt34nixGsIW6VWCJy47TAYoS3JZt9kjqggVbHdim0/f3O+ghGsg1X5FrXjyD4cfq8SIWXLFmqpiHz6zCng81Dl/atWDkRJJ8J+jn8SNugfikxCPCWuLsA2u5wf+k1U+kFKu3FGMFxjGA74HAP/9P3qbq63NbmCgknP+M5cyWZTCtLhRlIySPe3gxdbBdfysA8x5Pvqjt8oyyAyGlu0juT
  1. Append foo2 to original secret:
apiVersion: bitnami.com/v1alpha1
kind: SealedSecret
metadata:
  creationTimestamp: null
  name: mysecret
  namespace: default
spec:
  encryptedData:
    foo: AgDSe+W6X+6Tk5WfwmMPFrlspMNOypWGY7uNX2JoRlC5/lGieYlUm0LTuFmTXZXwC+YzpjixHDjTkLGIgXw2uQj/BFZAae6rTrP78auQKZWA6FvQAdfPrX7PL9vNCJrlHLCcMf+03giJp5N9fDnjJf9Apxof+YFU6n4dy7EBn0AhIBJ0Zk7cJ30BHT4qUdrNf9W0u9ME4CmeaR7kwj7iGBlstNvxvd8yGO9g608jhclPzkugG30BvbkzeKqyuDTH0qUlFf5aGCZtHI6zZhz0GOLLkKLwZEHM/BkOsydm4jYJPXFatU5uFwXpsof+PfqWSRuhH/gqDcfYNjRPXR2CuFC7Tiy7WOdPTqINmw1cu26yHR6wsDRY+6o7kjHdfn9zB9UGhWBBiItU9vMuwhvte4yBkAvMVH67CYUUAgduCHpE870mVupgS1xyze2gVOTxpoG4x3OmGrGThHWQwnSIHcgB8dJKXiFudOT8B+U6PqOjOPpyyGjnC0WeM99Ets56TUNIRpQ7M1m5clp7Zz+8MltqdO4KgHph6H2hNfN0tLwohhcodZE+qhjYSSzDqA/3sjU/xUASBJfQH8in4WhRi0hk9toWjBb0pMex1eeYTNcM/LMvsOjRnFUqqQy/GpBXx4B78HEZ/tNBXwR69Jdw5E2B2B1eUFRu1eYD9lviGcZjhzCWDu1W3Q0NMqImYZqIRPYf6T0=
    foo2: AgBuE1IsRoIJwjuLIp0w15Y1016ZU7KMgptdpgObNGJhmvPw8b6zshBhJ8N48eP/M6FHLPFhqPMj9ZR14tgA2aCjufLc6xBfAoQ9OGVZqepMQWI4jkjcJGtSlEZKy0FMpEaR0oF+cr/YCa7aiWBbdhjT+nBQLPxStH7L1RYJ5gQt/9tGiyCEJes8QcDLh5Fa78WLkMnaxcLO2JvhAh84vpdVea8dqA5U3l0bol0cN5pTZe+21xQZw6LXrvNdy+sGoyT0ytoOJx6xZdQQjz+EWUb+LgfLD2ddGY4SSHxUOTgZ9t3UvCpx6R4I2Gfe9nO2X7irvjZ2yEuBYMTxanGrlAoWlGXmVmC5U8t2BtVdEhnJ0BQsT2JbaTmXuiiggWzz6teyOKIne8Cxp5tojW+yNxEFO0Z0nXqnbVWpXoXy3dRExlAyHUYHr1NZ/b9lQpBuO64N+3KP/xWaM0RiB7brA46vAiCdgdKIGIxPcDUEPEuaw2yEN8mTt34nixGsIW6VWCJy47TAYoS3JZt9kjqggVbHdim0/f3O+ghGsg1X5FrXjyD4cfq8SIWXLFmqpiHz6zCng81Dl/atWDkRJJ8J+jn8SNugfikxCPCWuLsA2u5wf+k1U+kFKu3FGMFxjGA74HAP/9P3qbq63NbmCgknP+M5cyWZTCtLhRlIySPe3gxdbBdfysA8x5Pvqjt8oyyAyGlu0juT
  1. Apply the sealed secret to the cluster
    kubectl apply -f /tmp/ss.yaml

Result:

2019/07/16 16:47:23 Error updating default/new-keys-ss, giving up: No key could decrypt secret
E0716 16:47:23.374767       1 controller.go:191] No key could decrypt secret
2019/07/16 16:47:23 Event(v1.ObjectReference{Kind:"SealedSecret", Namespace:"default", Name:"new-keys-ss", UID:"ce8e2aa5-9887-4581-a796-c9d610be5b96", APIVersion:"bitnami.com/v1alpha1", ResourceVersion:"175933", FieldPath:""}): type: 'Warning' reason: 'ErrUnsealFailed' Failed to unseal: No key could decrypt secret
@chrisharm
Copy link
Contributor Author

See #162 and #95

mkmik pushed a commit that referenced this issue Jul 17, 2019
It's premature to enable key-rotation by default. See open issues like #185.
Ref #137

This should unblock the possibility of cutting a release with a lot of important features
and let users who want key-rotation to opt-in and deal with the consequences.
bors bot added a commit that referenced this issue Jul 17, 2019
187: Disable key rotation by default r=mkmik a=mkmik

It's premature to enable key-rotation by default. See open issues like #185.
Ref #137

This should unblock the possibility of cutting a release with a lot of important features
and let users who want key-rotation to opt-in and deal with the consequences.

Co-authored-by: Marko Mikulicic <[email protected]>
@mkmik mkmik self-assigned this Jul 19, 2019
@mkmik mkmik modified the milestones: v0.8.0, v0.9.0 Jul 19, 2019
@mkmik mkmik added the bug label Jul 19, 2019
This was referenced Jul 19, 2019
@grossvogel
Copy link

grossvogel commented Aug 2, 2019

I'm sure many are far ahead of me on this, but I think a good solution to this would be to store a unique key id along with each encrypted value in the SealedSecret. That way values encrypted by different keys could co-exist on the same SealedSecret and git diffs would be clean, indicating exactly which data fields are added or updated. I think this is similar to how openid connect handles key rotation.

@mkmik
Copy link
Collaborator

mkmik commented Aug 3, 2019

I also think that's an efficient approach. I'd just like to make sure it's done in a backward compatible way.

Currently all keys are tried in turn until one succeeds. This already sucks when there are many keys and sucks even more if the whole list has to be iterated for every item of a secret

mkmik pushed a commit that referenced this issue Aug 5, 2019
Prepares the ground for addressing #185 and #199 without adding any feature.

The only small side effect is that now private keys are attempted in random order (Go map iteration order).
This could only affect users who have turned on the key rotation feature which, as mentioned in #137, is not yet ready for prime time in the first place.
mkmik pushed a commit that referenced this issue Aug 5, 2019
Prepares the ground for addressing #185 and #199 without adding any feature.

The only small side effect is that now private keys are attempted in random order (Go map iteration order).
This could only affect users who have turned on the key rotation feature which, as mentioned in #137, is not yet ready for prime time in the first place.
mkmik pushed a commit that referenced this issue Aug 5, 2019
Prepares the ground for addressing #185 and #199 without adding any feature.

The only small side effect is that now private keys are attempted in random order (Go map iteration order).
This could only affect users who have turned on the key rotation feature which, as mentioned in #137, is not yet ready for prime time in the first place.
mkmik pushed a commit that referenced this issue Aug 6, 2019
Prepares the ground for addressing #185 and #199 without adding any feature.

The only small side effect is that now private keys are attempted in random order (Go map iteration order).
This could only affect users who have turned on the key rotation feature which, as mentioned in #137, is not yet ready for prime time in the first place.
mkmik pushed a commit that referenced this issue Aug 6, 2019
Ref #185

This is not the best implementation, but it technically unblocks #185 and lets us incrementally implement
a better solution (the current plan is to make kubeseal encode the fingerprint of the public key used to seal the secret in each item).
mkmik pushed a commit that referenced this issue Aug 6, 2019
Prepares the ground for addressing #185 and #199 without adding any feature.

The only small side effect is that now private keys are attempted in random order (Go map iteration order).
This could only affect users who have turned on the key rotation feature which, as mentioned in #137, is not yet ready for prime time in the first place.
mkmik pushed a commit that referenced this issue Aug 6, 2019
Ref #185

This is not the best implementation, but it technically unblocks #185 and lets us incrementally implement
a better solution (the current plan is to make kubeseal encode the fingerprint of the public key used to seal the secret in each item).
mkmik pushed a commit that referenced this issue Aug 6, 2019
Prepares the ground for addressing #185 and #199 without adding any feature.

The only small side effect is that now private keys are attempted in random order (Go map iteration order).
This could only affect users who have turned on the key rotation feature which, as mentioned in #137, is not yet ready for prime time in the first place.
mkmik pushed a commit that referenced this issue Aug 6, 2019
Ref #185

This is not the best implementation, but it technically unblocks #185 and lets us incrementally implement
a better solution (the current plan is to make kubeseal encode the fingerprint of the public key used to seal the secret in each item).
mkmik pushed a commit that referenced this issue Aug 6, 2019
Prepares the ground for addressing #185 and #199 without adding any feature.

The only small side effect is that now private keys are attempted in random order (Go map iteration order).
This could only affect users who have turned on the key rotation feature which, as mentioned in #137, is not yet ready for prime time in the first place.
mkmik pushed a commit that referenced this issue Aug 6, 2019
Prepares the ground for addressing #185 and #199 without adding any feature.

The only small side effect is that now private keys are attempted in random order (Go map iteration order).
This could only affect users who have turned on the key rotation feature which, as mentioned in #137, is not yet ready for prime time in the first place.
mkmik pushed a commit that referenced this issue Aug 6, 2019
Ref #185

This is not the best implementation, but it technically unblocks #185 and lets us incrementally implement
a better solution (the current plan is to make kubeseal encode the fingerprint of the public key used to seal the secret in each item).
mkmik pushed a commit that referenced this issue Aug 7, 2019
Prepares the ground for addressing #185 and #199 without adding any feature.

The only small side effect is that now private keys are attempted in random order (Go map iteration order).
This could only affect users who have turned on the key rotation feature which, as mentioned in #137, is not yet ready for prime time in the first place.
mkmik pushed a commit that referenced this issue Aug 7, 2019
Ref #185

This is not the best implementation, but it technically unblocks #185 and lets us incrementally implement
a better solution (the current plan is to make kubeseal encode the fingerprint of the public key used to seal the secret in each item).
bors bot added a commit that referenced this issue Aug 7, 2019
214: Refactor key book-keeping r=mkmik a=mkmik

Prepares the ground for addressing #185 and #199 without adding any feature.

The only small side effect is that now private keys are attempted in random order (Go map iteration order).
This could only affect users who have turned on the key rotation feature which, as mentioned in #137, is not yet ready for prime time in the first place.
Also improves on #190.

Co-authored-by: Marko Mikulicic <[email protected]>
mkmik pushed a commit that referenced this issue Aug 7, 2019
Ref #185

This is not the best implementation, but it technically unblocks #185 and lets us incrementally implement
a better solution (the current plan is to make kubeseal encode the fingerprint of the public key used to seal the secret in each item).
mkmik pushed a commit that referenced this issue Aug 7, 2019
Ref #185

This is not the best implementation, but it technically unblocks #185 and lets us incrementally implement
a better solution (the current plan is to make kubeseal encode the fingerprint of the public key used to seal the secret in each item).
bors bot added a commit that referenced this issue Aug 27, 2019
216: Handle items sealed with different keys r=mkmik a=mkmik

Ref #185

This is not the best implementation, but it technically unblocks #185 and lets us incrementally implement a better solution (the current plan is to make kubeseal encode the fingerprint of the public key used to seal the secret in each item).

Co-authored-by: Marko Mikulicic <[email protected]>
mkmik pushed a commit that referenced this issue Aug 28, 2019
Ref #185

This is not the best implementation, but it technically unblocks #185 and lets us incrementally implement
a better solution (the current plan is to make kubeseal encode the fingerprint of the public key used to seal the secret in each item).
bors bot added a commit that referenced this issue Aug 28, 2019
216: Handle items sealed with different keys r=mkmik a=mkmik

Ref #185

This is not the best implementation, but it technically unblocks #185 and lets us incrementally implement a better solution (the current plan is to make kubeseal encode the fingerprint of the public key used to seal the secret in each item).

Co-authored-by: Marko Mikulicic <[email protected]>
@mkmik
Copy link
Collaborator

mkmik commented Aug 28, 2019

Fixed in #216 but in a rather inefficient way.
We'll deal with the efficiency issue in #226.

@mkmik mkmik closed this as completed Aug 28, 2019
@mkmik mkmik modified the milestones: v0.9.0, v0.8.2 Aug 28, 2019
mkmik pushed a commit that referenced this issue Sep 3, 2019
All major blocker issues have been already closed (#185, #190) and stakeholders
have been given some time to play with the feature.

Users can easily turn this off by setting an env var, see #234.

I think we're ready to turn this on for everybody.
We have time to improve the efficiency, see #226.

Closes #137
mkmik pushed a commit that referenced this issue Sep 4, 2019
All major blocker issues have been already closed (#185, #190) and stakeholders
have been given some time to play with the feature.

Users can easily turn this off by setting an env var, see #234.

I think we're ready to turn this on for everybody.
We have time to improve the efficiency, see #226.

Closes #137
bors bot added a commit that referenced this issue Sep 4, 2019
236: Turn on key rotation every 30 days r=mkmik a=mkmik

All major blocker issues have been already closed (#185, #190) and stakeholders
have been given some time to play with the feature.

Users can easily turn this off by setting an env var, see #234.

I think we're ready to turn this on for everybody.
We have time to improve the efficiency, see #226.

Closes #137

Co-authored-by: Marko Mikulicic <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants