Skip to content

Commit

Permalink
Validate ACR token
Browse files Browse the repository at this point in the history
  • Loading branch information
ventifus committed Oct 23, 2023
1 parent 082e9a5 commit 0e29cc2
Show file tree
Hide file tree
Showing 7 changed files with 150 additions and 1 deletion.
75 changes: 74 additions & 1 deletion pkg/cluster/acrtoken.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@ package cluster

import (
"context"
"encoding/base64"
"fmt"
"strings"
"time"

dockerregistry "github.com/containers/image/v5/docker"
corev1 "k8s.io/api/core/v1"
kerrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -86,6 +90,10 @@ func (m *manager) rotateACRTokenPassword(ctx context.Context) error {
if err != nil {
return err
}
err = token.ValidateToken(ctx, registryProfile)
if err != nil {
return err
}

m.doc, err = m.db.PatchWithLease(ctx, m.doc.Key, func(doc *api.OpenShiftClusterDocument) error {
token.PutRegistryProfile(doc.OpenShiftCluster, registryProfile)
Expand Down Expand Up @@ -116,8 +124,18 @@ func (m *manager) rotateACRTokenPassword(ctx context.Context) error {
if err != nil {
return err
}

err = m.validateACRToken(ctx)
if err != nil {
return err
}
err = retryOperation(func() error {
return m.rotateOpenShiftConfigSecret(ctx, pullSecret.Data[corev1.DockerConfigJsonKey])
err := m.rotateOpenShiftConfigSecret(ctx, pullSecret.Data[corev1.DockerConfigJsonKey])
if err != nil {
return err
}
err = m.validateACRToken(ctx)
return err
})
if err != nil {
return err
Expand Down Expand Up @@ -162,8 +180,17 @@ func (m *manager) rotateOpenShiftConfigSecret(ctx context.Context, encodedDocker
}
}

err = m.validateACRToken(ctx)
if err != nil {
return err
}

return retryOperation(func() error {
_, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Apply(ctx, applyConfiguration, metav1.ApplyOptions{FieldManager: "aro-rp", Force: true})
if err != nil {
return err
}
err = m.validateACRToken(ctx)
return err
})
}
Expand All @@ -176,3 +203,49 @@ func retryOperation(retryable func() error) error {
return kerrors.IsBadRequest(err) || kerrors.IsInternalError(err) || kerrors.IsServerTimeout(err) || kerrors.IsConflict(err)
}, retryable)
}

func (m *manager) validateACRToken(ctx context.Context) error {
log := m.log.WithContext(ctx).WithField("secretName", operator.SecretName)
log.Info("validating acr token")
pullSecret, err := m.kubernetescli.CoreV1().Secrets(operator.Namespace).Get(ctx, operator.SecretName, metav1.GetOptions{})
if err != nil {
log.WithError(err).Errorln("failed to retrieve pull secret")
}
dockerConfig, err := pullsecret.UnmarshalSecretData(pullSecret)
if err != nil {
log.WithError(err).Error("failed to unmarshal pull secret")
}
for registry, authBase64 := range dockerConfig {
l := log.WithField("registry", registry)
l.Debug("validating registry authentication")
authDecoded, err := base64.StdEncoding.DecodeString(authBase64)
auth := strings.SplitN(string(authDecoded), ":", 2)
if len(auth) != 2 {
err = fmt.Errorf("registry credentials format error")
l.WithError(err).Error("failed to parse registry credentails")
}
l = l.WithField("clientid", auth[0])
if err != nil {
l.WithError(err).Error("failed to unmarshal pull secret")
return err
}
err = dockerregistry.CheckAuth(ctx, nil, auth[0], auth[1], registry)
if err != nil {
//time="2023-10-03T15:07:00-07:00" level=error msg="failed to authenticate to registry" func="cluster.(*manager).validateACRToken()" file="pkg/cluster/acrtoken.go:161" client_principal_name= client_request_id= component=backend correlation_id= error="unable to retrieve auth token: invalid username/password: unauthorized: authentication required, visit https://aka.ms/acr/authorization for more information." registry=arointsvc.azurecr.io request_id=dc67f86f-207b-4e9d-a280-f9cde690fc6a resource_group=aro-adenton resource_id=/subscriptions/fe16a035-e540-4ab7-80d9-373fa9a3d6ae/resourcegroups/aro-adenton/providers/microsoft.redhatopenshift/openshiftclusters/aro-adenton resource_name=aro-adenton secretName=cluster subscription_id=fe16a035-e540-4ab7-80d9-373fa9a3d6ae username=e2e-pull version=4.11.49
// time="2023-10-03T15:07:00-07:00" level=error msg="failed to authenticate to registry" func="cluster.(*manager).validateACRToken()" file="pkg/cluster/acrtoken.go:161" client_principal_name= client_request_id= component=backend correlation_id= error="unable to retrieve auth token: invalid username/password: unauthorized: Invalid clientid or client secret." registry=arosvc.azurecr.io request_id=dc67f86f-207b-4e9d-a280-f9cde690fc6a resource_group=aro-adenton resource_id=/subscriptions/fe16a035-e540-4ab7-80d9-373fa9a3d6ae/resourcegroups/aro-adenton/providers/microsoft.redhatopenshift/openshiftclusters/aro-adenton resource_name=aro-adenton secretName=cluster subscription_id=fe16a035-e540-4ab7-80d9-373fa9a3d6ae username=939049b4-59e1-4c9a-bec8-202e01f761ae version=4.11.49
l.WithError(err).Error("failed to authenticate to registry")
return err
} else {
l.Info("successfully authenticated")
}
}
return nil
}

func (m *manager) ensureValidACRToken(ctx context.Context) (bool, error) {
err := m.validateACRToken(ctx)
if err != nil {
return false, err
}
return true, nil
}
8 changes: 8 additions & 0 deletions pkg/cluster/adminupdate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,9 @@ func TestAdminUpdateSteps(t *testing.T) {
"[Action fixMCSCert-fm]",
"[Action fixMCSUserData-fm]",
"[Action ensureGatewayUpgrade-fm]",
"[Condition aroDeploymentReady-fm, timeout 5m0s]",
"[Action rotateACRTokenPassword-fm]",
"[Condition ensureValidACRToken-fm, timeout 5m0s]",
"[Action configureAPIServerCertificate-fm]",
"[Action configureIngressCertificate-fm]",
"[Action populateRegistryStorageAccountName-fm]",
Expand Down Expand Up @@ -218,7 +220,9 @@ func TestAdminUpdateSteps(t *testing.T) {
"[Action fixMCSCert-fm]",
"[Action fixMCSUserData-fm]",
"[Action ensureGatewayUpgrade-fm]",
"[Condition aroDeploymentReady-fm, timeout 5m0s]",
"[Action rotateACRTokenPassword-fm]",
"[Condition ensureValidACRToken-fm, timeout 5m0s]",
"[Action configureAPIServerCertificate-fm]",
"[Action configureIngressCertificate-fm]",
"[Action populateRegistryStorageAccountName-fm]",
Expand Down Expand Up @@ -259,7 +263,9 @@ func TestAdminUpdateSteps(t *testing.T) {
"[Action fixMCSCert-fm]",
"[Action fixMCSUserData-fm]",
"[Action ensureGatewayUpgrade-fm]",
"[Condition aroDeploymentReady-fm, timeout 5m0s]",
"[Action rotateACRTokenPassword-fm]",
"[Condition ensureValidACRToken-fm, timeout 5m0s]",
"[Action configureAPIServerCertificate-fm]",
"[Action configureIngressCertificate-fm]",
"[Action populateRegistryStorageAccountName-fm]",
Expand Down Expand Up @@ -331,7 +337,9 @@ func TestAdminUpdateSteps(t *testing.T) {
"[Action fixMCSCert-fm]",
"[Action fixMCSUserData-fm]",
"[Action ensureGatewayUpgrade-fm]",
"[Condition aroDeploymentReady-fm, timeout 5m0s]",
"[Action rotateACRTokenPassword-fm]",
"[Condition ensureValidACRToken-fm, timeout 5m0s]",
"[Action configureAPIServerCertificate-fm]",
"[Action configureIngressCertificate-fm]",
"[Action populateRegistryStorageAccountName-fm]",
Expand Down
4 changes: 4 additions & 0 deletions pkg/cluster/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@ func (m *manager) adminUpdate() []steps.Step {
if isEverything {
toRun = append(toRun,
steps.Action(m.ensureGatewayUpgrade),
steps.Condition(m.aroDeploymentReady, 5*time.Minute, true),
steps.Action(m.rotateACRTokenPassword),
steps.Condition(m.ensureValidACRToken, 5*time.Minute, true),
)
}

Expand Down Expand Up @@ -195,7 +197,9 @@ 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.Condition(m.aroDeploymentReady, 5*time.Minute, true),
steps.Action(m.rotateACRTokenPassword),
steps.Condition(m.ensureValidACRToken, 5*time.Minute, true),
steps.Action(m.configureAPIServerCertificate),
steps.Action(m.configureIngressCertificate),
steps.Action(m.renewMDSDCertificate),
Expand Down
3 changes: 3 additions & 0 deletions pkg/database/openshiftclusters.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import (
"github.com/Azure/ARO-RP/pkg/api"
"github.com/Azure/ARO-RP/pkg/database/cosmosdb"
"github.com/Azure/ARO-RP/pkg/util/uuid"

log "github.com/sirupsen/logrus"
)

const (
Expand Down Expand Up @@ -157,6 +159,7 @@ func (c *openShiftClusters) Get(ctx context.Context, key string) (*api.OpenShift
func (c *openShiftClusters) QueueLength(ctx context.Context, collid string) (int, error) {
partitions, err := c.collc.PartitionKeyRanges(ctx, collid)
if err != nil {
log.WithError(err).WithField("collid", collid).Errorf("unable to retrieve partition key ranges")
return 0, err
}

Expand Down
38 changes: 38 additions & 0 deletions pkg/operator/controllers/pullsecret/pullsecret_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,13 @@ package pullsecret

import (
"context"
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"strings"

dockerregistry "github.com/containers/image/v5/docker"
"github.com/sirupsen/logrus"
corev1 "k8s.io/api/core/v1"
kerrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -192,10 +196,20 @@ func (r *Reconciler) ensureGlobalPullSecret(ctx context.Context, operatorSecret,
}

err = r.client.Create(ctx, secret)
if err != nil {
return secret, err
}

err = r.validatePullSecret(ctx, secret)
return secret, err
}

err = r.client.Update(ctx, secret)
if err != nil {
return secret, err
}

err = r.validatePullSecret(ctx, secret)
return secret, err
}

Expand Down Expand Up @@ -223,3 +237,27 @@ func (r *Reconciler) parseRedHatKeys(secret *corev1.Secret) (foundKeys []string,

return foundKeys, nil
}

// validatePullSecret walk through each configured registry and check the pull secret works
func (r *Reconciler) validatePullSecret(ctx context.Context, secret *corev1.Secret) error {
dockerConfig, err := pullsecret.UnmarshalSecretData(secret)
if err != nil {
return err
}
for registry, authBase64 := range dockerConfig {
authDecoded, err := base64.StdEncoding.DecodeString(authBase64)
auth := strings.SplitN(string(authDecoded), ":", 2)
if len(auth) != 2 {
err = fmt.Errorf("credentials format error: %s", registry)
return err
}
if err != nil {
return err
}
err = dockerregistry.CheckAuth(ctx, nil, auth[0], auth[1], registry)
if err != nil {
return fmt.Errorf("failed to authenticate to registry %s: %w", registry, err)
}
}
return nil
}
20 changes: 20 additions & 0 deletions pkg/util/acrtoken/acrtoken.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ import (
"github.com/Azure/go-autorest/autorest"
"github.com/Azure/go-autorest/autorest/azure"
"github.com/Azure/go-autorest/autorest/to"
dockerregistry "github.com/containers/image/v5/docker"
containertypes "github.com/containers/image/v5/types"
log "github.com/sirupsen/logrus"

"github.com/Azure/ARO-RP/pkg/api"
"github.com/Azure/ARO-RP/pkg/env"
Expand All @@ -25,6 +28,7 @@ type Manager interface {
PutRegistryProfile(oc *api.OpenShiftCluster, rp *api.RegistryProfile)
EnsureTokenAndPassword(ctx context.Context, rp *api.RegistryProfile) (string, error)
RotateTokenPassword(ctx context.Context, rp *api.RegistryProfile) error
ValidateToken(ctx context.Context, rp *api.RegistryProfile) error
Delete(ctx context.Context, rp *api.RegistryProfile) error
}

Expand Down Expand Up @@ -142,6 +146,22 @@ func (m *manager) RotateTokenPassword(ctx context.Context, rp *api.RegistryProfi
return nil
}

// ValidateToken attempts to verify that the current ACR token functions
func (m *manager) ValidateToken(ctx context.Context, rp *api.RegistryProfile) error {
log.SetReportCaller(true)
l := log.WithField("profile", rp)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by an access to Password
flows to a logging call.
Sensitive data returned by an access to newPassword
flows to a logging call.
l.Info("ValidateToken")
sys := &containertypes.SystemContext{}
registry := rp.Name
l = l.WithField("sys", sys).WithField("registry", registry)
err := dockerregistry.CheckAuth(ctx, sys, rp.Username, string(rp.Password), registry)
if err != nil {
l.WithError(err).Errorln("unable to validate token")
return fmt.Errorf("%s: unable to validate token: %s", registry, err)
}
return nil
}

// generateTokenPassword takes an existing ACR token and generates
// a password for the specified password name
func (m *manager) generateTokenPassword(ctx context.Context, passwordName mgmtcontainerregistry.TokenPasswordName, rp *api.RegistryProfile) (string, error) {
Expand Down
3 changes: 3 additions & 0 deletions pkg/util/acrtoken/acrtoken_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,9 @@ func TestRotateTokenPassword(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if err != nil {
t.Fatal(err)
}
if registryProfile.Password != api.SecureString(tt.wantPassword) {
t.Error(registryProfile.Password)
}
Expand Down

0 comments on commit 0e29cc2

Please sign in to comment.