Repository: openshift/assisted-service (language: python)

Issue:
OCPBUGS-35932: Allow auth in docker credentials to be empty
<!--
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

You can refer to [Kubernetes community documentation] on writing good commit messages, which provides good tips and ideas.

Some PRs address specific issues. Please, refer to the [CONTRIBUTING] documentation for more
information on how to link a PR to an existing issue.

It's recommended to take a few extra minutes to provide more information about
how this code was tested. Here are some questions that may be worth answering:

- Should this PR be tested by the reviewer?
- Is this PR relying on CI for an e2e test run?
- Should this PR be tested in a specific environment?
- Any logs, screenshots, etc that can help with the review process?

-->

Allow auth field to be empty in docker pull secret

The impacts are limited to 
* uploader/ package (for on-prem usecase)
* Saas. The code being called by https://github.com/openshift/assisted-service/blob/v2.32.0/internal/cluster/auth.go#L45
  * it only looks for "cloud.openshift.com"
  * this func is only called if the authType is `auth.TypeRHSSO`

I will appreciate to have the opinion of Assisted-Installer gurus to be sure I'm not missing something and it's safe to allow such entry without additional checks. For example, should I add some check here https://github.com/openshift/assisted-service/blob/v2.32.0/internal/cluster/auth.go#L32 or somewhere else to ensure that for SaaS the user:password is specified ?

Also 
  * removed Password that wasn't used https://github.com/openshift/assisted-service/blob/v2.32.0/internal/cluster/validations/pull_secret_validation.go#L51
  * removed credsStore logic, that didn't look functional

