migrate from adal to azidentity #83

sp98 · 2024-02-15T08:45:17Z

adal is out of support since March 31, 2023.
This PR migrates from adal to azidentity for azure key vault.
Migration guide was used as a reference.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional)
Closes #84

Special notes for your reviewer:

sp98 · 2024-03-04T11:19:44Z

@dahuang-purestorage Hi, can you please take a look at this PR. The change to support certificates based auth was very small to included in the same PR. If you want, I can open a different PR for that as well. Thanks.

cc @adityadani

azure/azure_kv.go

BlaineEXE · 2024-03-04T21:00:36Z

azure/azure_kv.go

+	if strings.EqualFold(cloudEnv, "AzureUSGovernment") {
+		opts.Cloud = cloud.AzureGovernment
+	} else if strings.EqualFold(cloudEnv, "AzureChinaCloud") {
+		opts.Cloud = cloud.AzureChina
+	} else {
+		opts.Cloud = cloud.AzurePublic
+	}


Do we need to do this? Can we not pass a user-defined string to the libopenstorage/secrets lib here so that Rook doesn't need to do custom processing?

The current version in the master is using this. So if we don't do this it might be a breaking change for users.

I'll check again if we really need this or not.

Maybe we can add this logic or these "AzureUSGovernment"/"AzureChinaCloud" strings to the libopenstorage library. But in general, I am concerned about hard coding these values.

It's entirely possible and reasonable that Azure might add other clouds like "AzureEuropeCloud", "AzureRussiaCloud", or any other reasonably big use-case where data security needs to be different from other clouds, and if we force any unrecognized option to use the public cloud, those users might be broken. It seems best to me just to pass in the value given to Rook into the lib without trying to validate it.

I was using AzurePublicCloud for any unrecognized option. That's wrong. I've updated to use only AzurePublicCloud only if the user has set it or if user does not provide an option.

It seems best to me just to pass in the value given to Rook into the lib without trying to validate it.

I think this would result in breaking changes to the users already using this library. Because they are only expected to provide the AZURE_ENVIRONMENT (string) and then libopenshift/secrets finds out the exact environment based on the AZURE_ENVIRONMENT

iPraveenParihar · 2024-03-08T10:07:50Z

azure/azure_kv.go

@@ -109,7 +132,7 @@ func (az *azureSecrets) GetSecret(
 		return nil, secrets.NoVersion, err
 	}

-	secretResp, ok := resp.(keyvault.SecretBundle)
+	secretResp, ok := resp.(azsecrets.GetSecretResponse)


Azure now returns secret version.
can we return version as well?

secretResp.ID.Version()

No other KMS in the library is returning the version currently. So I think for now, we can keep it as it is and revisit this later.

AWS Secrets Manager returns the version.The only reason all providers have not been updated to return the version is lack of bandwidth to check and test if each one of them If we are modifying this provider and you are able to verify its correctness we can modify it

azure/azure_kv.go

adal is is out of support since March 31, 2023. This PR migrates from adal to azidentity for azure key vault Signed-off-by: sp98 <[email protected]>

adityadani

Can you post the output of your azure_integration_tests.go results here?

adityadani · 2024-04-11T18:27:32Z

azure/azure_kv.go

