Azure kms handling for noobaa #1311

vh05 · 2024-03-04T15:45:07Z

This patch provides the support for azure keyvault.

We are using libopenstorage/secrets as the wrapper package to integrate with different kms and package provides the abstraction over several kms. It also provides the integraton support for azure and helps communication with azure key vault.

We are required to provide the definition for house keeping calls registered calls with libopenstorage/secrets.

libopenstorage/secrets does the creation of client handle based on the details provided in config data. Noobaa CR will have these details under connectionDetails and these details are popupated on Noobaa CR by Noobaa ocs by reading the configmap provided by ODF. The certificate details present in the secret are copied into a temp file and used to establish the connection with azure key vault as of now.

libopenstorage/secrets supports only secret as string as of now. Our requirement is to authenticate using certificate. To facililate this, we have PR to accomodate certificate related handling and the PR can be found here: https://github.com/libopenstorage/secrets/pull/83/files

Below are the connection details that are going to be populated on Noobaa CR by noobaa_system_reconciler at ocs side and this is the ocs code where connectiondetails on Noobaa CR are built: https://github.com/red-hat-storage/ocs-operator/blob/2d082fc4c1ac4cec961406053cece448f4b07684/controllers/storagecluster/noobaa_system_reconciler.go#L249

ex: configmap data:
AZURE_CERT_SECRET_NAME = Secret name from which we extract the certificate data and send it to libopenstorage/secrets to get the client handle for service principle to access the root master key

data:
  AZURE_CERT_SECRET_NAME: azure-ocs-ffwc9o1j
  AZURE_CLIENT_ID: az-client-id1
  AZURE_TENANT_ID: az-tenant-id1
  AZURE_VAULT_URL: az-valut-url1
  KMS_PROVIDER: azure-kv
  KMS_SERVICE_NAME: kms-conn-azure1

jackyalbo

I think we need to add test for azure KMS, also I think we need some more info about what we expect the info sent by the NooBaa CR should look like, I mean what config info we need.

jackyalbo · 2024-03-11T09:47:01Z

pkg/util/kms/kms_azure.go

