Validate ACR token validity after token password rotation #3059

ventifus · 2023-07-24T21:49:01Z

Which issue this PR addresses:

What this PR does / why we need it:

When we rotate ACR tokens via PUCM, it is possible for the token to end up in an ambiguous state, possibly due to a race condition. See slack thread for more context: https://redhat-internal.slack.com/archives/C02ULBRS68M/p1689709898669219

Test plan for issue:

TODO

Is there any documentation that needs to be updated for this PR?

TODO

pkg/cluster/acrtoken.go

azure-pipelines · 2023-07-26T16:24:05Z

No commit pushedDate could be found for PR 3059 in repo Azure/ARO-RP

pkg/cluster/adminupdate_test.go

dem4gus

I understand the desire to separate the token validation from the pull-secret controller's reconciliation process, which can possibly error for unrelated reasons, but we will need to keep in mind that what we're validating here isn't actually the thing the cluster is using to authenticate to the container registry. Ideally it eventually becomes that (or part of it), but there is much more that goes into the pull secret during the controller's reconciliation that could affect the final pull secret. Getting it from here is probably a better option though, because there's no chance of accessing customer secrets that could also be on the pull-secret object.

We will also want to keep in mind that we're never revalidating the token, which may become a problem if (or when) password expirations are introduced to the process. However, that is something we can revisit when the time comes.

dem4gus · 2023-07-27T16:59:00Z

pkg/util/acrtoken/acrtoken.go

+		tokens:             containerregistry.NewTokensClient(env.Environment(), r.SubscriptionID, localFPAuthorizer),
+		registries:         containerregistry.NewRegistriesClient(env.Environment(), r.SubscriptionID, localFPAuthorizer),
+		newAzAcrClient:     azcontainerregistry.NewClient,
+		getTokenCredential: clusterauthorizer.GetTokenCredential,


From what I can tell this clusterauthorizer.GetTokenCredential function is for getting an Azure OAuth token, which is not how the clusters authenticate with the container registry. The tokens that is being updated here and that is used in the pull secret is a Basic Auth token in .dockerconfigjson format, and the credentials live in the cluster itself.

dem4gus · 2023-07-27T17:05:19Z

pkg/util/acrtoken/acrtoken.go

