Skip to content

kms: Root key rotation for Vault KMS#1457

Merged
jackyalbo merged 2 commits intonoobaa:masterfrom
iPraveenParihar:kms/vault-keyrotation
Nov 11, 2024
Merged

kms: Root key rotation for Vault KMS#1457
jackyalbo merged 2 commits intonoobaa:masterfrom
iPraveenParihar:kms/vault-keyrotation

Conversation

@iPraveenParihar
Copy link
Contributor

@iPraveenParihar iPraveenParihar commented Oct 7, 2024

This PR adds support for key Rotation for Vault KMS.

@iPraveenParihar iPraveenParihar marked this pull request as ready for review October 7, 2024 11:30
@nimrod-becker
Copy link
Contributor

nimrod-becker commented Oct 8, 2024

@dannyzaken @jackyalbo @romayalon can you PTAL?

@iPraveenParihar
Copy link
Contributor Author

iPraveenParihar commented Oct 8, 2024

Testing Results -

Configured Vault KMS (vault implementing VersionSingleSecret)

  • secret created and stored in vault
    KMS-Status updated to Sync
  - lastHeartbeatTime: "2024-10-08T08:43:05Z"
    lastTransitionTime: "2024-10-08T08:38:35Z"
    status: vault
    type: KMS-Type
  - lastHeartbeatTime: "2024-10-08T08:43:05Z"
    lastTransitionTime: "2024-10-08T08:38:35Z"
    status: Sync
    type: KMS-Status
  • From vault KMS , secret stored under path NOOBAA_ROOT_SECRET_PATH with key name rootkeyb64-<noobaa-UID>
$ vault kv list vault-test-fff0175734684927973ef4796eab7/NOOBAA_ROOT_SECRET_PATH/                                              
Keys
----
rootkeyb64-f5f3dcfa-69a0-4f57-bcf6-6d75f823fc01
$ vault kv get vault-test-fff0175734684927973ef4796eab7/NOOBAA_ROOT_SECRET_PATH/rootkeyb64-f5f3dcfa-69a0-4f57-bcf6-6d75f823fc01
==================================================== Secret Path ====================================================
vault-test-fff0175734684927973ef4796eab7/data/NOOBAA_ROOT_SECRET_PATH/rootkeyb64-f5f3dcfa-69a0-4f57-bcf6-6d75f823fc01

======= Metadata =======
Key                Value
---                -----
created_time       2024-10-08T08:38:35.105804254Z
custom_metadata    <nil>
deletion_time      n/a
destroyed          false
version            1

========================= Data =========================
Key                                                Value
---                                                -----
rootkeyb64-f5f3dcfa-69a0-4f57-bcf6-6d75f823fc01    7fzTiXYYZg99M4nMNhGMO095Qy0UIyddDieP1hjqXYI=
  • After changing the NooBaa operator image (vault implementing VersionRotatingSecret)
    Now, the secret is stored under root path with key name noobaa-root-master-key-backend
$ vault kv list vault-test-fff0175734684927973ef4796eab7/                                                                      
Keys
----
noobaa-root-master-key-backend

$ vault kv get vault-test-fff0175734684927973ef4796eab7/noobaa-root-master-key-backend                                         
================================ Secret Path ================================
vault-test-fff0175734684927973ef4796eab7/data/noobaa-root-master-key-backend

======= Metadata =======
Key                Value
---                -----
created_time       2024-10-08T08:53:04.215345339Z
custom_metadata    <nil>
deletion_time      n/a
destroyed          false
version            1

============= Data =============
Key                        Value
---                        -----
active_root_key            key-1728377584212169051
key-1728377584212169051    7fzTiXYYZg99M4nMNhGMO095Qy0UIyddDieP1hjqXYI=

The same stored in k8s secrets and mounted to endpoint pod

$ k get secrets noobaa-root-master-key-volume -oyaml
apiVersion: v1
data:
  active_root_key: a2V5LTE3MjgzNzc1ODQyMTIxNjkwNTE=
  key-1728377584212169051: N2Z6VGlYWVlaZzk5TTRuTU5oR01PMDk1UXkwVUl5ZGREaWVQMWhqcVhZST0=
kind: Secret
metadata:
  creationTimestamp: "2024-10-08T08:53:38Z"
  labels:
    app: noobaa
  name: noobaa-root-master-key-volume
  namespace: openshift-storage
  • Enabling Key rotation

operator logs

time="2024-10-08T09:05:01Z" level=info msg="Authenticated to Vault with token\n"
time="2024-10-08T09:05:01Z" level=info msg="setKMSConditionType vault" sys=openshift-storage/noobaa
time="2024-10-08T09:05:01Z" level=info msg="ReconcileObject: Done - updated Secret noobaa-root-master-key-volume " sys=openshift-storage/noobaa
time="2024-10-08T09:05:01Z" level=info msg="setKMSConditionStatus Sync" sys=openshift-storage/noobaa
time="2024-10-08T09:05:01Z" level=info msg="ReconcileKeyRotation, KMS Starting" sys=openshift-storage/noobaa
time="2024-10-08T09:05:01Z" level=info msg="ReconcileKeyRotation, KMS rotation nextSchedule: 2024-10-08 09:05:00 +0000 UTC" sys=openshift-storage/noobaa
time="2024-10-08T09:05:01Z" level=info msg="Key rotation started at 2024-10-08 09:05:01.533742832 +0000 UTC m=+716.201718414" sys=openshift-storage/noobaa
time="2024-10-08T09:05:01Z" level=info msg="Update event detected for noobaa-root-master-key-volume (openshift-storage), queuing Reconcile"
time="2024-10-08T09:05:01Z" level=info msg="ReconcileObject: Done - updated Secret noobaa-root-master-key-volume " sys=openshift-storage/noobaa
time="2024-10-08T09:05:01Z" level=info msg="setKMSConditionStatus KeyRotate" sys=openshift-storage/noobaa
time="2024-10-08T09:05:01Z" level=info msg="ReconcileKeyRotation, KMS Updating Last Key Rotate time: 2024-10-08 09:05:01.533724531 +0000 UTC m=+716.201700113" sys=openshift-storage/noobaa
  • Update in Vault KMS as version 2
