Modify RBD plugin to use a single ID and move the id and key into the… #395

ShyamsundarR · 2019-06-01T21:30:57Z

… secret

RBD plugin needs only a single ID to manage images and operations against a
pool, mentioned in the storage class. The current scheme of 2 IDs is hence not
needed and removed in this commit.

Further, unlike CephFS plugin, the RBD plugin splits the user id and the key
into the storage class and the secret respectively. Also the parameter name
for the key in the secret is noted in the storageclass making it a variant and
hampers usability/comprehension. This is also fixed by moving the id and the key
to the secret and not retaining the same in the storage class, like CephFS.

Fixes #270

Testing done:

Basic PVC creation and mounting

Signed-off-by: ShyamsundarR srangana@redhat.com

dillaman · 2019-06-03T18:57:52Z

examples/rbd/secret.yaml

+  # Key values correspond to a user name and its key, base 64 encoded, as
+  # defined in the ceph cluster. User ID should have required access to the
+  # 'pool' specified in the storage class
+  userID: BASE64-ENCODED-VALUE


The ID shouldn't need to be base64 encoded

If the key userID is mentioned in a section named stringData it need not be encoded in base64 and can be mentioned in plain text. This is how a secret is specified in Kubernetes [1].

If required I can change the documentation to reflect the added stringData and mentioning the userID key within the same.

[1] Kubernetes manual secrets and data stringData options for base64 and plain text secrets: https://kubernetes.io/docs/concepts/configuration/secret/#creating-a-secret-manually

I'd vote for stringData --- honestly, since the key is already base64 encoded, I always thought it was a little silly (usability-wise) that it needed to be encoded again, so perhaps both could be moved? or perhaps just move the userId back to the StorageClass?

Then stringData it is. I will move both, as the key otherwise is double base64 encoded, as you point out.

Fractured ID and key in 2 files, to me, is a far worse usability proposition, so no I would not go that way.

Madhu-1 · 2019-06-11T04:23:30Z

pkg/rbd/controllerserver.go

@@ -122,7 +128,7 @@ func (cs *ControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 		return nil, err
 	}

-	found, err := checkVolExists(rbdVol, req.GetSecrets())
+	found, err := checkVolExists(rbdVol, cr.ID, cr.Key)


pass cr struct to checkVolExists

same applies wherever we are passing ID and Key

Done.

Did this more across the code, so that it is cleaner, than just certain instances.

pkg/rbd/controllerserver.go

Madhu-1 · 2019-06-11T04:30:56Z

@ShyamsundarR please fix merge conflicts

ShyamsundarR · 2019-06-11T19:30:31Z

@ShyamsundarR please fix merge conflicts

Done, also changed some code in e2e, to adapt to the changes storage class and secrets for RBD and CephFS.

ShyamsundarR · 2019-06-12T09:38:46Z

@humblec PTAL

humblec · 2019-06-12T10:28:10Z

examples/cephfs/secret.yaml

-  userID: BASE64-ENCODED-VALUE
-  userKey: BASE64-ENCODED-VALUE
+  userID: <plaintext ID>
+  userKey: <Ceph auth key corresponding to ID above>


why are we moving away from BASE64 to plain text here?

See usability discussion here, #395 (comment)

Basically, moving away to make it easy for an user to specify plain text IDs and key is already base64 encoded by Ceph.

ShyamsundarR · 2019-06-14T15:22:54Z

@humblec PTAL (rebased post recent PR merges to master)

humblec · 2019-06-15T09:08:37Z

@ShyamsundarR apart from PLAIN TEXT password, changes looks good to me. Please use base64 encoded values in secret.

dillaman · 2019-06-15T12:13:24Z

@ShyamsundarR apart from PLAIN TEXT password, changes looks good to me. Please use base64 encoded values in secret.

The CephX secrets are already base64 encoded, so this just eliminates the unnecessary double base64 encoding. Plus, it's not like an extra layer of base64 encoding offers any protection for the key.

ShyamsundarR · 2019-06-15T12:46:06Z

@ShyamsundarR apart from PLAIN TEXT password, changes looks good to me. Please use base64 encoded values in secret.

The CephX secrets are already base64 encoded, so this just eliminates the unnecessary double base64 encoding. Plus, it's not like an extra layer of base64 encoding offers any protection for the key.