+func (m *manager) ValidateToken(ctx context.Context, rp *api.RegistryProfile) error {
+	creds := clusterauthorizer.Credentials{
+		ClientID:     []byte(rp.Username),
+		ClientSecret: []byte(rp.Password),


It looks like you're using the registry profile that's stored on the cluster doc in CosmosDB to get the token password. I chose not to even worry about updating that password during the rotation process because the only time the cluster cares about that password during the initial cluster installation — that's how the RP passes the username (token name) and password to the cluster during the bootstrap process.

The cluster's "source of truth" for its current ACR password is the cluster secret in openshift-azure-operator. That's what it uses during the pullsecret controller's reconciliation to ultimately construct the pull secret that nodes use to get container images. The cluster never accesses the password on the cluster document after installation, so I opted not to as well in order to retain a single source of truth for the password.

As far as testing the validity of the new credentials, it would make sense to me to use the exact same credentials the cluster uses in order to minimize potential differences in data. That means getting the secret from the cluster instead of the RP, and deserializing the token stored on it.

dem4gus · 2023-07-27T17:12:56Z

pkg/util/acrtoken/acrtoken.go

+		return err
+	}
+
+	client, err := m.newAzAcrClient(m.env, fmt.Sprintf("https://%s", m.env.ACRDomain()), token, nil)


The container registry's URL is stored as part of the .dockerconfigjson string. You should be able to use that to get a connection to the ACR without having to use the environment interface at all.

github-actions · 2023-08-22T09:19:31Z

Please rebase pull request.

cadenmarchese

I know this is still in draft, but I thought I'd leave some thoughts on the logic. Thanks for your efforts so far!

pkg/cluster/acrtoken.go

pkg/util/acrtoken/acrtoken.go

cadenmarchese

The ValidatePullSecret logic looks good to me! I think the only thing to iron out is when exactly to run it. Right now, were running it on PUCM as well as in the operator, which I think I agree with because we'd get both an operator status and a PUCM failure, but I can see why we'd only really need one or the other since we're always doing the rotation.

pkg/cluster/adminupdate_test.go

pkg/operator/controllers/pullsecret/pullsecret_controller.go

pkg/util/pullsecret/pullsecret.go

cadenmarchese

Few small comments, but for the most part, looking good!

pkg/cluster/acrtoken.go

cadenmarchese · 2023-12-05T21:18:14Z

pkg/cluster/adminupdate_test.go

@@ -130,7 +130,7 @@ func TestAdminUpdateSteps(t *testing.T) {
 				"[Action fixMCSCert-fm]",
 				"[Action fixMCSUserData-fm]",
 				"[Action ensureGatewayUpgrade-fm]",
-				"[Action rotateACRTokenPassword-fm]",
+				"[Condition rotateAndValidateACRTokenPassword-fm, timeout 5m0s]",


Since we aren't waiting on a reconciliation here, do you think we should just make this an Action with no timeout instead of leaving it as Condition?

cadenmarchese · 2023-12-05T21:20:24Z

pkg/util/pullsecret/pullsecret.go

+		if err != nil {
+			return err
+		}


Redundant? It looks like we already returned any possible error on line 198.

cadenmarchese · 2023-12-05T21:21:24Z

pkg/util/pullsecret/pullsecret.go

+			err = fmt.Errorf("credentials format error: %s", registry)
+			return err


Nit - might as well consolidate these lines since you did it on line 205:

Suggested change

err = fmt.Errorf("credentials format error: %s", registry)

return err

return fmt.Errorf("credentials format error: %s", registry)

dem4gus

Some questions about which tokens we're validating in the pull secret.

dem4gus · 2023-12-12T16:02:50Z

pkg/util/pullsecret/pullsecret.go

+	if err != nil {
+		return err
+	}
+	for registry, authBase64 := range dockerConfig {


This will check every registry in openshift-config/pull-secret, including user-defined credentials. Are we okay with doing that? And if we are, errors returned from this loop will short-circuit the validation. For example, if there is a pull-secret with both a user-defined registry and the ARO ACR defined and the user secret is checked first and has some sort of error, this will early return on that error and the ARO ACR token will never be validated. I don't know how we feel about using goroutines to check credentials concurrently, but since one credential's validity shouldn't have a bearing on another cred that may be a solution. Alternatively, we could extract the ARO ACR token and only check that one.

I'm +1 to extracting the ARO ACR token for validation and disregarding the other tokens.

dem4gus · 2023-12-12T16:10:03Z

pkg/util/pullsecret/pullsecret.go

+			err = fmt.Errorf("credentials format error: %s", registry)
+			return err


dem4gus · 2023-12-12T16:12:53Z

pkg/util/pullsecret/pullsecret_test.go

+		wantAuth map[string]string
+		wantErr  string
+		client   RegistryClient
+	}{


If we're going to validate all tokens in the secret then we'll need a test case for a valid ACR token being checked after an invalid user token. I don't remember if go slices are guaranteed to be handled in order in a for loop but it would be good at least attempt it.

kimorris27 · 2023-12-15T16:26:14Z

pkg/cluster/install.go

@@ -102,7 +102,7 @@ func (m *manager) adminUpdate() []steps.Step {
 	if isEverything {
 		toRun = append(toRun,
 			steps.Action(m.ensureGatewayUpgrade),
-			steps.Action(m.rotateACRTokenPassword),
+			steps.Condition(m.rotateAndValidateACRTokenPassword, 5*time.Minute, true),


Don't we need to ensure the operator is running before moving forward with the rotation? You can make sure the operator is ready using the aroDeploymentReady Condition function.

kimorris27 · 2023-12-15T16:26:40Z

pkg/cluster/install.go

@@ -195,7 +195,7 @@ func (m *manager) Update(ctx context.Context) error {
 		steps.Action(m.createOrUpdateDenyAssignment),
 		steps.Action(m.startVMs),
 		steps.Condition(m.apiServersReady, 30*time.Minute, true),
-		steps.Action(m.rotateACRTokenPassword),
+		steps.Condition(m.rotateAndValidateACRTokenPassword, 5*time.Minute, true),


Same question/comment as in adminUpdate.

kimorris27 · 2023-12-15T16:47:42Z

pkg/util/pullsecret/pullsecret.go

+	if err != nil {
+		return err
+	}
+	for registry, authBase64 := range dockerConfig {


I'm +1 to extracting the ARO ACR token for validation and disregarding the other tokens.

github-actions · 2024-01-04T05:01:39Z

Please rebase pull request.

…RTokenPassword

github-actions · 2024-05-07T19:24:46Z

Please rebase pull request.

mociarain · 2024-07-09T08:47:20Z

What's the current state of this and can I help get it moving again?

github-advanced-security bot found potential problems Jul 24, 2023

View reviewed changes

pkg/cluster/acrtoken.go Fixed Show resolved Hide resolved

cadenmarchese added the chainsaw Pull requests or issues owned by Team Chainsaw label Jul 25, 2023

ventifus force-pushed the validate-acr-token branch 3 times, most recently from cf58141 to 341d000 Compare July 25, 2023 22:28

cadenmarchese reviewed Jul 26, 2023

View reviewed changes

pkg/cluster/adminupdate_test.go Outdated Show resolved Hide resolved

dem4gus reviewed Jul 27, 2023

View reviewed changes

github-actions bot added the needs-rebase branch needs a rebase label Aug 22, 2023

ventifus force-pushed the validate-acr-token branch from 341d000 to 4151458 Compare October 3, 2023 22:53

github-actions bot removed the needs-rebase branch needs a rebase label Oct 3, 2023

ventifus force-pushed the validate-acr-token branch from 4151458 to 023cece Compare October 3, 2023 23:40

cadenmarchese reviewed Oct 6, 2023

View reviewed changes

pkg/cluster/acrtoken.go Outdated Show resolved Hide resolved

pkg/cluster/acrtoken.go Outdated Show resolved Hide resolved

pkg/cluster/acrtoken.go Outdated Show resolved Hide resolved

pkg/cluster/acrtoken.go Show resolved Hide resolved

ventifus force-pushed the validate-acr-token branch from 023cece to 0e29cc2 Compare October 23, 2023 23:40

github-advanced-security bot found potential problems Oct 23, 2023

View reviewed changes

pkg/util/acrtoken/acrtoken.go Fixed Show fixed Hide fixed

ventifus force-pushed the validate-acr-token branch 6 times, most recently from 642f582 to 81064e4 Compare October 24, 2023 21:53

cadenmarchese reviewed Nov 1, 2023

View reviewed changes

pkg/cluster/adminupdate_test.go Outdated Show resolved Hide resolved

pkg/operator/controllers/pullsecret/pullsecret_controller.go Outdated Show resolved Hide resolved

pkg/util/pullsecret/pullsecret.go Outdated Show resolved Hide resolved

ventifus force-pushed the validate-acr-token branch from d5583ee to 48c82ff Compare November 17, 2023 16:18

ventifus marked this pull request as ready for review November 27, 2023 18:51

ventifus requested review from jewzaam, bennerv, hawkowl, rogbas and petrkotas as code owners November 27, 2023 18:51

ventifus requested review from jharrington22, cblecker, UlrichSchlueter, s-amann, SudoBrendan, Shivkumar13, yjst2012, anshulvermapatel and hlipsig as code owners November 27, 2023 18:51

cadenmarchese reviewed Dec 5, 2023

View reviewed changes

cadenmarchese added the ready-for-review label Dec 12, 2023

dem4gus reviewed Dec 12, 2023

View reviewed changes

kimorris27 reviewed Dec 15, 2023

View reviewed changes

github-actions bot added needs-rebase branch needs a rebase and removed ready-for-review labels Jan 4, 2024

ventifus requested review from jaitaiwan and dofinn as code owners February 7, 2024 23:42

ventifus force-pushed the validate-acr-token branch 3 times, most recently from cb8ec65 to 55611b5 Compare February 8, 2024 00:34

ventifus added 4 commits February 7, 2024 16:41

Validate ACR token

31d2980

Change Action rotateACRTokenPassword to Condition rotateAndValidateAC…

7d22769

…RTokenPassword

Enable setting degraded condition on validation failure

80826dd

Rebased

3ac820c

ventifus force-pushed the validate-acr-token branch from 55611b5 to 3ac820c Compare February 8, 2024 00:46

github-actions bot removed the needs-rebase branch needs a rebase label Feb 8, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate ACR token validity after token password rotation #3059

Validate ACR token validity after token password rotation #3059

ventifus commented Jul 24, 2023

azure-pipelines bot commented Jul 26, 2023

dem4gus left a comment

dem4gus Jul 27, 2023

dem4gus Jul 27, 2023

dem4gus Jul 27, 2023

github-actions bot commented Aug 22, 2023

cadenmarchese left a comment

cadenmarchese left a comment

cadenmarchese left a comment

cadenmarchese Dec 5, 2023

cadenmarchese Dec 5, 2023

cadenmarchese Dec 5, 2023

dem4gus Dec 12, 2023

dem4gus left a comment

dem4gus Dec 12, 2023

kimorris27 Dec 15, 2023

dem4gus Dec 12, 2023

dem4gus Dec 12, 2023

kimorris27 Dec 15, 2023

kimorris27 Dec 15, 2023

kimorris27 Dec 15, 2023

github-actions bot commented Jan 4, 2024

github-actions bot commented May 7, 2024

mociarain commented Jul 9, 2024

		err = fmt.Errorf("credentials format error: %s", registry)
		return err

	err = fmt.Errorf("credentials format error: %s", registry)
	return err
	return fmt.Errorf("credentials format error: %s", registry)

Validate ACR token validity after token password rotation #3059

Are you sure you want to change the base?

Validate ACR token validity after token password rotation #3059

Conversation

ventifus commented Jul 24, 2023

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

azure-pipelines bot commented Jul 26, 2023

dem4gus 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

github-actions bot commented Aug 22, 2023

cadenmarchese left a comment

Choose a reason for hiding this comment

cadenmarchese left a comment

Choose a reason for hiding this comment

cadenmarchese 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

dem4gus 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

github-actions bot commented Jan 4, 2024

github-actions bot commented May 7, 2024

mociarain commented Jul 9, 2024