OCM-18900 | fix: Improve error message for proxy passwords with special characters

Author: olucasfreitas

cmd/create/cluster/cmd.go

@@ -3053,15 +3053,15 @@ func run(cmd *cobra.Command, _ []string) {
 			Help:     cmd.Flags().Lookup("https-proxy").Usage,
 			Default:  httpsProxy,
 			Validators: []interactive.Validator{
-				interactive.IsURL,
+				ocm.ValidateHTTPSProxy,
 			},
 		})
 		if err != nil {
 			r.Reporter.Errorf("Expected a valid https proxy: %s", err)
 			os.Exit(1)
 		}
 	}
-	err = interactive.IsURL(httpsProxy)
+	err = ocm.ValidateHTTPSProxy(httpsProxy)
 	if err != nil {
 		r.Reporter.Errorf("%s", err)
 		os.Exit(1)

cmd/edit/cluster/cmd.go

@@ -513,7 +513,7 @@ func run(cmd *cobra.Command, _ []string) {
 		}
 	}
 	if httpsProxy != nil && *httpsProxy != input.DoubleQuotesToRemove {
-		err = interactive.IsURL(*httpsProxy)
+		err = ocm.ValidateHTTPSProxy(*httpsProxy)
 		if err != nil {
 			r.Reporter.Errorf("%s", err)
 			os.Exit(1)
@@ -562,10 +562,7 @@ func run(cmd *cobra.Command, _ []string) {
 	}
 
 	/*******  AdditionalTrustBundle *******/
-	updateAdditionalTrustBundle := false
-	if additionalTrustBundleFile != nil {
-		updateAdditionalTrustBundle = true
-	}
+	updateAdditionalTrustBundle := additionalTrustBundleFile != nil
 	if useExistingVPC && !updateAdditionalTrustBundle && additionalTrustBundleFile == nil &&
 		interactive.Enabled() {
 		updateAdditionalTrustBundleValue, err := interactive.GetBool(interactive.Input{
@@ -620,10 +617,7 @@ func run(cmd *cobra.Command, _ []string) {
 	}
 
 	/*******  AdditionalAllowedPrincipals *******/
-	updateAdditionalAllowedPrincipals := false
-	if additionalAllowedPrincipals != nil {
-		updateAdditionalAllowedPrincipals = true
-	}
+	updateAdditionalAllowedPrincipals := additionalAllowedPrincipals != nil
 	if !updateAdditionalAllowedPrincipals && additionalAllowedPrincipals == nil &&
 		interactive.Enabled() {
 		updateAdditionalAllowedPrincipalsValue, err := interactive.GetBool(interactive.Input{
@@ -1013,15 +1007,15 @@ func warnUserForOAuthHCPVisibility(r *rosa.Runtime, clusterKey string, cluster *
 func validateExpiration() (expiration time.Time, err error) {
 	// Validate options
 	if len(args.expirationTime) > 0 && args.expirationDuration != 0 {
-		err = errors.New("At most one of 'expiration-time' or 'expiration' may be specified")
+		err = errors.New("at most one of 'expiration-time' or 'expiration' may be specified")
 		return
 	}
 
 	// Parse the expiration options
 	if len(args.expirationTime) > 0 {
 		t, err := parseRFC3339(args.expirationTime)
 		if err != nil {
-			err = fmt.Errorf("Failed to parse expiration-time: %s", err)
+			err = fmt.Errorf("failed to parse expiration-time: %s", err)
 			return expiration, err
 		}
 
@@ -1039,7 +1033,7 @@ func validateExpiration() (expiration time.Time, err error) {
 func validateOvnInternalSubnetConfiguration() (ovnInternalSubnets map[string]string, err error) {
 	if len(args.ovnInternalSubnets) > 0 {
 		if args.networkType == "" {
-			err = fmt.Errorf("Expected a value for %s when supplying the flag %s", ocm.NetworkTypeFlagName,
+			err = fmt.Errorf("expected a value for %s when supplying the flag %s", ocm.NetworkTypeFlagName,
 				ocm.OvnInternalSubnetsFlagName)
 			return
 		}
@@ -1052,7 +1046,7 @@ func validateOvnInternalSubnetConfiguration() (ovnInternalSubnets map[string]str
 func validateNetworkType() (networkConfig string, err error) {
 	if len(args.networkType) > 0 {
 		if args.networkType != ocm.NetworkTypeOvn && args.networkType != ocm.NetworkTypeOvnAlias {
-			err = fmt.Errorf("Incorrect network type '%s', please use '%s' or remove the flag",
+			err = fmt.Errorf("incorrect network type '%s', please use '%s' or remove the flag",
 				args.networkType, ocm.NetworkTypeOvn)
 		} else {
 			networkConfig = ocm.NetworkTypeOvn // allows use of alias (OVN-Kubernetes)- but sets it to correct value for API
@@ -1088,11 +1082,11 @@ func setAuditLogForwarding(r *rosa.Runtime, cmd *cobra.Command, cluster *cmv1.Cl
 	argValuePtr *string, err error) {
 	if cmd.Flags().Changed("audit-log-arn") {
 		if !aws.IsHostedCP(cluster) {
-			return nil, fmt.Errorf("Audit log forwarding to AWS CloudWatch is only supported for Hosted Control Plane clusters")
+			return nil, fmt.Errorf("audit log forwarding to AWS CloudWatch is only supported for Hosted Control Plane clusters")
 
 		}
 		if auditLogArn != "" && !aws.RoleArnRE.MatchString(auditLogArn) {
-			return nil, fmt.Errorf("Expected a valid value for audit-log-arn matching %s", aws.RoleArnRE.String())
+			return nil, fmt.Errorf("expected a valid value for audit-log-arn matching %s", aws.RoleArnRE.String())
 		}
 		argValuePtr := new(string)
 		*argValuePtr = auditLogArn
@@ -1129,7 +1123,7 @@ func auditLogInteractivePrompt(r *rosa.Runtime, cmd *cobra.Command, cluster *cmv
 		Required: true,
 	})
 	if err != nil {
-		return nil, fmt.Errorf("Expected a valid value: %s", err)
+		return nil, fmt.Errorf("expected a valid value: %s", err)
 	}
 	if requestAuditLogForwarding {
 
@@ -1146,7 +1140,7 @@ func auditLogInteractivePrompt(r *rosa.Runtime, cmd *cobra.Command, cluster *cmv
 			},
 		})
 		if err != nil {
-			return nil, fmt.Errorf("Expected a valid value for audit-log-arn: %s", err)
+			return nil, fmt.Errorf("expected a valid value for audit-log-arn: %s", err)
 		}
 		*auditLogRolePtr = auditLogRoleValue
 		return auditLogRolePtr, nil
@@ -1159,7 +1153,7 @@ func auditLogInteractivePrompt(r *rosa.Runtime, cmd *cobra.Command, cluster *cmv
 			Default:  false,
 		})
 		if err != nil {
-			return nil, fmt.Errorf("Expected a valid value: %s", err)
+			return nil, fmt.Errorf("expected a valid value: %s", err)
 		}
 		if disableAuditLog {
 			*auditLogRolePtr = ""
@@ -1190,7 +1184,7 @@ func BuildClusterConfigWithRegistry(clusterConfig ocm.Spec, allowedRegistries []
 		ca, err := clusterregistryconfig.BuildAdditionalTrustedCAFromInputFile(additionalTrustedCa)
 		if err != nil {
 			return clusterConfig, fmt.Errorf(
-				"Failed to build the additional trusted ca from file %s, got error: %s",
+				"failed to build the additional trusted ca from file %s, got error: %s",
 				additionalTrustedCa, err)
 		}
 		clusterConfig.AdditionalTrustedCa = ca

cmd/edit/cluster/cmd_test.go

@@ -108,7 +108,7 @@ var _ = Describe("Edit cluster", func() {
 		})
 		It("KO: should fail with error if ca file does not exist", func() {
 			_, err := BuildClusterConfigWithRegistry(clusterConfig, allowedRegistries, nil, nil, "not-exist", "", "")
-			Expect(err).To(MatchError("Failed to build the additional trusted ca from file not-exist, " +
+			Expect(err).To(MatchError("failed to build the additional trusted ca from file not-exist, " +
 				"got error: expected a valid additional trusted certificate spec file:" +
 				" open not-exist: no such file or directory"))
 		})

pkg/helper/helpers.go

@@ -268,3 +268,65 @@ func FilterEmptyStrings(strings []string) []string {
 	}
 	return filteredResult
 }
+
+type URLCredentialValidation struct {
+	Field       string
+	InvalidChar rune
+	HasInvalid  bool
+}
+
+func ValidateURLCredentials(urlString string) *URLCredentialValidation {
+	if !strings.Contains(urlString, "@") || !strings.Contains(urlString, "://") {
+		return &URLCredentialValidation{HasInvalid: false}
+	}
+
+	schemeIdx := strings.Index(urlString, "://")
+	if schemeIdx == -1 {
+		return &URLCredentialValidation{HasInvalid: false}
+	}
+
+	rest := urlString[schemeIdx+3:]
+	atIdx := strings.LastIndex(rest, "@")
+	if atIdx == -1 {
+		return &URLCredentialValidation{HasInvalid: false}
+	}
+
+	userinfo := rest[:atIdx]
+
+	if strings.Count(urlString, "@") > 1 {
+		return &URLCredentialValidation{
+			Field:       "password",
+			InvalidChar: '@',
+			HasInvalid:  true,
+		}
+	}
+
+	colonIdx := strings.Index(userinfo, ":")
+	if colonIdx == -1 {
+		return checkForInvalidChars(userinfo, "username")
+	}
+
+	username := userinfo[:colonIdx]
+	if result := checkForInvalidChars(username, "username"); result.HasInvalid {
+		return result
+	}
+
+	password := userinfo[colonIdx+1:]
+	return checkForInvalidChars(password, "password")
+}
+
+func checkForInvalidChars(value, field string) *URLCredentialValidation {
+	invalidChars := "/:#?[]@!$&'()*+,;="
+
+	for _, char := range value {
+		if strings.ContainsRune(invalidChars, char) {
+			return &URLCredentialValidation{
+				Field:       field,
+				InvalidChar: char,
+				HasInvalid:  true,
+			}
+		}
+	}
+
+	return &URLCredentialValidation{HasInvalid: false}
+}

pkg/interactive/validation_test.go

@@ -131,7 +131,7 @@ var _ = Describe("SubnetsValidator", func() {
 
 			err := validator(answers)
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("The number of subnets for a public hosted " +
+			Expect(err.Error()).To(ContainSubstring("the number of subnets for a public hosted " +
 				"cluster should be at least two"))
 		})
 
@@ -153,7 +153,7 @@ var _ = Describe("SubnetsValidator", func() {
 
 			err := validator(answers)
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("Must have at least one public subnet when not using both " +
+			Expect(err.Error()).To(ContainSubstring("must have at least one public subnet when not using both " +
 				"'private API' and 'private ingress'"))
 		})
 
@@ -169,7 +169,7 @@ var _ = Describe("SubnetsValidator", func() {
 
 			err := validator(answers)
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("The number of subnets for a 'multi-AZ'"+
+			Expect(err.Error()).To(ContainSubstring("the number of subnets for a 'multi-AZ'"+
 				" 'private link cluster' should be '3', instead received: '%d'", len(answers)))
 		})