Removed DBToken. Made the components uses Managed Identity #3584

anshulvermapatel · 2024-05-16T16:42:17Z

Which issue this PR addresses:

As part of the ARO-5512 , we needed to disable local auth for cosmosdb. In order to disable local authentication, this PR is to make our components rely on managed identities/entra id rather than keys for authentication/authorizing cosmosdb.

This is currently a draft PR as I will be testing it in the INT env first before going forward, however, reviews are appreciated.

I have manually changed some generated code here. That was because the PR I created to have the generated code changed is not merged yet. Once that is merged, I will generate the code from the go-cosmosdb repo itself just like how we usually do.

Fixes
ARO-7195
ARO-7314

github-actions · 2024-05-17T10:49:30Z

Please rebase pull request.

bennerv

Also need to remove:

rm docs/dbtoken-service.md
The load balancing rule to allow gateway to talk to the dbtoken service from pkg/deploy (should be in templates)

We probably don't need to peer the gateway and RP vnets anymore. We might be able to remove that peering as well. Which would be good for overall security posture.

bennerv · 2024-05-17T12:07:03Z

pkg/database/billing.go

+		if err != nil {
+			return nil, err
+		}
+		_, err = poller.PollUntilDone(ctx, nil)
+		if err != nil {


the error logic handling is changing here. Should it be? Should we only ignore errors of http.StatusConflict from cosmosdb?

So this method is to create/update so if there is already something there, it will just make an update which would be nothing if the triggers are same. I ran this multiple times on the same cosmosdb database and it didn't return http.StatusConflict even when the triggers were already there.

bennerv · 2024-05-17T12:08:30Z

pkg/database/billing.go

+		ctx, cancel := context.WithTimeout(ctx, time.Minute*5)
+		defer cancel()


Do we have an over-arching context assocaited with this function already? is the old method triggerc.Create(ctx, trigger) blocking vs now we have to poll?

Previously, we used to get access to create/modify triggers using the keys, however now since we are using managed identities at cosmosdb scope, we cannot do any management operations on triggers(as it being management resource) because for that, the request must go through ARM and not directly to database.
https://learn.microsoft.com/en-us/azure/cosmos-db/how-to-setup-rbac#permission-model

bennerv · 2024-05-17T12:10:22Z

pkg/database/database.go

@@ -71,6 +73,22 @@ func NewMasterKeyAuthorizer(ctx context.Context, log *logrus.Entry, token azcore
 	return cosmosdb.NewMasterKeyAuthorizer(getDatabaseKey(keys, log))
 }

+func NewTokenAuthorizer(ctx context.Context, log *logrus.Entry, cred azcore.TokenCredential, databaseAccountName string, scopes []string) (cosmosdb.Authorizer, error) {
+	scopes = append(scopes, fmt.Sprintf("https://%s.documents.azure.com/.default", databaseAccountName))


This scope is incorrect for government cloud (and any non-Azure Public Cloud)

We should read the url from the env struct we use everywhere to be cloud aware.

https://learn.microsoft.com/en-us/azure/azure-government/compare-azure-government-global-azure

Gotcha! .. sorry for that, I'll correct it.

pkg/database/openshiftclusters.go

pkg/database/subscriptions.go

pkg/deploy/devconfig.go

bennerv · 2024-05-17T12:19:43Z

pkg/database/database.go

@@ -71,6 +73,22 @@ func NewMasterKeyAuthorizer(ctx context.Context, log *logrus.Entry, token azcore
 	return cosmosdb.NewMasterKeyAuthorizer(getDatabaseKey(keys, log))
 }

+func NewTokenAuthorizer(ctx context.Context, log *logrus.Entry, cred azcore.TokenCredential, databaseAccountName string, scopes []string) (cosmosdb.Authorizer, error) {


I think there's a problem inherently with this function.

The token we request from AAD with the scope is a short-lived credential (~12 hrs max I believe). That means that at some point, the token will expire both from AAD MSI perspective and cosmosdb perspective. As a result, we may need some refreshing of the actual database client in all places where it's used.

Previously, we used the primary database keys which never expire but are rotatable. That's why we didn't need to implement any database client reconstruction due to token expiration. Now we more than likely do.

The best way to test this would be to run an RP for a long time (or get the expiration of the token from this function) and see what happens when you get past the expiration time.

The azcore.TokenCredential, it is indeed an object for tokens but it does not statically contains the token. It has a getToken() method which gives a refreshed token every time we call the method.
There is a token refresh logic in place in this go-cosmosdb code in this PR which cache the token. Every time we make a request, it is check is the current token is expired or 5 mins left to expire, if so it calls getToken() to get an updated token else use the cached ones.

Is it possible on quieter regions that we might not have a request for over 12 hours? Would that have any effect?

No, because everything a request goes to cosmosdb, there's a method Authorize() which gets called, and this method has the logic to acquire the token if the token is expired. The same logic which we were discussing lately.

anshulvermapatel · 2024-05-22T13:18:51Z

/azp run ci,e2e

azure-pipelines · 2024-05-22T13:18:57Z

Pull request contains merge conflicts.

github-actions · 2024-05-22T13:19:50Z

Please rebase pull request.

jaitaiwan

A few small comments. Nothing critical. Seems like this PR really needs folks to manually test - is there instructions for how one might do this?

jaitaiwan · 2024-05-27T23:38:14Z

docs/deploy-full-rp-service-in-dev.md

Are there any changes in this file that aren't formatting? A quick review seemed like there wasn't any meaningful changes to this file which make me think it should be removed from the PR.

Ahh, I think prettier plugin changed this file. I will remove this changes.
I did remove this portion though:

az keyvault certificate import
--vault-name "$KEYVAULT_PREFIX-dbt"
--name dbtoken-server
--file secrets/localhost.pem >/dev/null

jaitaiwan · 2024-05-27T23:42:49Z

pkg/database/database.go

@@ -71,6 +73,22 @@ func NewMasterKeyAuthorizer(ctx context.Context, log *logrus.Entry, token azcore
 	return cosmosdb.NewMasterKeyAuthorizer(getDatabaseKey(keys, log))
 }

+func NewTokenAuthorizer(ctx context.Context, log *logrus.Entry, cred azcore.TokenCredential, databaseAccountName string, scopes []string) (cosmosdb.Authorizer, error) {


Is it possible on quieter regions that we might not have a request for over 12 hours? Would that have any effect?

jaitaiwan · 2024-05-27T23:43:30Z

pkg/deploy/deploy.go

+	globalaccounts    storage.AccountsClient
+	deployments       features.DeploymentsClient
+	groups            features.ResourceGroupsClient
+	// loadbalancers          network.LoadBalancersClient


We should just remove this entirely I think

remove what? line 52 // loadbalancers network.LoadBalancersClient ?
I was not sure about if this is affecting something or not, so I just commented it out for now. I will remove that as well once I have more peer reviews on this.

jaitaiwan · 2024-05-27T23:43:43Z

pkg/deploy/deploy.go

+		globalaccounts:    storage.NewAccountsClient(_env.Environment(), *config.Configuration.GlobalSubscriptionID, authorizer),
+		deployments:       features.NewDeploymentsClient(_env.Environment(), config.SubscriptionID, authorizer),
+		groups:            features.NewResourceGroupsClient(_env.Environment(), config.SubscriptionID, authorizer),
+		// loadbalancers:          network.NewLoadBalancersClient(_env.Environment(), config.SubscriptionID, authorizer),


We should just remove this entirely I think

anshulvermapatel · 2024-05-28T06:23:55Z

/azp run ci,e2e

azure-pipelines · 2024-05-28T06:24:07Z

Azure Pipelines successfully started running 2 pipeline(s).

niontive · 2024-05-29T19:14:27Z

Regarding impact for billing service:

Billing uses master key authorization and doesn't go through DB token service. Consequently, we can remove db token service and not affect billing.
Billing is using an older ARO-RP module release, so we can update the latest one and not affect billing.
For rollout, don't disable local auth for CosmosDB. We'll migrate billing service to not use master key authorization, rollout billing, then we should be good to disable local auth for Cosmos DB.

anshulvermapatel requested review from jewzaam, bennerv, hawkowl, rogbas, petrkotas, jharrington22, cblecker, cadenmarchese, UlrichSchlueter, s-amann, SudoBrendan, Shivkumar13, yjst2012, jaitaiwan, hlipsig and tiguelu as code owners May 16, 2024 16:42

anshulvermapatel marked this pull request as draft May 16, 2024 16:42

github-actions bot added the needs-rebase branch needs a rebase label May 17, 2024

anshulvermapatel force-pushed the anshulvermapatel/ARO-7195 branch from 4156bcd to f89ebdd Compare May 17, 2024 11:11

github-actions bot removed the needs-rebase branch needs a rebase label May 17, 2024

anshulvermapatel force-pushed the anshulvermapatel/ARO-7195 branch from f89ebdd to 4bd69b9 Compare May 17, 2024 11:27

bennerv requested changes May 17, 2024

View reviewed changes

github-actions bot added the needs-rebase branch needs a rebase label May 22, 2024

anshulvermapatel force-pushed the anshulvermapatel/ARO-7195 branch from 4bd69b9 to 20a5400 Compare May 22, 2024 15:37

github-actions bot removed the needs-rebase branch needs a rebase label May 22, 2024

anshulvermapatel added the firefly Issues or Pull requests owned by Team Firefly label May 27, 2024

anshulvermapatel marked this pull request as ready for review May 27, 2024 14:37

anshulvermapatel requested a review from bennerv May 27, 2024 17:12

anshulvermapatel added the ready-for-review label May 27, 2024

jaitaiwan requested changes May 27, 2024

View reviewed changes

anshulvermapatel added 5 commits May 28, 2024 13:03

removed generate issues

3f6768e

fixed usgov scope thing, also changed few things.

accbeee

Previously missed changing triggers logic for monitor, added it

84fc4a5

generate issue

5f5e322

removed all the references of dbtoken

97d99c6

anshulvermapatel force-pushed the anshulvermapatel/ARO-7195 branch 3 times, most recently from f691b3b to d507229 Compare May 28, 2024 13:30

createUpdateTriggers were taking time. Reduced poll fequency time

5e3fecd

anshulvermapatel force-pushed the anshulvermapatel/ARO-7195 branch from d507229 to 5e3fecd Compare May 28, 2024 13:31

added logic to create the role assigments while deploying

dc31785

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed DBToken. Made the components uses Managed Identity #3584

Removed DBToken. Made the components uses Managed Identity #3584

anshulvermapatel commented May 16, 2024 •

edited

github-actions bot commented May 17, 2024

bennerv left a comment

bennerv May 17, 2024

anshulvermapatel May 22, 2024

bennerv May 17, 2024

anshulvermapatel May 22, 2024

bennerv May 17, 2024

anshulvermapatel May 22, 2024

bennerv May 17, 2024

anshulvermapatel May 22, 2024

jaitaiwan May 27, 2024

anshulvermapatel May 28, 2024

anshulvermapatel commented May 22, 2024

azure-pipelines bot commented May 22, 2024

github-actions bot commented May 22, 2024

jaitaiwan left a comment

jaitaiwan May 27, 2024

anshulvermapatel May 28, 2024

jaitaiwan May 27, 2024

jaitaiwan May 27, 2024

anshulvermapatel May 28, 2024

jaitaiwan May 27, 2024

anshulvermapatel May 28, 2024

anshulvermapatel commented May 28, 2024

azure-pipelines bot commented May 28, 2024

niontive commented May 29, 2024

		ctx, cancel := context.WithTimeout(ctx, time.Minute*5)
		defer cancel()

Removed DBToken. Made the components uses Managed Identity #3584

Are you sure you want to change the base?

Removed DBToken. Made the components uses Managed Identity #3584

Conversation

anshulvermapatel commented May 16, 2024 • edited

Which issue this PR addresses:

github-actions bot commented May 17, 2024

bennerv 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

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

anshulvermapatel commented May 22, 2024

azure-pipelines bot commented May 22, 2024

github-actions bot commented May 22, 2024

jaitaiwan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshulvermapatel commented May 28, 2024

azure-pipelines bot commented May 28, 2024

niontive commented May 29, 2024

anshulvermapatel commented May 16, 2024 •

edited