Hello @jameshearttech, I want to work on this issue. Can you please assign it to me?

@jameshearttech we do store accesskey and accesssecret. what are you requesting here? you suggesting to objectstoreuser should point to someSecret for this values?

Probably they are asking similar to this #11336

Probably they are asking similar to this #11336

Yes I guess so, @jameshearttech can you please check above PR owner about its status?

@jameshearttech we do store accesskey and accesssecret. what are you requesting here? you suggesting to objectstoreuser should point to someSecret for this values?

Yes, exactly.

https://rook-io.slack.com/archives/CK9CF5H2R/p1674075543129569

In my case everything is committed to Git. Storing the accesskey and accesssecret in the CephObjectUserStore resource would potentially leak those sensitive values.

Btw, I don't think the docs reflect spec.accessKey and spec.secretKey if those are already implemented.

is there any new progress? @jameshearttech @thotz @divaspathak @subhamkrai @parth-gr

I thought this issue was related to #11336

Yeah, they are definitely related. If it makes sense, feel free to close this one.

EDIT: Oh, wait, I see the other issue was closed. The other issue has a lot of work done on it. In my case I was able to use block instead of bucket. Regardless, I think there should be a solution to this issue.

EDIT2: In my last edit I was setting up Loki and I thought that it only worked with object storage, but I found I could use file storage; however, that only works with single binary mode. We are looking to move to simple scaling mode, which requires object storage so this is now a block for me. Likewise, we are looking at implementing Thanos, which also requires object storage.

Hi,

I'd appreciate the approach of referring to existing k8s secrets. This would decouple the direct communication from Ceph to a KMS like Vault. One could implement K8s secrets by themselve and create CephObjectStoreUser instances referencing to an existing K8s secret.

I'm interested in this feature as well. I have rgw clients which are external to k8s. It would be great if I could use something like external-secrets operator to sync rgw user creds. This would be an alternative to using ldap auth.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

This is a feature request we should probably keep open.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

Please do not close feature request. Afaik this has not been implemented.

A few notes on implementing this feature for someone to pick it up:
In the createOrUpdateCephUser() method, on line 298 if the GetUser() returns ErrNoSuchUser, then we need to:

1. Query if the K8s secret exists where Rook would later store the user creds.
2. If the secret exists and the expected keys exist (see example below), then set those values on the r.userConfig struct that is being passed to the CreateUser() method.
3. Assuming Ceph accepts the user secret as input, the user would be created with the pre-defined keys.

Here is an example user secret which is named rook-ceph-object-user-<store>-<user>:

% kubectl -n rook-ceph get secret rook-ceph-object-user-my-store-my-user -o yaml
apiVersion: v1
kind: Secret
data:
  AccessKey: U0lVNVJQTVg0VFIwMTI1QkxGSjc=
  SecretKey: aHdnbGdIVmlDcEJqTHN3WUJDakJqWlBRQ2tzeEM3dkRuRDdXMTNySw==
  Endpoint: aHR0cDovL3Jvb2stY2VwaC1yZ3ctbXktc3RvcmUucm9vay1jZXBoLnN2Yzo4MA==

If this secret is created in advance of the reconcile, then Rook would pick it up.

Note that since this issue was originally opened, now the user and secret could be created in any namespace since #12730 was implemented.

Removing the assigning as I'm focused on some other issue and I don't want to hold this to my name for long

Also added it to project board 1.13 to get more attention

I just don't understand what is SecretKeys: for?
I think it is related to supporting multiple secrets at a time, Let discuss that, maybe we can keep this change straightforward and add this change as a followup for supporting multiple keys

You said that when you tried adding a key via radosgw-admin, a new key was added, and the old key remained active. If we are beginning to implement better key rotation strategies, it seems natural to me to include some kind of output/feedback that helps users understand that multiple keys may exist. Users may accidentally forget to revoke a compromised key, and this can help them avoid that.

• Update flow (S3 user exists already)
• if rook.io/source-of-truth == secret
  • if SecretKeys is not empty, verify that SecretKey is in the SecretKeys list (error if not)

So the user when updating the secret should update the secret at two places always the SecretKey and then SecretKeys list ?

• update the user creds with the k8s secret content
  - if SecretKeys is empty, SecretKey is the only key that should exist for the S3 user
  - otherwise, all SecretKeys should be active for the S3 user

• if rook.io/source-of-truth != secret
  • update the k8s secret with the creds present from the S3 user
  • SecretKey should be filled with the last item in list of all active s3 secret keys

Do you mean the latest key, actually it appends the latest key at the top of the array

• SecretKeys should be filled in with comma-delimited list of all active s3 secret keys

If the SecretKeys already has some values that should be deleted and replaced with the ceph user keys?

