Skip to content

Commit 560e392

Browse files
olucasfreitasolucasfreitas
committed
OCM-18900 | fix: Improve error message for proxy passwords with special characters
1 parent 0d193e3 commit 560e392

File tree

7 files changed

+568
-41
lines changed

7 files changed

+568
-41
lines changed

‎cmd/create/cluster/cmd.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3053,15 +3053,15 @@ func run(cmd *cobra.Command, _ []string) {
30533053
Help: cmd.Flags().Lookup("https-proxy").Usage,
30543054
Default: httpsProxy,
30553055
Validators: []interactive.Validator{
3056-
interactive.IsURL,
3056+
ocm.ValidateHTTPSProxy,
30573057
},
30583058
})
30593059
if err != nil {
30603060
r.Reporter.Errorf("Expected a valid https proxy: %s", err)
30613061
os.Exit(1)
30623062
}
30633063
}
3064-
err = interactive.IsURL(httpsProxy)
3064+
err = ocm.ValidateHTTPSProxy(httpsProxy)
30653065
if err != nil {
30663066
r.Reporter.Errorf("%s", err)
30673067
os.Exit(1)

‎cmd/edit/cluster/cmd.go‎

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -513,7 +513,7 @@ func run(cmd *cobra.Command, _ []string) {
513513
}
514514
}
515515
if httpsProxy != nil && *httpsProxy != input.DoubleQuotesToRemove {
516-
err = interactive.IsURL(*httpsProxy)
516+
err = ocm.ValidateHTTPSProxy(*httpsProxy)
517517
if err != nil {
518518
r.Reporter.Errorf("%s", err)
519519
os.Exit(1)
@@ -562,10 +562,7 @@ func run(cmd *cobra.Command, _ []string) {
562562
}
563563

564564
/******* AdditionalTrustBundle *******/
565-
updateAdditionalTrustBundle := false
566-
if additionalTrustBundleFile != nil {
567-
updateAdditionalTrustBundle = true
568-
}
565+
updateAdditionalTrustBundle := additionalTrustBundleFile != nil
569566
if useExistingVPC && !updateAdditionalTrustBundle && additionalTrustBundleFile == nil &&
570567
interactive.Enabled() {
571568
updateAdditionalTrustBundleValue, err := interactive.GetBool(interactive.Input{
@@ -620,10 +617,7 @@ func run(cmd *cobra.Command, _ []string) {
620617
}
621618

622619
/******* AdditionalAllowedPrincipals *******/
623-
updateAdditionalAllowedPrincipals := false
624-
if additionalAllowedPrincipals != nil {
625-
updateAdditionalAllowedPrincipals = true
626-
}
620+
updateAdditionalAllowedPrincipals := additionalAllowedPrincipals != nil
627621
if !updateAdditionalAllowedPrincipals && additionalAllowedPrincipals == nil &&
628622
interactive.Enabled() {
629623
updateAdditionalAllowedPrincipalsValue, err := interactive.GetBool(interactive.Input{
@@ -1013,15 +1007,15 @@ func warnUserForOAuthHCPVisibility(r *rosa.Runtime, clusterKey string, cluster *
10131007
func validateExpiration() (expiration time.Time, err error) {
10141008
// Validate options
10151009
if len(args.expirationTime) > 0 && args.expirationDuration != 0 {
1016-
err = errors.New("At most one of 'expiration-time' or 'expiration' may be specified")
1010+
err = errors.New("at most one of 'expiration-time' or 'expiration' may be specified")
10171011
return
10181012
}
10191013

10201014
// Parse the expiration options
10211015
if len(args.expirationTime) > 0 {
10221016
t, err := parseRFC3339(args.expirationTime)
10231017
if err != nil {
1024-
err = fmt.Errorf("Failed to parse expiration-time: %s", err)
1018+
err = fmt.Errorf("failed to parse expiration-time: %s", err)
10251019
return expiration, err
10261020
}
10271021

@@ -1039,7 +1033,7 @@ func validateExpiration() (expiration time.Time, err error) {
10391033
func validateOvnInternalSubnetConfiguration() (ovnInternalSubnets map[string]string, err error) {
10401034
if len(args.ovnInternalSubnets) > 0 {
10411035
if args.networkType == "" {
1042-
err = fmt.Errorf("Expected a value for %s when supplying the flag %s", ocm.NetworkTypeFlagName,
1036+
err = fmt.Errorf("expected a value for %s when supplying the flag %s", ocm.NetworkTypeFlagName,
10431037
ocm.OvnInternalSubnetsFlagName)
10441038
return
10451039
}
@@ -1052,7 +1046,7 @@ func validateOvnInternalSubnetConfiguration() (ovnInternalSubnets map[string]str
10521046
func validateNetworkType() (networkConfig string, err error) {
10531047
if len(args.networkType) > 0 {
10541048
if args.networkType != ocm.NetworkTypeOvn && args.networkType != ocm.NetworkTypeOvnAlias {
1055-
err = fmt.Errorf("Incorrect network type '%s', please use '%s' or remove the flag",
1049+
err = fmt.Errorf("incorrect network type '%s', please use '%s' or remove the flag",
10561050
args.networkType, ocm.NetworkTypeOvn)
10571051
} else {
10581052
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
10881082
argValuePtr *string, err error) {
10891083
if cmd.Flags().Changed("audit-log-arn") {
10901084
if !aws.IsHostedCP(cluster) {
1091-
return nil, fmt.Errorf("Audit log forwarding to AWS CloudWatch is only supported for Hosted Control Plane clusters")
1085+
return nil, fmt.Errorf("audit log forwarding to AWS CloudWatch is only supported for Hosted Control Plane clusters")
10921086

10931087
}
10941088
if auditLogArn != "" && !aws.RoleArnRE.MatchString(auditLogArn) {
1095-
return nil, fmt.Errorf("Expected a valid value for audit-log-arn matching %s", aws.RoleArnRE.String())
1089+
return nil, fmt.Errorf("expected a valid value for audit-log-arn matching %s", aws.RoleArnRE.String())
10961090
}
10971091
argValuePtr := new(string)
10981092
*argValuePtr = auditLogArn
@@ -1129,7 +1123,7 @@ func auditLogInteractivePrompt(r *rosa.Runtime, cmd *cobra.Command, cluster *cmv
11291123
Required: true,
11301124
})
11311125
if err != nil {
1132-
return nil, fmt.Errorf("Expected a valid value: %s", err)
1126+
return nil, fmt.Errorf("expected a valid value: %s", err)
11331127
}
11341128
if requestAuditLogForwarding {
11351129

@@ -1146,7 +1140,7 @@ func auditLogInteractivePrompt(r *rosa.Runtime, cmd *cobra.Command, cluster *cmv
11461140
},
11471141
})
11481142
if err != nil {
1149-
return nil, fmt.Errorf("Expected a valid value for audit-log-arn: %s", err)
1143+
return nil, fmt.Errorf("expected a valid value for audit-log-arn: %s", err)
11501144
}
11511145
*auditLogRolePtr = auditLogRoleValue
11521146
return auditLogRolePtr, nil
@@ -1159,7 +1153,7 @@ func auditLogInteractivePrompt(r *rosa.Runtime, cmd *cobra.Command, cluster *cmv
11591153
Default: false,
11601154
})
11611155
if err != nil {
1162-
return nil, fmt.Errorf("Expected a valid value: %s", err)
1156+
return nil, fmt.Errorf("expected a valid value: %s", err)
11631157
}
11641158
if disableAuditLog {
11651159
*auditLogRolePtr = ""
@@ -1190,7 +1184,7 @@ func BuildClusterConfigWithRegistry(clusterConfig ocm.Spec, allowedRegistries []
11901184
ca, err := clusterregistryconfig.BuildAdditionalTrustedCAFromInputFile(additionalTrustedCa)
11911185
if err != nil {
11921186
return clusterConfig, fmt.Errorf(
1193-
"Failed to build the additional trusted ca from file %s, got error: %s",
1187+
"failed to build the additional trusted ca from file %s, got error: %s",
11941188
additionalTrustedCa, err)
11951189
}
11961190
clusterConfig.AdditionalTrustedCa = ca

‎pkg/helper/helpers.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,3 +268,67 @@ func FilterEmptyStrings(strings []string) []string {
268268
}
269269
return filteredResult
270270
}
271+
272+
type URLCredentialValidation struct {
273+
Field string
274+
InvalidChar rune
275+
HasInvalid bool
276+
}
277+
278+
func ValidateURLCredentials(urlString string) *URLCredentialValidation {
279+
if !strings.Contains(urlString, "@") || !strings.Contains(urlString, "://") {
280+
return &URLCredentialValidation{HasInvalid: false}
281+
}
282+
283+
schemeIdx := strings.Index(urlString, "://")
284+
if schemeIdx == -1 {
285+
return &URLCredentialValidation{HasInvalid: false}
286+
}
287+
288+
rest := urlString[schemeIdx+3:]
289+
atIdx := strings.LastIndex(rest, "@")
290+
if atIdx == -1 {
291+
return &URLCredentialValidation{HasInvalid: false}
292+
}
293+
294+
userinfo := rest[:atIdx]
295+
296+
// Check for multiple @ symbols first (indicates @ in credentials)
297+
if strings.Count(urlString, "@") > 1 {
298+
return &URLCredentialValidation{
299+
Field: "password",
300+
InvalidChar: '@',
301+
HasInvalid: true,
302+
}
303+
}
304+
305+
// Check username and password separately
306+
colonIdx := strings.Index(userinfo, ":")
307+
if colonIdx == -1 {
308+
return checkForInvalidChars(userinfo, "username")
309+
}
310+
311+
username := userinfo[:colonIdx]
312+
if result := checkForInvalidChars(username, "username"); result.HasInvalid {
313+
return result
314+
}
315+
316+
password := userinfo[colonIdx+1:]
317+
return checkForInvalidChars(password, "password")
318+
}
319+
320+
func checkForInvalidChars(value, field string) *URLCredentialValidation {
321+
invalidChars := "/:#?[]@!$&'()*+,;="
322+
323+
for _, char := range value {
324+
if strings.ContainsRune(invalidChars, char) {
325+
return &URLCredentialValidation{
326+
Field: field,
327+
InvalidChar: char,
328+
HasInvalid: true,
329+
}
330+
}
331+
}
332+
333+
return &URLCredentialValidation{HasInvalid: false}
334+
}

‎pkg/interactive/validation.go‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,9 @@ func IsURLHttps(val interface{}) error {
8989
if err != nil {
9090
return err
9191
}
92+
if parsedUri == nil {
93+
return nil
94+
}
9295
if parsedUri.Scheme != helper.ProtocolHttps {
9396
return fmt.Errorf("expect URL '%s' to use an 'https://' scheme", val.(string))
9497
}
@@ -106,8 +109,20 @@ func _isUrl(val interface{}) (*url.URL, error) {
106109
if s == "" {
107110
return nil, nil
108111
}
109-
parsedUri, err := url.ParseRequestURI(fmt.Sprintf("%v", val))
110-
return parsedUri, err
112+
113+
if credValidation := helper.ValidateURLCredentials(s); credValidation.HasInvalid {
114+
return nil, fmt.Errorf("invalid URL: %s contains invalid character '%c'",
115+
credValidation.Field, credValidation.InvalidChar)
116+
}
117+
118+
parsedUri, err := url.Parse(fmt.Sprintf("%v", val))
119+
if err != nil {
120+
return nil, err
121+
}
122+
if parsedUri.Scheme == "" || parsedUri.Host == "" {
123+
return nil, fmt.Errorf("url must have a scheme and host")
124+
}
125+
return parsedUri, nil
111126
}
112127

113128
// IsCert validates whether the given filepath is a valid cert file

‎pkg/interactive/validation_test.go‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,134 @@ var _ = Describe("Validation", func() {
108108
})
109109
})
110110

111+
Context("IsURL", func() {
112+
It("accepts empty string", func() {
113+
err := IsURL("")
114+
Expect(err).NotTo(HaveOccurred())
115+
})
116+
117+
It("accepts nil", func() {
118+
err := IsURL(nil)
119+
Expect(err).NotTo(HaveOccurred())
120+
})
121+
122+
It("accepts valid http URL", func() {
123+
err := IsURL("http://example.com")
124+
Expect(err).NotTo(HaveOccurred())
125+
})
126+
127+
It("accepts valid https URL", func() {
128+
err := IsURL("https://example.com")
129+
Expect(err).NotTo(HaveOccurred())
130+
})
131+
132+
It("accepts URL with authentication", func() {
133+
err := IsURL("http://user:[email protected]:8080")
134+
Expect(err).NotTo(HaveOccurred())
135+
})
136+
137+
It("rejects URL with unencoded special characters in password", func() {
138+
err := IsURL("http://proxyuser:QvoZjyy/[email protected]:8080")
139+
Expect(err).To(HaveOccurred())
140+
})
141+
142+
It("accepts URL with percent-encoded special characters in password", func() {
143+
err := IsURL("http://proxyuser:QvoZjyy%[email protected]:8080")
144+
Expect(err).NotTo(HaveOccurred())
145+
})
146+
147+
It("rejects URL with unencoded @ in password", func() {
148+
err := IsURL("http://user:pass@[email protected]:8080")
149+
Expect(err).To(HaveOccurred())
150+
Expect(err.Error()).To(Equal("invalid URL: password contains invalid character '@'"))
151+
})
152+
153+
It("accepts URL with percent-encoded @ in password", func() {
154+
err := IsURL("http://user:pass%[email protected]:8080")
155+
Expect(err).NotTo(HaveOccurred())
156+
})
157+
158+
It("rejects URL with extra colon in password", func() {
159+
err := IsURL("http://user:pass:[email protected]:8080")
160+
Expect(err).To(HaveOccurred())
161+
Expect(err.Error()).To(Equal("invalid URL: password contains invalid character ':'"))
162+
})
163+
164+
It("accepts URL with percent-encoded colon in password", func() {
165+
err := IsURL("http://user:pass%[email protected]:8080")
166+
Expect(err).NotTo(HaveOccurred())
167+
})
168+
169+
It("rejects URL without scheme", func() {
170+
err := IsURL("example.com")
171+
Expect(err).To(HaveOccurred())
172+
Expect(err.Error()).To(ContainSubstring("url must have a scheme and host"))
173+
})
174+
175+
It("rejects URL without host", func() {
176+
err := IsURL("http://")
177+
Expect(err).To(HaveOccurred())
178+
Expect(err.Error()).To(ContainSubstring("url must have a scheme and host"))
179+
})
180+
181+
It("rejects non-string input", func() {
182+
err := IsURL(123)
183+
Expect(err).To(HaveOccurred())
184+
Expect(err.Error()).To(ContainSubstring("can only validate strings"))
185+
})
186+
})
187+
188+
Context("IsURLHttps", func() {
189+
It("accepts empty string", func() {
190+
err := IsURLHttps("")
191+
Expect(err).NotTo(HaveOccurred())
192+
})
193+
194+
It("accepts nil", func() {
195+
err := IsURLHttps(nil)
196+
Expect(err).NotTo(HaveOccurred())
197+
})
198+
199+
It("accepts valid https URL", func() {
200+
err := IsURLHttps("https://example.com")
201+
Expect(err).NotTo(HaveOccurred())
202+
})
203+
204+
It("accepts https URL with authentication", func() {
205+
err := IsURLHttps("https://user:[email protected]:8080")
206+
Expect(err).NotTo(HaveOccurred())
207+
})
208+
209+
It("rejects https URL with unencoded special characters in password", func() {
210+
err := IsURLHttps("https://proxyuser:QvoZjyy/[email protected]:8080")
211+
Expect(err).To(HaveOccurred())
212+
Expect(err.Error()).To(Equal("invalid URL: password contains invalid character '/'"))
213+
})
214+
215+
It("accepts https URL with percent-encoded special characters in password", func() {
216+
err := IsURLHttps("https://proxyuser:QvoZjyy%[email protected]:8080")
217+
Expect(err).NotTo(HaveOccurred())
218+
})
219+
220+
It("rejects http URL", func() {
221+
err := IsURLHttps("http://example.com")
222+
Expect(err).To(HaveOccurred())
223+
Expect(err.Error()).To(ContainSubstring("expect URL 'http://example.com' to use an 'https://' scheme"))
224+
})
225+
226+
It("rejects URL without scheme", func() {
227+
err := IsURLHttps("example.com")
228+
Expect(err).To(HaveOccurred())
229+
Expect(err.Error()).To(ContainSubstring("url must have a scheme and host"))
230+
})
231+
232+
It("rejects non-string input", func() {
233+
err := IsURLHttps(123)
234+
Expect(err).To(HaveOccurred())
235+
Expect(err.Error()).To(ContainSubstring("can only validate strings"))
236+
})
237+
})
238+
111239
})
112240

113241
var _ = Describe("SubnetsValidator", func() {

0 commit comments

Comments
 (0)