+//
+
+// Config returns this driver secret config
+func (*AzureVault) Config(config map[string]string, tokenSecretName, namespace string) (map[string]interface{}, error) {


missing v no?

Suggested change

func (*AzureVault) Config(config map[string]string, tokenSecretName, namespace string) (map[string]interface{}, error) {

func (v *AzureVault) Config(config map[string]string, tokenSecretName, namespace string) (map[string]interface{}, error) {

Actually *AzureVault is just used to bind it to method and not using it inside the function/method

I see, I was confused with the key, value (k,v) in line 40

but let's remove all the unneeded as well, like in Get/Set/DeleteContext

These are the once we need to interface them with the driver type. Here: https://github.com/noobaa/noobaa-operator/blob/906a82032bfc8927145186984034c41577467c9d/pkg/util/kms/kms.go#L44C1-L53C2

Here are the functions calling Get/Set/Delete context.

noobaa-operator/pkg/util/kms/kms_version.go

Lines 40 to 74 in 906a820

// Get implements SecretStorage interface for single string secret

func (v *VersionSingleSecret) Get() error {

s, err := v.k.GetSecret(v.k.driver.Path(), v.k.driver.GetContext())

if err != nil {

// handle k8s get from non-existent secret

if strings.Contains(err.Error(), "not found") {

return secrets.ErrInvalidSecretId

}

return err

}

value, ok := s[v.k.driver.Name()]

if ok {

switch value.(type) {

case string:

v.data = s[v.k.driver.Name()].(string)

return nil

}

}

return secrets.ErrInvalidSecretId

}

// Set implements SecretStorage interface for single string secret

func (v *VersionSingleSecret) Set(val string) error {

data := map[string]interface{}{

v.k.driver.Name(): val,

}

v.data = val

return v.k.PutSecret(v.k.driver.Path(), data, v.k.driver.SetContext())

}

// Delete implements SecretStorage interface for single string secret

func (v *VersionSingleSecret) Delete() error {

return v.k.DeleteSecret(v.k.driver.Path(), v.k.driver.DeleteContext())

I was talking about GetContext and SetContext, but never mind as I see that also in the other implementations, we sometimes write with the variable name and sometimes without when we don't use the variable.

pkg/util/kms/kms_ibm_kp_storage.go

jackyalbo · 2024-03-11T10:06:39Z

pkg/util/kms/kms_azure.go

+		c[k] = v
+	}
+
+	// create TLS files out of secrets


Give some info about what is needed by the secrets module. What is this cert file and why did we create it also about the other config variables

We get below data as part of config map[string]string parameter. This details comes from connectiondetails section of noobaa CR (

noobaa-operator/pkg/util/kms/kms.go

Line 99 in 906a820

func NewKMS(connectionDetails map[string]string, tokenSecretName, name, namespace, uid string) (*KMS, error) {

)

ex: configmap data:

data: AZURE_CERT_SECRET_NAME: azure-ocs-ffwc9o1j AZURE_CLIENT_ID: az-client-id1 AZURE_TENANT_ID: az-tenant-id1 AZURE_VAULT_URL: az-valut-url1 KMS_PROVIDER: azure-kv KMS_SERVICE_NAME: kms-conn-azure1

vh05 · 2024-03-11T12:47:00Z

I think we need to add test for azure KMS, also I think we need some more info about what we expect the info sent by the NooBaa CR should look like, I mean what config info we need.

Below are the connection details that are going to be populated on Noobaa CR by noobaa_system_reconciler at ocs side and this is the ocs code where connectiondetails on Noobaa CR are built: https://github.com/red-hat-storage/ocs-operator/blob/2d082fc4c1ac4cec961406053cece448f4b07684/controllers/storagecluster/noobaa_system_reconciler.go#L249

ex: configmap data:

data:
  AZURE_CERT_SECRET_NAME: azure-ocs-ffwc9o1j
  AZURE_CLIENT_ID: az-client-id1
  AZURE_TENANT_ID: az-tenant-id1
  AZURE_VAULT_URL: az-valut-url1
  KMS_PROVIDER: azure-kv
  KMS_SERVICE_NAME: kms-conn-azure1

jackyalbo

I have some comment, the main issue I have is with AZURE_CERT_SECRET_NAME
which I don't understand if it needs to be in a file and where this requirement is coming for, what I did find in the secrets was AZURE_CLIENT_SECRET which I'm not sure if that't what you meant, and it looks like a regular string.

jackyalbo · 2024-03-14T08:06:41Z

pkg/util/kms/kms_azure.go

+	}
+
+	// create TLS files out of secrets
+	// replace in azure vaultConfig secret names with local temp files paths


from where do we know that we expect a file path and not just the string itself?

That is what I found in the secrets: https://github.com/libopenstorage/secrets/blob/5f4b25ceede180e8d67577288bba4223af658eb1/azure/azure_kv.go#L64
Am I missing something?

This explains what is not there in the existing libopenstorage/secrets implemenation and why the file path for secret certificate is needed in the comment: #1311 (comment)

jackyalbo · 2024-03-14T08:08:36Z

pkg/util/kms/kms_azure.go

+		c[k] = v
+	}
+
+	// create TLS files out of secrets


Does it really has to do with TLS? otherwise, I think we would expect a both key and cert.

In case of azure vault, as per our requirement we dont support secret as string. Only providing support for secret as certificate. Did I answer that ?

jackyalbo · 2024-03-14T08:33:20Z

pkg/util/kms/kms_azure.go

+// Config utils
+//
+
+// create temp files with TLS keys and certs


Not sure if it has to do with TLS, I would expect both key and cert in that case. maybe you just need to update the comment...

Yes. will update the comment. Thats misleading.

vh05 · 2024-03-14T09:03:36Z

I have some comment, the main issue I have is with AZURE_CERT_SECRET_NAME which I don't understand if it needs to be in a file and where this requirement is coming for, what I did find in the secrets was AZURE_CLIENT_SECRET which I'm not sure if that't what you meant, and it looks like a regular string.

There are 2 things.

libopenstorage/secrets currently support "secret" as string.
The requirement is to go for "secret" as certificate.

Currently libopenstorage/secrets dont support secret as certificate. We have (Member from Rook team, Santosh, working for azure kms on rook side and owning this azure kms story) pushed a patch to libopenstorage/sectes support secret certificate and that can be found here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R82-R93

With the certificate handling in place, we need to create a temp file and put the temp path in the map. This is used at libopenstrorage/secrets to create a service principle. The code for the same is here at: https://github.com/libopenstorage/secrets/pull/83/files#diff-feaeae3297b3a2250b55e5307f4f8ef82bf0c6032af4ffe0736c6814ec7855f5R33-R37

The path we store in the map is accessed at the libopenstorage/secrets at here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R74

I have imported the hash for the same in this PR which here: https://github.com/noobaa/noobaa-operator/pull/1311/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R5-R8

jackyalbo · 2024-03-14T13:22:16Z

I have some comment, the main issue I have is with AZURE_CERT_SECRET_NAME which I don't understand if it needs to be in a file and where this requirement is coming for, what I did find in the secrets was AZURE_CLIENT_SECRET which I'm not sure if that't what you meant, and it looks like a regular string.

There are 2 things.

libopenstorage/secrets currently support "secret" as string.

The requirement is to go for "secret" as certificate.

Currently libopenstorage/secrets dont support secret as certificate. We have (Member from Rook team, Santosh, working for azure kms on rook side and owning this azure kms story) pushed a patch to libopenstorage/sectes support secret certificate and that can be found here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R82-R93

With the certificate handling in place, we need to create a temp file and put the temp path in the map. This is used at libopenstrorage/secrets to create a service principle. The code for the same is here at: https://github.com/libopenstorage/secrets/pull/83/files#diff-feaeae3297b3a2250b55e5307f4f8ef82bf0c6032af4ffe0736c6814ec7855f5R33-R37

The path we store in the map is accessed at the libopenstorage/secrets at here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R74

I have imported the hash for the same in this PR which here: https://github.com/noobaa/noobaa-operator/pull/1311/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R5-R8

I'm a bit worried about us relying on a PR that is not yet merged, and not even properly reviewed...
Also think that maybe we should wait for the tests and testing infra before pushing this.
@liranmauda @nimrod-becker what do you think?

vh05 · 2024-03-15T05:52:45Z

I have some comment, the main issue I have is with AZURE_CERT_SECRET_NAME which I don't understand if it needs to be in a file and where this requirement is coming for, what I did find in the secrets was AZURE_CLIENT_SECRET which I'm not sure if that't what you meant, and it looks like a regular string.

There are 2 things.

libopenstorage/secrets currently support "secret" as string.

The requirement is to go for "secret" as certificate.

Currently libopenstorage/secrets dont support secret as certificate. We have (Member from Rook team, Santosh, working for azure kms on rook side and owning this azure kms story) pushed a patch to libopenstorage/sectes support secret certificate and that can be found here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R82-R93
With the certificate handling in place, we need to create a temp file and put the temp path in the map. This is used at libopenstrorage/secrets to create a service principle. The code for the same is here at: https://github.com/libopenstorage/secrets/pull/83/files#diff-feaeae3297b3a2250b55e5307f4f8ef82bf0c6032af4ffe0736c6814ec7855f5R33-R37
The path we store in the map is accessed at the libopenstorage/secrets at here: https://github.com/libopenstorage/secrets/pull/83/files#diff-91153976f4824b52d20fd598108e77d2a5d605f6b67dedec4f0ba53e70ca5e49R74
I have imported the hash for the same in this PR which here: https://github.com/noobaa/noobaa-operator/pull/1311/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R5-R8

I'm a bit worried about us relying on a PR that is not yet merged, and not even properly reviewed... Also think that maybe we should wait for the tests and testing infra before pushing this. @liranmauda @nimrod-becker what do you think?

Quick update to this. Rook is planning to have libopenstorage/secrets as a submodule (cloned) inside rook and merge the PR (rook/secrets#1) for azure certificate as of now and use it for the tech preview. Once the PR is merged at libopenstorage/secrets then we switch to that package.

The cloned branch (submodule at rook: https://github.com/rook/secrets) is libopenstorage/secrets master + PR (for azure certificate handling: rook/secrets#1)

The PR is merged at the rook side. This temp arrangement is required since the PR is not yet reviewed by libopenstorage team.

Changes at Noobaa side that facilitates the above explained approach: https://github.com/noobaa/noobaa-operator/pull/1311/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R5-R9

Your thoughts ?
@liranmauda @jackyalbo @nimrod-becker @dannyzaken

sp98 · 2024-03-15T06:05:30Z

I'm a bit worried about us relying on a PR that is not yet merged, and not even properly reviewed... Also think that maybe we should wait for the tests and testing infra before pushing this. @liranmauda @nimrod-becker what do you think?

Agree that we don't have it reviewed by the maintainers yet. Pinged them a few weeks ago and pinged on slack, but no response yet.

We have review comments from the our own team members that have been addressed.

For rook, we created a fork and plan to use that fork. This is temporary until the PR is reviewed by the library maintainer and merged.

Since the changes were made for azure module, I don't see it affecting the other modules (like hashicorp vault). But we should ensure that there are no issue is manual testing of this fork. We are ensuring that in Rook.

This patch provides the support for azure keyvault. We are using "libopenstorage/secrets" as the wrapper package to integrate with different kms and package provides the abstraction over several kms. It also provides the integraton support for azure and helps communication with azure key vault. We are required to provide the definition for house keeping calls registered calls with libopenstorage/secrets. "libopenstorage/secrets" does the creation of client handle based on the details provided in configmap. The certificate details present in the secret are preserved inside a temp file and used to establish the connection with azure key vault as of now. Below are the connection details that are going to be populated on Noobaa CR by `noobaa_system_reconciler` at ocs side and this is the ocs code where connectiondetails on Noobaa CR are built: https://github.com/red-hat-storage/ocs-operator/blob/2d082fc4c1ac4cec961406053cece448f4b07684/controllers/storagecluster/noobaa_system_reconciler.go#L249 ex: configmap data: ``` data: AZURE_CERT_SECRET_NAME: azure-ocs-ffwc9o1j AZURE_CLIENT_ID: az-client-id1 AZURE_TENANT_ID: az-tenant-id1 AZURE_VAULT_URL: az-valut-url1 KMS_PROVIDER: azure-kv KMS_SERVICE_NAME: kms-conn-azure1 ``` Signed-off-by: Vinayakswami Hariharmath <[email protected]>

pull-request-size bot added the size/L label Mar 4, 2024

vh05 marked this pull request as draft March 4, 2024 15:45

vh05 force-pushed the azure_kms branch 2 times, most recently from ad83444 to d948357 Compare March 5, 2024 07:55

liranmauda requested review from jackyalbo, liranmauda and dannyzaken March 5, 2024 09:17

vh05 force-pushed the azure_kms branch from d948357 to fb387d3 Compare March 5, 2024 14:33

pull-request-size bot added size/XXL and removed size/L labels Mar 5, 2024

vh05 force-pushed the azure_kms branch 2 times, most recently from da67397 to 2005ffb Compare March 11, 2024 06:51

vh05 marked this pull request as ready for review March 11, 2024 06:51

jackyalbo reviewed Mar 11, 2024

View reviewed changes

vh05 force-pushed the azure_kms branch 8 times, most recently from 067c6b1 to 114439b Compare March 13, 2024 09:27

vh05 requested a review from jackyalbo March 13, 2024 11:42

jackyalbo reviewed Mar 14, 2024

View reviewed changes

vh05 force-pushed the azure_kms branch from 114439b to ec23a17 Compare March 14, 2024 10:46

vh05 requested a review from jackyalbo March 14, 2024 10:49

vh05 force-pushed the azure_kms branch from ec23a17 to f94e3f9 Compare March 15, 2024 06:04

vh05 force-pushed the azure_kms branch 2 times, most recently from 3ce1f33 to a934974 Compare March 18, 2024 13:54

jackyalbo approved these changes Mar 19, 2024

View reviewed changes

vh05 force-pushed the azure_kms branch from a934974 to 68ad3ea Compare March 19, 2024 10:51

vh05 force-pushed the azure_kms branch from 68ad3ea to 68a0d2a Compare March 19, 2024 11:57

vh05 merged commit 4ee28d6 into noobaa:master Mar 19, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure kms handling for noobaa #1311

Azure kms handling for noobaa #1311

vh05 commented Mar 4, 2024 •

edited

Loading

jackyalbo left a comment

jackyalbo Mar 11, 2024

vh05 Mar 11, 2024

jackyalbo Mar 11, 2024

jackyalbo Mar 11, 2024

vh05 Mar 12, 2024 •

edited

Loading

jackyalbo Mar 14, 2024

jackyalbo Mar 11, 2024

vh05 Mar 11, 2024

vh05 commented Mar 11, 2024

jackyalbo left a comment

jackyalbo Mar 14, 2024

jackyalbo Mar 14, 2024

vh05 Mar 14, 2024

jackyalbo Mar 14, 2024

vh05 Mar 14, 2024 •

edited

Loading

jackyalbo Mar 14, 2024

vh05 Mar 14, 2024

vh05 commented Mar 14, 2024

jackyalbo commented Mar 14, 2024

vh05 commented Mar 15, 2024 •

edited

Loading

sp98 commented Mar 15, 2024

	func (*AzureVault) Config(config map[string]string, tokenSecretName, namespace string) (map[string]interface{}, error) {
	func (v *AzureVault) Config(config map[string]string, tokenSecretName, namespace string) (map[string]interface{}, error) {

	// Get implements SecretStorage interface for single string secret
	func (v *VersionSingleSecret) Get() error {
	s, err := v.k.GetSecret(v.k.driver.Path(), v.k.driver.GetContext())
	if err != nil {
	// handle k8s get from non-existent secret
	if strings.Contains(err.Error(), "not found") {
	return secrets.ErrInvalidSecretId
	}
	return err
	}

	value, ok := s[v.k.driver.Name()]
	if ok {
	switch value.(type) {
	case string:
	v.data = s[v.k.driver.Name()].(string)
	return nil
	}
	}
	return secrets.ErrInvalidSecretId
	}

	// Set implements SecretStorage interface for single string secret
	func (v *VersionSingleSecret) Set(val string) error {
	data := map[string]interface{}{
	v.k.driver.Name(): val,
	}

	v.data = val
	return v.k.PutSecret(v.k.driver.Path(), data, v.k.driver.SetContext())
	}

	// Delete implements SecretStorage interface for single string secret
	func (v *VersionSingleSecret) Delete() error {
	return v.k.DeleteSecret(v.k.driver.Path(), v.k.driver.DeleteContext())

Azure kms handling for noobaa #1311

Azure kms handling for noobaa #1311

Conversation

vh05 commented Mar 4, 2024 • edited Loading

jackyalbo left a 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

vh05 Mar 12, 2024 • 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

vh05 commented Mar 11, 2024

jackyalbo left a 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

vh05 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vh05 commented Mar 14, 2024

jackyalbo commented Mar 14, 2024

vh05 commented Mar 15, 2024 • edited Loading

sp98 commented Mar 15, 2024

vh05 commented Mar 4, 2024 •

edited

Loading

vh05 Mar 12, 2024 •

edited

Loading

vh05 Mar 14, 2024 •

edited

Loading

vh05 commented Mar 15, 2024 •

edited

Loading