credsStore didn't seem functional because:
* The wrong field was parsed (it is at the same level of `auths`, the code was looking at it in `auths.*`. See https://github.com/docker/cli/blob/master/cli/config/configfile/file.go#L28
* The logic looked wrong at first sight, so it probably never worked ?
  * check if one of `credsStore` or `auth` is valuated https://github.com/openshift/assisted-service/blob/v2.32.0/internal/cluster/validations/pull_secret_validation.go#L89
  * if auth is not present take the default value of it (empty string)  - why ? https://github.com/openshift/assisted-service/blob/v2.32.0/internal/cluster/validations/pull_secret_validation.go#L97
  * Assume from here, auth is valuated and try to parse it and extract info from it. This should fail all the time if auth is not present

So I think it's safe to assume that credsStore can be removed. Let me know what you think

## List all the issues related to this PR

- [ ] New Feature <!-- new functionality -->
- [ ] Enhancement <!-- refactor, code changes, improvement, that won't add new features -->
- [x] Bug fix
- [ ] Tests
- [ ] Documentation
- [ ] CI/CD <!-- Notice that changes for Dockerfiles/Jenkinsfiles aren't tested in CI due to a known bug. -->

## What environments does this code impact?

- [ ] Automation (CI, tools, etc)
- [x] Cloud
- [x] Operator Managed Deployments
- [ ] None

## How was this code tested?

<!-- Please, select one or more if needed: -->

- [ ] assisted-test-infra environment
- [ ] dev-scripts environment
- [ ] Reviewer's test appreciated
- [ ] Waiting for CI to do a full test run
- [ ] Manual (Elaborate on how it was tested)
- [x] No tests needed

## Checklist

- [x] Title and description added to both, commit and PR.
- [x] Relevant issues have been associated (see [CONTRIBUTING] guide)
- [x] This change does not require a documentation update (docstring, `docs`, README, etc)
- [x] Does this change include unit-tests (note that code changes require unit-tests)

## Reviewers Checklist

- Are the title and description (in both PR and commit) meaningful and clear?
- Is there a bug required (and linked) for this change?
- Should this PR be backported?

[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines
[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md



Discussion / hints:
@pastequo: This pull request references [Jira Issue OCPBUGS-35932](https://issues.redhat.com//browse/OCPBUGS-35932), which is invalid:
 - expected the bug to target the "4.17.0" version, but no target version was set

Comment <code>/jira refresh</code> to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

<details>

In response to [this](https://github.com/openshift/assisted-service/pull/6465):

><!--
>Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
>
>You can refer to [Kubernetes community documentation] on writing good commit messages, which provides good tips and ideas.
>
>Some PRs address specific issues. Please, refer to the [CONTRIBUTING] documentation for more
>information on how to link a PR to an existing issue.
>
>It's recommended to take a few extra minutes to provide more information about
>how this code was tested. Here are some questions that may be worth answering:
>
>- Should this PR be tested by the reviewer?
>- Is this PR relying on CI for an e2e test run?
>- Should this PR be tested in a specific environment?
>- Any logs, screenshots, etc that can help with the review process?
>
>-->
>
>Allow auth field to be empty in docker pull secret
>
>The impacts are limited to 
>* uploader/ package (for on-prem usecase)
>* Saas. The code being called by https://github.com/openshift/assisted-service/blob/v2.32.0/internal/cluster/auth.go#L45
>  * it only looks for "cloud.openshift.com"
>  * this func is only called if the authType is `auth.TypeRHSSO`
>
>I will appreciate to have the opinion of Assisted-Installer gurus to be sure I'm not missing something and it's safe to allow such entry without additional checks. For example, should I add some check here https://github.com/openshift/assisted-service/blob/v2.32.0/internal/cluster/auth.go#L32 or somewhere else to ensure that for SaaS the user:password is specified ?
>
>Also 
>  * removed Password that wasn't used https://github.com/openshift/assisted-service/blob/v2.32.0/internal/cluster/validations/pull_secret_validation.go#L51
>  * removed credsStore logic, that didn't look functional
>
>credsStore didn't seem functional because:
>* The wrong field was parsed (it is at the same level of `auths`, the code was looking at it in `auths.*`. See https://github.com/docker/cli/blob/master/cli/config/configfile/file.go#L28
>* The logic looked wrong at first sight, so it probably never worked ?
>  * check if one of `credsStore` or `auth` is valuated https://github.com/openshift/assisted-service/blob/v2.32.0/internal/cluster/validations/pull_secret_validation.go#L89
>  * if auth is not present take the default value of it (empty string)  - why ? https://github.com/openshift/assisted-service/blob/v2.32.0/internal/cluster/validations/pull_secret_validation.go#L97
>  * Assume from here, auth is valuated and try to parse it and extract info from it. This should fail all the time if auth is not present
>
>So I think it's safe to assume that credsStore is safe to be removed. Let me know what you think
>
>## List all the issues related to this PR
>
>- [ ] New Feature <!-- new functionality -->
>- [ ] Enhancement <!-- refactor, code changes, improvement, that won't add new features -->
>- [x] Bug fix
>- [ ] Tests
>- [ ] Documentation
>- [ ] CI/CD <!-- Notice that changes for Dockerfiles/Jenkinsfiles aren't tested in CI due to a known bug. -->
>
>## What environments does this code impact?
>
>- [ ] Automation (CI, tools, etc)
>- [x] Cloud
>- [x] Operator Managed Deployments
>- [ ] None
>
>## How was this code tested?
>
><!-- Please, select one or more if needed: -->
>
>- [ ] assisted-test-infra environment
>- [ ] dev-scripts environment
>- [ ] Reviewer's test appreciated
>- [ ] Waiting for CI to do a full test run
>- [ ] Manual (Elaborate on how it was tested)
>- [x] No tests needed
>
>## Checklist
>
>- [x] Title and description added to both, commit and PR.
>- [x] Relevant issues have been associated (see [CONTRIBUTING] guide)
>- [x] This change does not require a documentation update (docstring, `docs`, README, etc)
>- [x] Does this change include unit-tests (note that code changes require unit-tests)
>
>## Reviewers Checklist
>
>- Are the title and description (in both PR and commit) meaningful and clear?
>- Is there a bug required (and linked) for this change?
>- Should this PR be backported?
>
>[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines
>[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md
>


Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service).  If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
</details>
/jira refresh
@pastequo: This pull request references [Jira Issue OCPBUGS-35932](https://issues.redhat.com//browse/OCPBUGS-35932), which is valid. The bug has been moved to the POST state.

<details><summary>3 validation(s) were run on this bug</summary>

* bug is open, matching expected state (open)
* bug target version (4.17.0) matches configured target version for branch (4.17.0)
* bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)</details>

No GitHub users were found matching the public email listed for the QA contact in Jira (mburman@redhat.com), skipping review request.

<details>

In response to [this](https://github.com/openshift/assisted-service/pull/6465#issuecomment-2186934728):

>/jira refresh


Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service).  If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
</details>

Fix patch:
diff --git a/internal/cluster/validations/pull_secret_validation.go b/internal/cluster/validations/pull_secret_validation.go
index 228b1b888d3..5096229a02f 100644
--- a/internal/cluster/validations/pull_secret_validation.go
+++ b/internal/cluster/validations/pull_secret_validation.go
@@ -1,7 +1,6 @@
 package validations
 
 import (
-	"bytes"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
@@ -68,7 +67,7 @@ func (e *PullSecretError) Unwrap() error {
 	return e.Cause
 }
 
-// ParsePullSecret validates the format of a pull secret and converts the secret string into individual credentail entries
+// ParsePullSecret validates the format of a pull secret and converts the secret string into individual credential entries
 func ParsePullSecret(secret string) (map[string]PullSecretCreds, error) {
 	result := make(map[string]PullSecretCreds)
 	var s imagePullSecret
@@ -77,49 +76,19 @@ func ParsePullSecret(secret string) (map[string]PullSecretCreds, error) {
 	if err != nil {
 		return nil, &PullSecretError{Msg: "pull secret must be a well-formed JSON", Cause: err}
 	}
-
 	if len(s.Auths) == 0 {
 		return nil, &PullSecretError{Msg: "pull secret must contain 'auths' JSON-object field"}
 	}
 
 	for d, a := range s.Auths {
-
-		_, authPresent := a["auth"]
-		_, credsStorePresent := a["credsStore"]
-		if !authPresent && !credsStorePresent {
-			return nil, &PullSecretError{Msg: fmt.Sprintf("invalid pull secret: %q JSON-object requires either 'auth' or 'credsStore' field", d)}
-		}
-
-		var authRaw string
-		if auth, ok := a["auth"].(string); authPresent && !ok {
-			return nil, &PullSecretError{Msg: fmt.Sprintf("invalid pull secret: 'auth' field of %q is %v but should be a string", d, a["auth"])}
-		} else {
-			authRaw = auth
-		}
-		data, err := base64.StdEncoding.DecodeString(authRaw)
+		pullSecretCreds, err := parseAuthConfig(d, a)
 		if err != nil {
-			return nil, &PullSecretError{Msg: fmt.Sprintf("invalid pull secret: 'auth' field of %q is not base64-encoded", d)}
-		}
-
-		res := bytes.SplitN(data, []byte(":"), 2)
-		if len(res) != 2 {
-			return nil, &PullSecretError{Msg: fmt.Sprintf("invalid pull secret: 'auth' for %s is not in 'user:password' format", d)}
-		}
-
-		var email string
-		if emailString, ok := a["email"].(string); ok {
-			email = emailString
-		}
-
-		result[d] = PullSecretCreds{
-			Password: string(res[1]),
-			Username: string(res[0]),
-			AuthRaw:  authRaw,
-			Registry: d,
-			Email:    email,
+			return nil, err
 		}
 
+		result[d] = pullSecretCreds
 	}
+
 	return result, nil
 }
 
@@ -161,6 +130,46 @@ func NewPullSecretValidator(publicRegistries map[string]bool, authHandler auth.A
 	}, nil
 }
 
+func parseAuthConfig(registry string, authConfig map[string]interface{}) (PullSecretCreds, error) {
+	email, _ := authConfig["email"].(string)
+
+	ret := PullSecretCreds{
+		Registry: registry,
+		Email:    email,
+	}
+
+	auth, authPresent := authConfig["auth"]
+	if !authPresent {
+		return ret, nil
+	}
+
+	authRaw, ok := auth.(string)
+	if !ok {
+		return ret, &PullSecretError{Msg: fmt.Sprintf("invalid pull secret: 'auth' field of %q is %v but should be a string", registry, authConfig["auth"])}
+	}
+
+	if len(authRaw) == 0 {
+		return ret, nil
+	}
+
+	ret.AuthRaw = authRaw
+
+	data, err := base64.StdEncoding.DecodeString(authRaw)
+	if err != nil {
+		return ret, &PullSecretError{Msg: fmt.Sprintf("invalid pull secret: 'auth' field of %q is not base64-encoded", registry)}
+	}
+
+	res := strings.SplitN(string(data), ":", 2)
+	if len(res) != 2 {
+		return ret, &PullSecretError{Msg: fmt.Sprintf("invalid pull secret: 'auth' for %s is not in 'user:password' format", registry)}
+	}
+
+	ret.Username = res[0]
+	ret.Password = res[1]
+
+	return ret, nil
+}
+
 func validateRegistryWithAuth(registry string, credentials map[string]PullSecretCreds) error {
 	// Both "docker.io" and "https://index.docker.io/v1/" are acceptable for DockerHub login
 	if registry == dockerHubRegistry {
diff --git a/internal/uploader/auth_utils.go b/internal/uploader/auth_utils.go
index a8c5452ad1d..7e3ba5ffef6 100644
--- a/internal/uploader/auth_utils.go
+++ b/internal/uploader/auth_utils.go
@@ -50,6 +50,10 @@ func getPullSecret(clusterPullSecret string, k8sclient k8sclient.K8SClient) (Pul
 	ret.Identity.EmailDomain = getEmailDomain(identityCreds.Email)
 	ret.APIAuth.AuthRaw = authCreds.AuthRaw
 
+	if ret.APIAuth.AuthRaw == "" {
+		return ret, errors.New("no credentials provided")
+	}
+
 	return ret, nil
 }
 
@@ -91,18 +95,53 @@ func getCloudOpenshiftCreds(pullSecretData string) (*validations.PullSecretCreds
 }
 
 func selectPullSecretForDataProcessing(managementCreds, workloadCreds *validations.PullSecretCreds) *validations.PullSecretCreds {
-	if workloadCreds != nil {
+	// If one of the two is nil, return the other
+	if workloadCreds == nil {
+		return managementCreds
+	}
+
+	if managementCreds == nil {
 		return workloadCreds
 	}
 
-	return managementCreds
+	// Otherwise, return the one with more information.
+	// If both have the same number of information, priority to the workloadCreds
+	workloadInfos := countIdentityInfo(workloadCreds)
+	managementInfos := countIdentityInfo(managementCreds)
+
+	if managementInfos > workloadInfos {
+		return managementCreds
+	}
+
+	return workloadCreds
+}
+
+func countIdentityInfo(creds *validations.PullSecretCreds) int {
+	ret := 0
+
+	if creds.Username != "" {
+		ret++
+	}
+
+	if creds.Email != "" {
+		ret++
+	}
+
+	return ret
 }
 
 func selectPullSecretForAuthent(managementCreds, workloadCreds *validations.PullSecretCreds) *validations.PullSecretCreds {
-	if managementCreds != nil {
+	if managementCreds != nil && managementCreds.AuthRaw != "" {
 		return managementCreds
 	}
 
+	if workloadCreds != nil && workloadCreds.AuthRaw != "" {
+		return workloadCreds
+	}
+
+	if managementCreds != nil {
+		return managementCreds
+	}
 	return workloadCreds
 }
 


Test patch:
diff --git a/internal/uploader/auth_utils_test.go b/internal/uploader/auth_utils_test.go
index ccf91466c04..add7cd91429 100644
--- a/internal/uploader/auth_utils_test.go
+++ b/internal/uploader/auth_utils_test.go
@@ -16,11 +16,12 @@ import (
 
 var _ = Describe("getPullSecret", func() {
 	var (
-		ctrl                   *gomock.Controller
-		mockK8sClient          *k8sclient.MockK8SClient
-		clusterPullSecretToken string
-		OCMPullSecretToken     string
-		pullSecretFormat       string
+		ctrl                          *gomock.Controller
+		mockK8sClient                 *k8sclient.MockK8SClient
+		clusterPullSecretToken        string
+		OCMPullSecretToken            string
+		pullSecretFormat              string
+		pullSecretWithEmptyAuthFormat string
 	)
 
 	BeforeEach(func() {
@@ -28,7 +29,8 @@ var _ = Describe("getPullSecret", func() {
 		mockK8sClient = k8sclient.NewMockK8SClient(ctrl)
 		clusterPullSecretToken = base64.StdEncoding.EncodeToString([]byte("clustersecret:clustercredentials"))
 		OCMPullSecretToken = base64.StdEncoding.EncodeToString([]byte("ocmuser:ocmcredentials"))
-		pullSecretFormat = `{"auths":{"%s":{"auth":"%s","email":"r@example.com"}}}` // #nosec
+		pullSecretFormat = `{"auths":{"%s":{"auth":"%s","email":"r@example.com"}}}`          // #nosec
+		pullSecretWithEmptyAuthFormat = `{"auths":{"%s":{"auth":"","email":"r@empty.com"}}}` // #nosec
 	})
 
 	It("successfully gets a pull secret when the OCM pull secret is present", func() {
@@ -65,6 +67,66 @@ var _ = Describe("getPullSecret", func() {
 		Expect(pullSecret.Identity.EmailDomain).To(Equal("example.com"))
 		Expect(pullSecret.APIAuth.AuthRaw).To(Equal(clusterPullSecretToken))
 	})
+	It("successfully forge a pull secret when auth is not defined in the OCM pull secret", func() {
+		By("setting up the ocm pull secret")
+		OCMPullSecret := fmt.Sprintf(pullSecretWithEmptyAuthFormat, openshiftTokenKey)
+		data := map[string][]byte{corev1.DockerConfigJsonKey: []byte(OCMPullSecret)}
+
+		OCMSecret := &corev1.Secret{
+			TypeMeta: metav1.TypeMeta{
+				Kind:       "Secret",
+				APIVersion: "v1",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: "openshift-config",
+				Name:      "pull-secret",
+			},
+			Data: data,
+			Type: corev1.SecretTypeDockerConfigJson,
+		}
+		mockK8sClient.EXPECT().GetSecret("openshift-config", "pull-secret").Return(OCMSecret, nil).Times(1)
+
+		By("setting up the cluster pull secret")
+		clusterPullSecret := fmt.Sprintf(pullSecretFormat, openshiftTokenKey, clusterPullSecretToken)
+
+		By("calling getPullSecret")
+		pullSecret, err := getPullSecret(clusterPullSecret, mockK8sClient)
+
+		Expect(err).NotTo(HaveOccurred())
+		Expect(pullSecret.Identity.Username).To(Equal("clustersecret"))
+		Expect(pullSecret.Identity.EmailDomain).To(Equal("example.com"))
+		Expect(pullSecret.APIAuth.AuthRaw).To(Equal(clusterPullSecretToken))
+	})
+	It("successfully forge a pull secret when auth is not defined in the cluster pull secret", func() {
+		By("setting up the ocm pull secret")
+		OCMPullSecret := fmt.Sprintf(pullSecretFormat, openshiftTokenKey, OCMPullSecretToken)
+		data := map[string][]byte{corev1.DockerConfigJsonKey: []byte(OCMPullSecret)}
+
+		OCMSecret := &corev1.Secret{
+			TypeMeta: metav1.TypeMeta{
+				Kind:       "Secret",
+				APIVersion: "v1",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: "openshift-config",
+				Name:      "pull-secret",
+			},
+			Data: data,
+			Type: corev1.SecretTypeDockerConfigJson,
+		}
+		mockK8sClient.EXPECT().GetSecret("openshift-config", "pull-secret").Return(OCMSecret, nil).Times(1)
+
+		By("setting up the cluster pull secret")
+		clusterPullSecret := fmt.Sprintf(pullSecretWithEmptyAuthFormat, openshiftTokenKey)
+
+		By("calling getPullSecret")
+		pullSecret, err := getPullSecret(clusterPullSecret, mockK8sClient)
+
+		Expect(err).NotTo(HaveOccurred())
+		Expect(pullSecret.Identity.Username).To(Equal("ocmuser"))
+		Expect(pullSecret.Identity.EmailDomain).To(Equal("example.com"))
+		Expect(pullSecret.APIAuth.AuthRaw).To(Equal(OCMPullSecretToken))
+	})
 	It("fails to gets a pull secret when the OCM pull secret exists, but doesn't contain the correct credentials", func() {
 		OCMPullSecret := fmt.Sprintf(pullSecretFormat, "clouds.openshift.com", OCMPullSecretToken)
 		data := map[string][]byte{corev1.DockerConfigJsonKey: []byte(OCMPullSecret)}
@@ -106,6 +168,33 @@ var _ = Describe("getPullSecret", func() {
 		_, err := getPullSecret("", mockK8sClient)
 		Expect(err).To(HaveOccurred())
 	})
+	It("fails when auth fields are empty", func() {
+		By("setting up the ocm pull secret")
+		OCMPullSecret := fmt.Sprintf(pullSecretWithEmptyAuthFormat, openshiftTokenKey)
+		data := map[string][]byte{corev1.DockerConfigJsonKey: []byte(OCMPullSecret)}
+
+		OCMSecret := &corev1.Secret{
+			TypeMeta: metav1.TypeMeta{
+				Kind:       "Secret",
+				APIVersion: "v1",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: "openshift-config",
+				Name:      "pull-secret",
+			},
+			Data: data,
+			Type: corev1.SecretTypeDockerConfigJson,
+		}
+		mockK8sClient.EXPECT().GetSecret("openshift-config", "pull-secret").Return(OCMSecret, nil).Times(1)
+
+		By("setting up the cluster pull secret")
+		clusterPullSecret := fmt.Sprintf(pullSecretWithEmptyAuthFormat, openshiftTokenKey)
+
+		By("calling getPullSecret")
+		_, err := getPullSecret(clusterPullSecret, mockK8sClient)
+
+		Expect(err).To(HaveOccurred())
+	})
 })
 
 var _ = Describe("getEmailDomain", func() {