@humblec This would be my reasoning as well, and also on reading the difference between data and stringData as in the kubernetes docs [1], it is just a convenience.

[1] https://kubernetes.io/docs/concepts/configuration/secret/#creating-a-secret-manually

humblec · 2019-06-19T06:40:55Z

@ShyamsundarR @dillaman the best practice of the secret key creation and use is base 64 encoded values. Thats been the practice in the field and also the way people use secrets . I dont see a reason why we have to move away from it. IMO, regardless of how the backend use it, it is better to store it in base 64 for kubernetes artifacts. @Madhu-1 what do you think ?

dillaman · 2019-06-19T11:39:02Z

Please provide a reference for "the best practice of the secret key creation and use is base 64 encoded values".

Again, it's already base64 encoded and double-encoding it adds exactly zero value. Plus, it's confusing to users (look at the issues where folks have incorrectly re-encoded the CephX base64 key by either incorrectly appending a carriage return or not re-encoding it).

humblec

Please provide a reference for "the best practice of the secret key creation and use is base 64 encoded values".

Again, it's already base64 encoded and double-encoding it adds exactly zero value. Plus, it's confusing to users (look at the issues where folks have incorrectly re-encoded the CephX base64 key by either incorrectly appending a carriage return or not re-encoding it).

@dillaman @shyamsundar I dont know there exist a document called best practice. However I can point out resources like, in OCS we use base64 encoded secret as shown here https://github.com/kubernetes/examples/blob/master/staging/persistent-volume-provisioning/glusterfs/glusterfs-secret.yaml . The examples in kubernetes repo also list many instances ( RBD ..etc) with base 64 encoded secrets. Also think, in first place we put base64 encoded values in secret here ( ceph-csi) too. These examples lists from kube and the first samples/existing method of ceph-csi itself pointing out that thats the common or best practice people follow or used to. I dont want to mention it explictly, however, existing users also have a practice of using it in that way. Thats why I was wondering why to change it ? if thats not going to give us any advantages? If there are user issues or complaints about , using base 64 is a pain or need to be reconsidered, please point , may be I missed those references. Please note, I am trying to share my view based on the experience, but it doesnot mean that, we should not change, but atleast I have to point out.

humblec · 2019-06-15T09:07:37Z

examples/cephfs/secret.yaml


  # Required for dynamically provisioned volumes
-  adminID: BASE64-ENCODED-VALUE
-  adminKey: BASE64-ENCODED-VALUE
+  adminID: <plaintext ID>


@ShyamsundarR the secret values in BASE64 is the recommended practice. IMO, we should not use plain text in secret , thats actually a kind of security issue.

Please specify what the security issue is? The plain text stringData is just a user convenience, it is not stored and displayed in plain text if the secret is queried, just like passing it in using data instead.

The documentation here [1] states this: "stringData allows specifying non-binary secret data in string form. It is provided as a write-only convenience method. All keys and values are merged into the data field on write, overwriting any existing values. It is never output when reading from the API."

[1] Read stringData section: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.12/#secret-v1-core

humblec · 2019-06-15T09:07:48Z

examples/rbd/secret.yaml

+  # Key values correspond to a user name and its key, as defined in the
+  # ceph cluster. User ID should have required access to the 'pool'
+  # specified in the storage class
+  userID: <plaintext ID>


Same as above.

ShyamsundarR · 2019-06-22T10:40:20Z

Please provide a reference for "the best practice of the secret key creation and use is base 64 encoded values".
Again, it's already base64 encoded and double-encoding it adds exactly zero value. Plus, it's confusing to users (look at the issues where folks have incorrectly re-encoded the CephX base64 key by either incorrectly appending a carriage return or not re-encoding it).

@dillaman @shyamsundar I dont know there exist a document called best practice. However I can point out resources like, in OCS we use base64 encoded secret as shown here https://github.com/kubernetes/examples/blob/master/staging/persistent-volume-provisioning/glusterfs/glusterfs-secret.yaml . The examples in kubernetes repo also list many instances ( RBD ..etc) with base 64 encoded secrets. Also think, in first place we put base64 encoded values in secret here ( ceph-csi) too. These examples lists from kube and the first samples/existing method of ceph-csi itself pointing out that thats the common or best practice people follow or used to. I dont want to mention it explictly, however, existing users also have a practice of using it in that way. Thats why I was wondering why to change it ? if thats not going to give us any advantages? If there are user issues or complaints about , using base 64 is a pain or need to be reconsidered, please point , may be I missed those references. Please note, I am trying to share my view based on the experience, but it doesnot mean that, we should not change, but atleast I have to point out.