I've been investigating key rotation for the COSI design updates as well, and after speaking with Josh on a call and collecting more information, I think we might want to revisit the design discussion for this Rook feature as well. It's tiring to get 90% of the way to completion with design and code and then have to rework it, but I think it may be worthwhile in this case.

From my discussion with Josh, the biggest gap that I think we have with the implementation proposed is that users don't have a good way to determine when the key rotation has completed. Josh has over a dozen k8s clusters that he manages, and that may grow to 2 dozen in not too long. In large environments like that, being able to know definitively when a key has been rotated is important. Status/monitoring/feedback is also a major tenet of the k8s design philosophy as well, and I missed it when proposing a design for this feature.

I'd like to talk about the details more in a Rook huddle next week, but the summary of my proposed new design is to not reuse the current secret for both input and output. I think it's simpler for developers and for users if we use a dedicated input secret, and Rook always uses the existing secret for output.

As for monitoring, I propose to achieve that by recording the metadata.generation field of the user input secret as a status field on the COSUser CR. Users can then easily use k8s tooling to compare the two values to know when the key rotation is complete.

I'd like to discuss this as a general idea before we start changing the code, so we don't burn ourselves out when reworking this. I think we can reuse a large part of the work we've already done on specifying the secret formatting, so hopefully we can limit that aspect. If that sounds good, we can iron out smaller details regarding COSUser spec/status API fields.

So for implementation:

1. Keep the original rgw secret as it is for the status.

2. If a user wants to provide their own credentials or do key rotation.
   i) Provide the user secret name in the objectuser CR
   CR will have a spec,

       spec:
         userSecretName: my-secret
   

   ii) Create or update the secret with this name

        apiVersion: v1
        kind: Secret
        metadata:
             name: my-secret
        data:
          AccessKey: U0lVNVJQTVg0VFIwMTI1QkxGSjc=
          SecretKey: aHdnbGdIVmlDcEJqTHN3WUJDakJqWlBRQ2tzeEM3dkRuRDdXMTNySw==
          Endpoint: aHR0cDovL3Jvb2stY2VwaC1yZ3ctbXktc3RvcmUucm9vay1jZXBoLnN2Yzo4MA==
   

   iii) The data fields remain the same as it was before
   iv) If the reconciliation of cephobjectstore user was successful and credentials were updated,
   Add the status to the cephobjectstore user CR, my-user secret creds successfully updated
   v) Also update the original rgw secret with the AccessKey and SecretKey.

@BlaineEXE please review is that what we need to do?

The key reason to make this change is to make sure we are also planning a way for users to get feedback about the key rotation process. We need to also plan the status fields. I would suggest this to be something like this.

status:
  # ...
  inputCredentials:
    secretName: <the name of the secret that Rook last used to apply creds>
    secretGeneration: <the generation of the secret Rook last used to apply creds>

Having secretName and secretGeneration are both important. The first allows the user to change the secret used, and ensure Rook applied the correct one. The second allows the user to know when the current secret matches what Rook has applied.

From my discussion with Josh, I think it is also still good to plan for a way that users can have multiple active credentials. Josh explained this to be important to him, as well as other admins. It allows a migration window for end users so that key rotation isn't something that causes application downtime.

I think we should still plan to include the Credentials field in both the output secret and input secret to allow for this. (We could use multiple input credentials, but I think having a single input secret will mean that status comparison is easier for users.)

Also, I have another concern: Because the resource is called CephObjectStoreUser, I think we should refrain from choosing userSecretName. IMO, this naming overlap allows too much room for misunderstanding.

I also think we might consider using the a new section and the inputCredentials.secretName pattern for the input spec. This helps clarify for users that we are inputting credentials, and that it takes a secret name. I don't think this is strictly necessary, but by creating a new section, it allows us flexibility in the future to add more fields to inputCredentials if we need to, without bloating the main spec. I'm curious to hear @travisn 's preferences here. I could go either way.

If we keep it in the top level, inputCredentialsSecretName would be the most clear naming, though inputSecretName would probably be fine. I would steer clear of credentialsSecretName since that might also be interpreted as the config changes the name of the secret Rook uses for output.

If we're going to use a different secret for input, that needs to be on the spec, right? Then the status would just have the generation of that secret that has been evaluated, something like this?

spec:
  inputCredentials:
    secretName: <the name of the secret that Rook last used to apply creds>
status:
  inputCredentials:
    secretGeneration: <the generation of the secret Rook last used to apply creds>

And I'm not clear what the proposal is for specifying multiple sets of creds? For simplicity it's nice to have the secret values match the output secret values, but if multiple sets of creds need to be specified, how are additional creds specified?