8000 Fix/OIDC - relaxed OIDC scope validation (#3863) · nginx/kubernetes-ingress@ab67547 · GitHub
[go: up one dir, main page]
Skip to content

Commit ab67547

Browse files
jjngxjjngx
authored
Fix/OIDC - relaxed OIDC scope validation (#3863)
1 parent 18a37de commit ab67547

File tree

4 files changed

+189
-119
lines changed

4 files changed

+189
-119
lines changed

‎docs/content/configuration/policy-resource.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -442,7 +442,7 @@ The OIDC policy defines a few internal locations that can't be customized: `/_jw
442442
|``authExtraArgs`` | A list of extra URL arguments to pass to the authorization endpoint provided by your OpenID Connect provider. Arguments must be URL encoded, multiple arguments may be included in the list, for example ``[ arg1=value1, arg2=value2 ]`` | ``string[]`` | No |
443443
|``tokenEndpoint`` | URL for the token endpoint provided by your OpenID Connect provider. | ``string`` | Yes |
444444
|``jwksURI`` | URL for the JSON Web Key Set (JWK) document provided by your OpenID Connect provider. | ``string`` | Yes |
445-
|``scope`` | List of OpenID Connect scopes. Possible values are ``openid``, ``profile``, ``email``, ``address`` and ``phone``. The scope ``openid`` always needs to be present and others can be added concatenating them with a ``+`` sign, for example ``openid+profile+email``. The default is ``openid``. | ``string`` | No |
445+
|``scope`` | List of OpenID Connect scopes. The scope ``openid`` always needs to be present and others can be added concatenating them with a ``+`` sign, for example ``openid+profile+email``, ``openid+email+userDefinedScope``. The default is ``openid``. | ``string`` | No |
446446
|``redirectURI`` | Allows overriding the default redirect URI. The default is ``/_codexch``. | ``string`` | No |
447447
|``zoneSyncLeeway`` | Specifies the maximum timeout in milliseconds for synchronizing ID/access tokens and shared values between Ingress Controller pods. The default is ``200``. | ``int`` | No |
448448
|``accessTokenEnable`` | Option of whether Bearer token is used to authorize NGINX to access protected backend. | ``boolean`` | No |

‎pkg/apis/configuration/validation/common.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ func validateStringWithVariables(str string, fieldPath *field.Path, specialVars
105105
return field.ErrorList{field.Invalid(fieldPath, str, "must not end with $")}
106106
}
107107

108+
allErrs := field.ErrorList{}
108109
for i, c := range str {
109110
if c == '$' {
110111
msg := "variables must be enclosed in curly braces, for example ${host}"
@@ -119,7 +120,6 @@ func validateStringWithVariables(str string, fieldPath *field.Path, specialVars
119120
}
120121
}
121122

122-
allErrs := field.ErrorList{}
123123
nginxVars := captureVariables(str)
124124
for _, nVar := range nginxVars {
125125
special := false
@@ -157,7 +157,6 @@ func validateOffset(offset string, fieldPath *field.Path) field.ErrorList {
157157
if offset == "" {
158158
return nil
159159
}
160-
161160
if _, err := configs.ParseOffset(offset); err != nil {
162161
msg := validation.RegexError(offsetErrMsg, configs.OffsetFmt, "16", "32k", "64M", "2G")
163162
return field.ErrorList{field.Invalid(fieldPath, offset, msg)}

‎pkg/apis/configuration/validation/policy.go‎

Lines changed: 40 additions & 46 deletions

0 commit comments

Comments
 (0)
0
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"regexp"
88
"strconv"
99
"strings"
10+
"unicode"
1011

1112
v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1"
1213
"k8s.io/apimachinery/pkg/util/validation"
@@ -185,25 +186,21 @@ func validateJWT(jwt *v1.JWTAuth, fieldPath *field.Path) field.ErrorList {
185186
}
186187

187188
func validateBasic(basic *v1.BasicAuth, fieldPath *field.Path) field.ErrorList {
188-
allErrs := field.ErrorList{}
189+
if basic.Secret == "" {
190+
return field.ErrorList{field.Required(fieldPath.Child("secret"), "")}
191+
}
189192

193+
allErrs := field.ErrorList{}
190194
if basic.Realm != "" {
191195
allErrs = append(allErrs, validateRealm(basic.Realm, fieldPath.Child("realm"))...)
192196
}
193-
194-
if basic.Secret == "" {
195-
return append(allErrs, field.Required(fieldPath.Child("secret"), ""))
196-
}
197-
allErrs = append(allErrs, validateSecretName(basic.Secret, fieldPath.Child("secret"))...)
198-
199-
return allErrs
197+
return append(allErrs, validateSecretName(basic.Secret, fieldPath.Child("secret"))...)
200198
}
201199

202200
func validateIngressMTLS(ingressMTLS *v1.IngressMTLS, fieldPath *field.Path) field.ErrorList {
203201
if ingressMTLS.ClientCertSecret == "" {
204202
return field.ErrorList{field.Required(fieldPath.Child("clientCertSecret"), "")}
205203
}
206-
207204
allErrs := validateSecretName(ingressMTLS.ClientCertSecret, fieldPath.Child("clientCertSecret"))
208205
allErrs = append(allErrs, validateIngressMTLSVerifyClient(ingressMTLS.VerifyClient, fieldPath.Child("verifyClient"))...)
209206
if ingressMTLS.VerifyDepth != nil {
@@ -223,10 +220,7 @@ func validateEgressMTLS(egressMTLS *v1.EgressMTLS, fieldPath *field.Path) field.
223220
if egressMTLS.VerifyDepth != nil {
224221
allErrs = append(allErrs, validatePositiveIntOrZero(*egressMTLS.VerifyDepth, fieldPath.Child("verifyDepth"))...)
225222
}
226-
227-
allErrs = append(allErrs, validateSSLName(egressMTLS.SSLName, fieldPath.Child("sslName"))...)
228-
229-
return allErrs
223+
return append(allErrs, validateSSLName(egressMTLS.SSLName, fieldPath.Child("sslName"))...)
230224
}
231225

232226
func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList {
@@ -264,9 +258,7 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList {
264258
allErrs = append(allErrs, validateURL(oidc.TokenEndpoint, fieldPath.Child("tokenEndpoint"))...)
265259
allErrs = append(allErrs, validateURL(oidc.JWKSURI, fieldPath.Child("jwksURI"))...)
266260
allErrs = append(allErrs, validateSecretName(oidc.ClientSecret, fieldPath.Child("clientSecret"))...)
267-
allErrs = append(allErrs, validateClientID(oidc.ClientID, fieldPath.Child("clientID"))...)
268-
269-
return allErrs
261+
return append(allErrs, validateClientID(oidc.ClientID, fieldPath.Child("clientID"))...)
270262
}
271263

272264
func validateWAF(waf *v1.WAF, fieldPath *field.Path) field.ErrorList {
@@ -296,7 +288,6 @@ func validateWAF(waf *v1.WAF, fieldPath *field.Path) field.ErrorList {
296288
if waf.SecurityLog != nil {
297289
allErrs = append(allErrs, validateLogConf(waf.SecurityLog.ApLogConf, waf.SecurityLog.LogDest, fieldPath.Child("securityLog"))...)
298290
}
299-
300291
return allErrs
301292
}
302293

@@ -317,43 +308,49 @@ func validateLogConf(logConf, logDest string, fieldPath *field.Path) field.Error
317308
}
318309

319310
func validateClientID(client string, fieldPath *field.Path) field.ErrorList {
320-
allErrs := field.ErrorList{}
321-
322311
// isValidHeaderValue checks for $ and " in the string
323312
if isValidHeaderValue(client) != nil {
324-
allErrs = append(allErrs, field.Invalid(
313+
return field.ErrorList{field.Invalid(
325314
fieldPath,
326315
client,
327316
`invalid string. String must contain valid ASCII characters, must have all '"' escaped and must not contain any '$' or end with an unescaped '\'
328-
`))
317+
`)}
329318
}
330-
331-
return allErrs
319+
return nil
332320
}
333321

334-
var validScopes = map[string]bool{
335-
"openid": true,
336-
"profile": true,
337-
"email": true,
338-
"address": true,
339-
"phone": true,
322+
// Allowed unicode ranges in OIDC scope tokens.
323+
// Ref. https://datatracker.ietf.org/doc/html/rfc6749#section-3.3
324+
var validOIDCScopeRanges = &unicode.RangeTable{
325+
R16: []unicode.Range16{
326+
{0x21, 0x21, 1},
327+
{0x23, 0x5B, 1},
328+
{0x5D, 0x7E, 1},
329+
},
340330
}
341331

342-
// https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims
332+
// validateOIDCScope takes a scope representing OIDC scope tokens and
333+
// checks if the scope is valid. OIDC scope must contain scope token
334+
// "openid". Additionally, custom scope tokens can be added to the scope.
335+
//
336+
// Ref:
337+
// - https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims
338+
//
339+
// Scope tokens must be separated by "+", and the "+" can't be a part of the token.
343340
func validateOIDCScope(scope string, fieldPath *field.Path) field.ErrorList {
344341
if !strings.Contains(scope, "openid") {
345-
return field.ErrorList{field.Required(fieldPath, "openid scope")}
342+
return field.ErrorList{field.Required(fieldPath, "openid is required")}
346343
}
347344

348-
allErrs := field.ErrorList{}
349-
s := strings.Split(scope, "+")
350-
for _, v := range s {
351-
if !validScopes[v] {
352-
msg := fmt.Sprintf("invalid Scope. Accepted scopes are: %v", mapToPrettyString(validScopes))
353-
allErrs = append(allErrs, field.Invalid(fieldPath, v, msg))
345+
for _, token := range strings.Split(scope, "+") {
346+
for _, v := range token {
347+
if !unicode.Is(validOIDCScopeRanges, v) {
348+
msg := fmt.Sprintf("not allowed character %v in scope %s", v, scope)
349+
return field.ErrorList{field.Invalid(fieldPath, scope, msg)}
350+
}
354351< B5F2 /td>
}
355352
}
356-
return allErrs
353+
return nil
357354
}
358355

359356
func validateURL(name string, fieldPath *field.Path) field.ErrorList {
@@ -460,7 +457,6 @@ func validateRateLimitZoneSize(zoneSize string, fieldPath *field.Path) field.Err
460457
if err == nil && kbZoneSizeNum < 32 || mbErr == nil && mbZoneSizeNum == 0 {
461458
allErrs = append(allErrs, field.Invalid(fieldPath, zoneSize, "must be greater than 31k"))
462459
}
463-
464460
return allErrs
465461
}
466462

@@ -478,13 +474,11 @@ func validateRateLimitKey(key string, fieldPath *field.Path, isPlus bool) field.
478474
if key == "" {
479475
return field.ErrorList{field.Required(fieldPath, "")}
480476
}
481-
482477
allErrs := field.ErrorList{}
483478
if err := ValidateEscapedString(key, `Hello World! \n`, `\"${request_uri}\" is unavailable. \n`); err != nil {
484479
allErrs = append(allErrs, field.Invalid(fieldPath, key, err.Error()))
485480
}
486-
allErrs = append(allErrs, validateStringWithVariables(key, fieldPath, rateLimitKeySpecialVariables, rateLimitKeyVariables, isPlus)...)
487-
return allErrs
481+
return append(allErrs, validateStringWithVariables(key, fieldPath, rateLimitKeySpecialVariables, rateLimitKeyVariables, isPlus)...)
488482
}
489483

490484
var jwtTokenSpecialVariables = []string{"arg_", "http_", "cookie_"}
@@ -508,12 +502,12 @@ func validateJWTToken(token string, fieldPath *field.Path) field.ErrorList {
508502
break
509503
}
510504
}
511-
if special {
512-
// validateJWTToken is called only when NGINX Plus is running
513-
return validateSpecialVariable(nVar, fieldPath, true)
514-
} else {
505+
506+
if !special {
515507
return field.ErrorList{field.Invalid(fieldPath, token, "must only have special vars")}
516508
}
509+
// validateJWTToken is called only when NGINX Plus is running
510+
return validateSpecialVariable(nVar, fieldPath, true)
517511
}
518512

519513
var validLogLevels = map[string]bool{