Repeating comment on security issue here again:
The plain text stringData is just a user convenience, it is not stored and displayed in plain text if the secret is queried, just like passing it in using data instead.

The documentation here [1] states this: "stringData allows specifying non-binary secret data in string form. It is provided as a write-only convenience method. All keys and values are merged into the data field on write, overwriting any existing values. It is never output when reading from the API."

I believe @humblec you are missing the usability point, and as already pointed out by @dillaman there are users who have incorrectly encoded the key or ID (with trailing newlines or missing the encoding). Here are some that I could find as issues #350 #98 #80 and there has been at least one or two more that I addressed in slack channels.

Finally: This PR blocks for me a couple of other PRs in flight (#391, and partially at least #318 ), so a decision here sooner the better.

[1] Read stringData section: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.12/#secret-v1-core

humblec · 2019-06-24T02:48:20Z

@ShyamsundarR please rebase.

shyamsundar · 2019-06-24T05:00:56Z

Hi , I think you are talking to a wrong shyam Sundar. I don't know anything this project. Regards.

…

On Sun, Jun 23, 2019, 7:48 PM Humble Devassy Chirammal < ***@***.***> wrote: @ShyamsundarR <https://github.com/ShyamsundarR> please rebase. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#395?email_source=notifications&email_token=AANZLJECDHMPWQGCEWKPR4LP4AYXPA5CNFSM4HSBK2XKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYLTKKQ#issuecomment-504837418>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANZLJGXX2TTSXIEFA6R5GLP4AYXPANCNFSM4HSBK2XA> .

humblec · 2019-06-24T05:45:46Z

Hi , I think you are talking to a wrong shyam Sundar. I don't know anything this project.
@shyamsundar ah.. my bad .. sorry for the wrong notification, please discard. :(

… secret RBD plugin needs only a single ID to manage images and operations against a pool, mentioned in the storage class. The current scheme of 2 IDs is hence not needed and removed in this commit. Further, unlike CephFS plugin, the RBD plugin splits the user id and the key into the storage class and the secret respectively. Also the parameter name for the key in the secret is noted in the storageclass making it a variant and hampers usability/comprehension. This is also fixed by moving the id and the key to the secret and not retaining the same in the storage class, like CephFS. Fixes ceph#270 Testing done: - Basic PVC creation and mounting Signed-off-by: ShyamsundarR <srangana@redhat.com>

ShyamsundarR · 2019-06-24T11:19:43Z

@ShyamsundarR please rebase.

@humblec Done PTAL.

humblec · 2019-06-24T13:24:39Z

Thanks @ShyamsundarR . As discussed we may need a mention in the release-note or changelog of v1.1 that, we are flexible on plain text or base 64 input . We also need a followup PR to Rook to accommodate these changes in manifests shipped in Rook.

LGTM. thanks !

cephcsi PR ceph/ceph-csi#395 changed the way we are storing secrets. This PR will bring same changes to the rook as well. Fixes: rook#3357 Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

nehaberry · 2019-06-27T04:34:07Z

@Madhu-1 @shyamsundar I have a question:

Does that mean for rbd, we only need to specify a single user and its key and not 2 users like cephfs (admin and kubernetes) ?.

Also, that user in rbd/secret.yaml can either be an "admin" user, or a normal user , e.g. ceph_user1 ? What if we want to give both , is that not supported now ?

ceph_user1 = ceph auth get-or-create-key client.kubernetes mon  "allow profile rbd" osd "profile rbd pool=rbd"

userID: ${admin_id}
userKey: ${admin_key}

OR

userID: ${ceph_user1_id}
userKey: ${ceph_user1_key}

nehaberry · 2019-06-27T04:48:38Z

@Madhu-1 @shyamsundar I tried this and it worked.. But I am a little confused if 2 users in secret are going to be supported or not:


oc get pvc

rbd-pvc2     Bound     pvc-6f89e5ba-9896-11e9-9734-0a13693d844a   1Gi        RWO            csi-rbd2       4s


(new-ocs-ci) [nberry@localhost rbd]$ oc get secret/csi-rbd-secret2 -n default -o yaml
apiVersion: v1
data:
  adminID: YWRtaW4=
  adminKey: QVFCdVBSTmRxSFJTTmhBQVNGMmxlVkJkVGV3T3NBWFJHZnFxVXc9PQ==
  userID: a3ViZXJuZXRlcw==
  userKey: QVFDM1FCTmR4NzRmR0JBQVI1d1NVbkg3SjAvWFYyZUxLN1U4SWc9PQ==
kind: Secret
metadata:
  creationTimestamp: "2019-06-27T04:44:12Z"
  name: csi-rbd-secret2
  namespace: default
  resourceVersion: "373315"
  selfLink: /api/v1/namespaces/default/secrets/csi-rbd-secret2
  uid: 35db2587-9896-11e9-b24d-02b94c70af94
type: Opaque


(new-ocs-ci) [nberry@localhost rbd]$ oc get sc/csi-rbd2 -n default -o yaml
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  creationTimestamp: "2019-06-27T04:44:43Z"
  name: csi-rbd2
  resourceVersion: "373465"
  selfLink: /apis/storage.k8s.io/v1/storageclasses/csi-rbd2
  uid: 47f4e969-9896-11e9-b24d-02b94c70af94
parameters:
  adminid: admin
  clusterID: openshift-storage
  csi.storage.k8s.io/node-publish-secret-name: csi-rbd-secret2
  csi.storage.k8s.io/node-publish-secret-namespace: default
  csi.storage.k8s.io/provisioner-secret-name: csi-rbd-secret2
  csi.storage.k8s.io/provisioner-secret-namespace: default
  imageFeatures: layering
  imageFormat: "2"
  pool: rbd
  userid: kubernetes
provisioner: rbd.csi.ceph.com
reclaimPolicy: Delete
volumeBindingMode: Immediate

cephcsi PR ceph/ceph-csi#395 changed the way we are storing secrets. This PR will bring same changes to the rook as well. Fixes: rook#3357 Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

Madhu-1 · 2019-06-27T09:21:40Z

@nehaberry adminID and adminKey are supported for cephfs not for rbd.
rbd requires only one user, even if you specify any keys other than userID and userKey we are not validating the extra keys in secret

Madhu-1 · 2019-06-27T09:24:47Z

@Madhu-1 @shyamsundar I have a question:

Does that mean for rbd, we only need to specify a single user and its key and not 2 users like cephfs (admin and kubernetes) ?.

Also, that user in rbd/secret.yaml can either be an "admin" user, or a normal user , e.g. ceph_user1 ? What if we want to give both , is that not supported now ?
ceph_user1 = ceph auth get-or-create-key client.kubernetes mon  "allow profile rbd" osd "profile rbd pool=rbd"
userID: ${admin_id}
userKey: ${admin_key}

OR

userID: ${ceph_user1_id}
userKey: ${ceph_user1_key}

you can have both users in secret, either you can use admin user or ceph_user1 but he should have the permission for the pool mentioned in storage class

nehaberry · 2019-06-27T09:27:58Z

@Madhu-1 thanks for the clarification. So, we actually don't need a separate adminID anymore since we can specify admin user in "userID" irtself. Makes sense.

Thanks again

cephcsi PR ceph/ceph-csi#395 changed the way we are storing secrets. This PR will bring same changes to the rook as well. Fixes: rook#3357 Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

Syncing latest changes from devel for ceph-csi

ShyamsundarR requested review from Madhu-1 and humblec June 1, 2019 21:31

dillaman reviewed Jun 3, 2019

View reviewed changes

Madhu-1 requested changes Jun 11, 2019

View reviewed changes

ShyamsundarR force-pushed the rbd-update-secret-config branch 2 times, most recently from d90975d to e4eac4a Compare June 11, 2019 19:13

Madhu-1 approved these changes Jun 12, 2019

View reviewed changes

humblec reviewed Jun 12, 2019

View reviewed changes

ShyamsundarR force-pushed the rbd-update-secret-config branch from e4eac4a to 768ac89 Compare June 14, 2019 15:06

humblec reviewed Jun 22, 2019

View reviewed changes

ShyamsundarR force-pushed the rbd-update-secret-config branch from 768ac89 to 5a8226b Compare June 24, 2019 11:02

ShyamsundarR mentioned this pull request Jun 24, 2019

Update base dependency of kube to 1.15.0 #439

Merged

humblec added release-note-required PRs or issues which need entry in "release-note" or "changelog" release-1.1.0 Track release 1.1 items labels Jun 24, 2019

humblec self-assigned this Jun 24, 2019

humblec approved these changes Jun 24, 2019

View reviewed changes

Merge branch 'master' into rbd-update-secret-config

6a95ab6

mergify bot merged commit c5762b6 into ceph:master Jun 24, 2019

ShyamsundarR deleted the rbd-update-secret-config branch June 25, 2019 14:01

Madhu-1 mentioned this pull request Jun 26, 2019

Update rbd and cephfs secret rook/rook#3358

Merged

5 tasks

hswong3i added a commit to alvistack/ansible-role-kube_csi_rbd that referenced this pull request Jun 28, 2019

https://github.com/ceph/ceph-csi/pull/395

860e597

Madhu-1 mentioned this pull request Jul 16, 2019

doc: adminid and userid not documented #80

Closed

ShyamsundarR mentioned this pull request Dec 8, 2019

Adds persistent volume claims encryption with LUKS #697

Merged

yati1998 pushed a commit to yati1998/ceph-csi that referenced this pull request Nov 26, 2024

Merge pull request ceph#395 from red-hat-storage/sync_ds--devel

4edd440

Syncing latest changes from devel for ceph-csi

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify RBD plugin to use a single ID and move the id and key into the… #395

Modify RBD plugin to use a single ID and move the id and key into the… #395

ShyamsundarR commented Jun 1, 2019 •

edited

Loading

dillaman Jun 3, 2019

ShyamsundarR Jun 11, 2019

dillaman Jun 11, 2019

ShyamsundarR Jun 11, 2019

Madhu-1 Jun 11, 2019

Madhu-1 Jun 11, 2019

ShyamsundarR Jun 11, 2019

Madhu-1 commented Jun 11, 2019

ShyamsundarR commented Jun 11, 2019

ShyamsundarR commented Jun 12, 2019

humblec Jun 12, 2019

ShyamsundarR Jun 12, 2019 •

edited

Loading

ShyamsundarR commented Jun 14, 2019

humblec commented Jun 15, 2019

dillaman commented Jun 15, 2019

ShyamsundarR commented Jun 15, 2019

humblec commented Jun 19, 2019

dillaman commented Jun 19, 2019

humblec left a comment •

edited

Loading

humblec Jun 15, 2019

ShyamsundarR Jun 22, 2019

humblec Jun 15, 2019

ShyamsundarR commented Jun 22, 2019

humblec commented Jun 24, 2019

shyamsundar commented Jun 24, 2019 via email

humblec commented Jun 24, 2019

ShyamsundarR commented Jun 24, 2019

humblec commented Jun 24, 2019

nehaberry commented Jun 27, 2019

nehaberry commented Jun 27, 2019 •

edited

Loading

Madhu-1 commented Jun 27, 2019

Madhu-1 commented Jun 27, 2019

nehaberry commented Jun 27, 2019

Modify RBD plugin to use a single ID and move the id and key into the… #395

Modify RBD plugin to use a single ID and move the id and key into the… #395

Conversation

ShyamsundarR commented Jun 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Jun 11, 2019

ShyamsundarR commented Jun 11, 2019

ShyamsundarR commented Jun 12, 2019

Choose a reason for hiding this comment

ShyamsundarR Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

ShyamsundarR commented Jun 14, 2019

humblec commented Jun 15, 2019

dillaman commented Jun 15, 2019

ShyamsundarR commented Jun 15, 2019

humblec commented Jun 19, 2019

dillaman commented Jun 19, 2019

humblec left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShyamsundarR commented Jun 22, 2019

humblec commented Jun 24, 2019

shyamsundar commented Jun 24, 2019 via email

humblec commented Jun 24, 2019

ShyamsundarR commented Jun 24, 2019

humblec commented Jun 24, 2019

nehaberry commented Jun 27, 2019

nehaberry commented Jun 27, 2019 • edited Loading

Madhu-1 commented Jun 27, 2019

Madhu-1 commented Jun 27, 2019

nehaberry commented Jun 27, 2019

ShyamsundarR commented Jun 1, 2019 •

edited

Loading

ShyamsundarR Jun 12, 2019 •

edited

Loading

humblec left a comment •

edited

Loading

nehaberry commented Jun 27, 2019 •

edited

Loading