+			// don't retry if the secret key was not found
+			responseError, ok := err.(*azcore.ResponseError)
+			if ok && responseError.StatusCode == 404 {
+				return nil, false, secrets.ErrSecretNotFound


Can you return this error instead. - ErrInvalidSecretId since all the other providers are returning it when a secret is not found.

@adityadani Sorry for the delay. I'll update the PR this week.

adityadani · 2024-04-11T18:31:41Z

azure/azure_kv.go

@@ -109,7 +132,7 @@ func (az *azureSecrets) GetSecret(
 		return nil, secrets.NoVersion, err
 	}

-	secretResp, ok := resp.(keyvault.SecretBundle)
+	secretResp, ok := resp.(azsecrets.GetSecretResponse)


AWS Secrets Manager returns the version.The only reason all providers have not been updated to return the version is lack of bandwidth to check and test if each one of them If we are modifying this provider and you are able to verify its correctness we can modify it

sp98 · 2024-09-03T10:20:21Z

@adityadani Hi. This got delayed a bit due to other commitments. I plan to address your review comments this week. Hope we can get this merged soon. Thanks.

sp98 changed the title ~~migrate from adal to azidentity~~ [WIP] migrate from adal to azidentity Feb 15, 2024

sp98 force-pushed the use-azidentity branch 2 times, most recently from b0d2f1d to b33f0b1 Compare February 20, 2024 07:24

sp98 changed the title ~~[WIP] migrate from adal to azidentity~~ migrate from adal to azidentity Feb 20, 2024

sp98 changed the title ~~migrate from adal to azidentity~~ [WIP]migrate from adal to azidentity Feb 20, 2024

sp98 force-pushed the use-azidentity branch 3 times, most recently from 3c002ec to a202e93 Compare March 4, 2024 10:59

sp98 changed the title ~~[WIP]migrate from adal to azidentity~~ migrate from adal to azidentity Mar 4, 2024

BlaineEXE reviewed Mar 4, 2024

View reviewed changes

azure/azure_kv.go Show resolved Hide resolved

BlaineEXE reviewed Mar 4, 2024

View reviewed changes

azure/azure_kv.go Outdated Show resolved Hide resolved

BlaineEXE reviewed Mar 4, 2024

View reviewed changes

sp98 force-pushed the use-azidentity branch 3 times, most recently from 8f7772b to 5a08606 Compare March 7, 2024 06:14

sp98 requested review from BlaineEXE and iPraveenParihar March 7, 2024 06:14

iPraveenParihar reviewed Mar 8, 2024

View reviewed changes

azure/azure_kv.go Outdated Show resolved Hide resolved

sp98 requested a review from iPraveenParihar March 11, 2024 12:32

sp98 mentioned this pull request Mar 14, 2024

support using certs for authenticating azure kv #82

Closed

migrate from adal to azidentity

b78d7f0

adal is is out of support since March 31, 2023. This PR migrates from adal to azidentity for azure key vault Signed-off-by: sp98 <[email protected]>

sp98 force-pushed the use-azidentity branch from 5a08606 to b78d7f0 Compare March 14, 2024 01:13

parth-gr mentioned this pull request Mar 18, 2024

core: azure kms support rook/rook#13852

Merged

9 tasks

adityadani reviewed Apr 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate from adal to azidentity #83

migrate from adal to azidentity #83

sp98 commented Feb 15, 2024 •

edited

Loading

sp98 commented Mar 4, 2024 •

edited

Loading

BlaineEXE Mar 4, 2024

sp98 Mar 5, 2024

BlaineEXE Mar 5, 2024

sp98 Mar 6, 2024

sp98 Mar 6, 2024 •

edited

Loading

iPraveenParihar Mar 8, 2024 •

edited

Loading

sp98 Mar 11, 2024

adityadani Apr 11, 2024

adityadani left a comment

adityadani Apr 11, 2024

sp98 May 2, 2024

adityadani Apr 11, 2024

sp98 commented Sep 3, 2024

migrate from adal to azidentity #83

Are you sure you want to change the base?

migrate from adal to azidentity #83

Conversation

sp98 commented Feb 15, 2024 • edited Loading

sp98 commented Mar 4, 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

sp98 Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

iPraveenParihar Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityadani 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

sp98 commented Sep 3, 2024

sp98 commented Feb 15, 2024 •

edited

Loading

sp98 commented Mar 4, 2024 •

edited

Loading

sp98 Mar 6, 2024 •

edited

Loading

iPraveenParihar Mar 8, 2024 •

edited

Loading