$ vault kv get vault-test-fff0175734684927973ef4796eab7/noobaa-root-master-key-backend
================================ Secret Path ================================
vault-test-fff0175734684927973ef4796eab7/data/noobaa-root-master-key-backend

======= Metadata =======
Key                Value
---                -----
created_time       2024-10-08T09:05:01.821321812Z
custom_metadata    <nil>
deletion_time      n/a
destroyed          false
version            2                     // NOTE: Version 2 is created

============= Data =============
Key                        Value
---                        -----
active_root_key            key-1728378301818071119
key-1728378301818071119    X6GuUG8i+Nf/aEXoido98j1u6lHKIt2pyk4LYocjFpY=
  • Updated in k8s secret
$ k get secrets noobaa-root-master-key-volume -oyaml                                  
apiVersion: v1
data:
  active_root_key: a2V5LTE3MjgzNzgzMDE4MTgwNzExMTk=.    //  key-1728378301818071119
  key-1728377584212169051: N2Z6VGlYWVlaZzk5TTRuTU5oR01PMDk1UXkwVUl5ZGREaWVQMWhqcVhZST0=
  key-1728378301818071119: WDZHdVVHOGkrTmYvYUVYb2lkbzk4ajF1NmxIS0l0MnB5azRMWW9jakZwWT0=
kind: Secret
metadata:
  creationTimestamp: "2024-10-08T08:53:38Z"
  labels:
    app: noobaa
  name: noobaa-root-master-key-volume
  namespace: openshift-storage

@jackyalbo
Copy link
Contributor

jackyalbo commented Oct 15, 2024 •
edited
Loading

Not a Vault expert, will let @dannyzaken review also. But I think we want both versions in the Vault no? Can we still access version 1 after the rotation? I guess we can't rely only on the mounted secret to have all of the key's history in case of some catastrophic issues during the key rotation in core side.

@iPraveenParihar
Copy link
Contributor Author

iPraveenParihar commented Oct 16, 2024

Not a Vault expert, will let @dannyzaken review also. But I think we want both versions in the Vault no? Can we still access version 1 after the rotation? I guess we can't rely only on the mounted secret to have all of the key's history in case of some catastrophic issues during the key rotation in core side.

@jackyalbo You mean in the vault, we need something like this? with previous secrets?

============= Data =============
Key                        Value
---                        -----
active_root_key            key-1728378301818071119
key-1728378301818071119    X6GuUG8i+Nf/aEXoido98j1u6lHKIt2pyk4LYocjFpY=
key-1728378301818071119    WDZHdVVHOGkrTmYvYUVYb2lkbzk4ajF1NmxIS0l0MnB5azRMWW9jakZwWT0=

@iPraveenParihar
Copy link
Contributor Author

iPraveenParihar commented Oct 16, 2024

pushed changes and tested. Now the vault will have the previous keys as well

Initial

============= Data =============
Key                        Value
---                        -----
active_root_key            key-1729062082494730928
key-1729062082494730928    FejE+iKr2st/JHRNgR9PB5VFJ5XYLSly8Z186aIS46c=

After KeyRotation

============= Data =============
Key                        Value
---                        -----
active_root_key            key-1729062124774666630
key-1729062082494730928    FejE+iKr2st/JHRNgR9PB5VFJ5XYLSly8Z186aIS46c=
key-1729062124774666630    vOHgHCYi2q5XJ0SGiHhbGe3pxMfMwoCHa1HbOOcxDMo=

Also, tagging @baum as key rotation for k8s (#1071) was done by him.

baum
baum approved these changes Oct 20, 2024
View reviewed changes
Copy link
Contributor

@baum baum left a comment

Choose a reason for hiding this comment

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

lgtm 🖖

@iPraveenParihar
Copy link
Contributor Author

iPraveenParihar commented Oct 21, 2024

@dannyzaken, can you PTAL?

@iPraveenParihar
Copy link
Contributor Author

iPraveenParihar commented Nov 6, 2024

Hey @jackyalbo, do we need one more approval ✅ to get PR merged?

@iPraveenParihar
kms: support key rotation for vault
d2a7617 
Signed-off-by: Praveen M <m.praveen@ibm.com>
@iPraveenParihar
kms: add key rotation tests for vault
53d299b 
Signed-off-by: Praveen M <m.praveen@ibm.com>
@iPraveenParihar
Copy link
Contributor Author

iPraveenParihar commented Nov 11, 2024

rebased PR

@jackyalbo
Copy link
Contributor

jackyalbo commented Nov 11, 2024

Hey @jackyalbo, do we need one more approval ✅ to get PR merged?

We don't; we just need the tests to pass. I re-ran the failing test.

@jackyalbo jackyalbo merged commit 87b1023 into noobaa:master Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dannyzaken dannyzaken dannyzaken approved these changes

@jackyalbo jackyalbo jackyalbo approved these changes

@romayalon romayalon Awaiting requested review from romayalon

+1 more reviewer

@baum baum baum approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@iPraveenParihar @nimrod-becker @jackyalbo @baum @dannyzaken