Skip to content

Commit

Permalink
Sync changes from OIDC repo, add field in policy (#2654)
Browse files Browse the repository at this point in the history
  • Loading branch information
lucacome authored May 13, 2022
1 parent 4b05f3d commit d8d4d3e
Show file tree
Hide file tree
Showing 13 changed files with 144 additions and 65 deletions.
2 changes: 2 additions & 0 deletions deployments/common/crds/k8s.nginx.org_policies.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ spec:
type: string
tokenEndpoint:
type: string
zoneSyncLeeway:
type: integer
rateLimit:
description: RateLimit defines a rate limit policy.
type: object
Expand Down
2 changes: 2 additions & 0 deletions deployments/helm-chart/crds/k8s.nginx.org_policies.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ spec:
type: string
tokenEndpoint:
type: string
zoneSyncLeeway:
type: integer
rateLimit:
description: RateLimit defines a rate limit policy.
type: object
Expand Down
7 changes: 4 additions & 3 deletions docs/content/configuration/policy-resource.md
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ NGINX Plus will pass the ID of an authenticated user to the backend in the HTTP

#### Prerequisites

In order to use OIDC, you need to enable [zone synchronization](https://docs.nginx.com/nginx/admin-guide/high-availability/zone_sync/). If you don't set up zone synchronization, NGINX Plus will fail to reload.
In order to use OIDC, you need to enable [zone synchronization](https://docs.nginx.com/nginx/admin-guide/high-availability/zone_sync/). If you don't set up zone synchronization, NGINX Plus will fail to reload.
You also need to configure a resolver, which NGINX Plus will use to resolve the IDP authorization endpoint. You can find an example configuration [in our GitHub repo](https://github.com/nginxinc/kubernetes-ingress/blob/v2.2.0/examples/custom-resources/oidc#step-7---configure-nginx-plus-zone-synchronization-and-resolver).

> **Note**: The configuration in the example doesn't enable TLS and the synchronization between the replica happens in clear text. This could lead to the exposure of tokens.
Expand All @@ -314,8 +314,9 @@ The OIDC policy defines a few internal locations that can't be customized: `/_jw
|``authEndpoint`` | URL for the authorization endpoint provided by your OpenID Connect provider. | ``string`` | Yes |
|``tokenEndpoint`` | URL for the token endpoint provided by your OpenID Connect provider. | ``string`` | Yes |
|``jwksURI`` | URL for the JSON Web Key Set (JWK) document provided by your OpenID Connect provider. | ``string`` | Yes |
|``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 |
|``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 |
|``redirectURI`` | Allows overriding the default redirect URI. The default is ``/_codexch``. | ``string`` | No |
|``zoneSyncLeeway`` | Specifies the maximum timeout in milliseconds for synchronizing ID tokens and shared values between Ingress Controller pods. The default is ``200``. | ``int`` | No |
{{% /table %}}

> **Note**: Only one OIDC policy can be referenced in a VirtualServer and its VirtualServerRoutes. However, the same policy can still be applied to different routes in the VirtualServer and VirtualServerRoutes.
Expand Down Expand Up @@ -366,7 +367,7 @@ waf:
logDest: "syslog:server=syslog-svc.default:514"
- enable: true
apLogConf: "default/logconf"
logDest: "syslog:server=syslog-svc-secondary.default:514"
logDest: "syslog:server=syslog-svc-secondary.default:514"
```
> Note: The field `waf.securityLog` is deprecated and will be removed in future releases.It will be ignored if `waf.securityLogs` is populated.
> Note: The feature is implemented using the NGINX Plus [NGINX App Protect Module](https://docs.nginx.com/nginx-app-protect/configuration/).
Expand Down
5 changes: 5 additions & 0 deletions internal/configs/oidc/oidc_common.conf
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,19 @@ map $http_x_forwarded_proto $proto {
default $http_x_forwarded_proto;
}

# JWK Set will be fetched from $oidc_jwks_uri and cached here - ensure writable by nginx user
proxy_cache_path /var/cache/nginx/jwk levels=1 keys_zone=jwk:64k max_size=1m;

# Change timeout values to at least the validity period of each token type
keyval_zone zone=oidc_id_tokens:1M timeout=1h sync;
keyval_zone zone=refresh_tokens:1M timeout=8h sync;
#keyval_zone zone=oidc_pkce:128K timeout=90s sync; # Temporary storage for PKCE code verifier.

keyval $cookie_auth_token $session_jwt zone=oidc_id_tokens; # Exchange cookie for JWT
keyval $cookie_auth_token $refresh_token zone=refresh_tokens; # Exchange cookie for refresh token
keyval $request_id $new_session zone=oidc_id_tokens; # For initial session creation
keyval $request_id $new_refresh zone=refresh_tokens; # ''
#keyval $pkce_id $pkce_code_verifier zone=oidc_pkce;

auth_jwt_claim_set $jwt_audience aud; # In case aud is an array
js_import oidc from oidc/openid_connect.js;
33 changes: 27 additions & 6 deletions internal/configs/oidc/openid_connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,30 @@ var newSession = false; // Used by oidcAuth() and validateIdToken()

export default { auth, codeExchange, validateIdToken, logout };

function auth(r) {
function retryOriginalRequest(r) {
delete r.headersOut["WWW-Authenticate"]; // Remove evidence of original failed auth_jwt
r.internalRedirect(r.variables.uri + r.variables.is_args + (r.variables.args || ''));
}

// If the ID token has not been synced yet, poll the variable every 100ms until
// get a value or after a timeout.
function waitForSessionSync(r, timeLeft) {
if (r.variables.session_jwt) {
retryOriginalRequest(r);
} else if (timeLeft > 0) {
setTimeout(waitForSessionSync, 100, r, timeLeft - 100);
} else {
auth(r, true);
}
}

function auth(r, afterSyncCheck) {
// If a cookie was sent but the ID token is not in the key-value database, wait for the token to be in sync.
if (r.variables.cookie_auth_token && !r.variables.session_jwt && !afterSyncCheck && r.variables.zone_sync_leeway > 0) {
waitForSessionSync(r, r.variables.zone_sync_leeway);
return;
}

if (!r.variables.refresh_token || r.variables.refresh_token == "-") {
newSession = true;

Expand Down Expand Up @@ -83,14 +106,12 @@ function auth(r) {
r.variables.session_jwt = tokenset.id_token; // Update key-value store

// Update refresh token (if we got a new one)
// 12.2021 - In rare cases the IdP does not include the refresh-token in the response. The rt will be undefined in this case.
if (r.variables.refresh_token != tokenset.refresh_token && tokenset.refresh_token != undefined) {
if (r.variables.refresh_token != tokenset.refresh_token) {
r.log("OIDC replacing previous refresh token (" + r.variables.refresh_token + ") with new value: " + tokenset.refresh_token);
r.variables.refresh_token = tokenset.refresh_token; // Update key-value store
}

delete r.headersOut["WWW-Authenticate"]; // Remove evidence of original failed auth_jwt
r.internalRedirect(r.variables.request_uri); // Continue processing original request
retryOriginalRequest(r); // Continue processing original request
}
);
} catch (e) {
Expand All @@ -104,7 +125,7 @@ function auth(r) {

function codeExchange(r) {
// First check that we received an authorization code from the IdP
if (r.variables.arg_code.length == 0) {
if (r.variables.arg_code == undefined || r.variables.arg_code.length == 0) {
if (r.variables.arg_error) {
r.error("OIDC error receiving authorization code from IdP: " + r.variables.arg_error_description);
} else {
Expand Down
15 changes: 8 additions & 7 deletions internal/configs/version2/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,14 @@ type EgressMTLS struct {

// OIDC holds OIDC configuration data.
type OIDC struct {
AuthEndpoint string
ClientID string
ClientSecret string
JwksURI string
Scope string
TokenEndpoint string
RedirectURI string
AuthEndpoint string
ClientID string
ClientSecret string
JwksURI string
Scope string
TokenEndpoint string
RedirectURI string
ZoneSyncLeeway int
}

// WAF defines WAF configuration.
Expand Down
1 change: 1 addition & 0 deletions internal/configs/version2/nginx-plus.virtualserver.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ server {
set $oidc_pkce_enable 0;
set $oidc_logout_redirect "/_logout";
set $oidc_hmac_key "{{ $s.VSName }}";
set $zone_sync_leeway {{ $oidc.ZoneSyncLeeway }};

set $oidc_authz_endpoint "{{ $oidc.AuthEndpoint }}";
set $oidc_token_endpoint "{{ $oidc.TokenEndpoint }}";
Expand Down
15 changes: 8 additions & 7 deletions internal/configs/virtualserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -991,13 +991,14 @@ func (p *policiesCfg) addOIDCConfig(
}

oidcPolCfg.oidc = &version2.OIDC{
AuthEndpoint: oidc.AuthEndpoint,
TokenEndpoint: oidc.TokenEndpoint,
JwksURI: oidc.JWKSURI,
ClientID: oidc.ClientID,
ClientSecret: string(clientSecret),
Scope: scope,
RedirectURI: redirectURI,
AuthEndpoint: oidc.AuthEndpoint,
TokenEndpoint: oidc.TokenEndpoint,
JwksURI: oidc.JWKSURI,
ClientID: oidc.ClientID,
ClientSecret: string(clientSecret),
Scope: scope,
RedirectURI: redirectURI,
ZoneSyncLeeway: generateIntFromPointer(oidc.ZoneSyncLeeway, 200),
}
oidcPolCfg.key = polKey
}
Expand Down
45 changes: 24 additions & 21 deletions internal/configs/virtualserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2894,13 +2894,14 @@ func TestGeneratePolicies(t *testing.T) {
},
Spec: conf_v1.PolicySpec{
OIDC: &conf_v1.OIDC{
AuthEndpoint: "http://example.com/auth",
TokenEndpoint: "http://example.com/token",
JWKSURI: "http://example.com/jwks",
ClientID: "client-id",
ClientSecret: "oidc-secret",
Scope: "scope",
RedirectURI: "/redirect",
AuthEndpoint: "http://example.com/auth",
TokenEndpoint: "http://example.com/token",
JWKSURI: "http://example.com/jwks",
ClientID: "client-id",
ClientSecret: "oidc-secret",
Scope: "scope",
RedirectURI: "/redirect",
ZoneSyncLeeway: createPointerFromInt(20),
},
},
},
Expand Down Expand Up @@ -3891,13 +3892,14 @@ func TestGeneratePoliciesFails(t *testing.T) {
context: "route",
oidcPolCfg: &oidcPolicyCfg{
oidc: &version2.OIDC{
AuthEndpoint: "https://foo.com/auth",
TokenEndpoint: "https://foo.com/token",
JwksURI: "https://foo.com/certs",
ClientID: "foo",
ClientSecret: "super_secret_123",
RedirectURI: "/_codexch",
Scope: "openid",
AuthEndpoint: "https://foo.com/auth",
TokenEndpoint: "https://foo.com/token",
JwksURI: "https://foo.com/certs",
ClientID: "foo",
ClientSecret: "super_secret_123",
RedirectURI: "/_codexch",
Scope: "openid",
ZoneSyncLeeway: 0,
},
key: "default/oidc-policy-1",
},
Expand Down Expand Up @@ -3991,13 +3993,14 @@ func TestGeneratePoliciesFails(t *testing.T) {
},
expectedOidc: &oidcPolicyCfg{
&version2.OIDC{
AuthEndpoint: "https://foo.com/auth",
TokenEndpoint: "https://foo.com/token",
JwksURI: "https://foo.com/certs",
ClientID: "foo",
ClientSecret: "super_secret_123",
RedirectURI: "/_codexch",
Scope: "openid",
AuthEndpoint: "https://foo.com/auth",
TokenEndpoint: "https://foo.com/token",
JwksURI: "https://foo.com/certs",
ClientID: "foo",
ClientSecret: "super_secret_123",
RedirectURI: "/_codexch",
Scope: "openid",
ZoneSyncLeeway: 200,
},
"default/oidc-policy",
},
Expand Down
15 changes: 8 additions & 7 deletions pkg/apis/configuration/v1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -434,13 +434,14 @@ type EgressMTLS struct {

// OIDC defines an Open ID Connect policy.
type OIDC struct {
AuthEndpoint string `json:"authEndpoint"`
TokenEndpoint string `json:"tokenEndpoint"`
JWKSURI string `json:"jwksURI"`
ClientID string `json:"clientID"`
ClientSecret string `json:"clientSecret"`
Scope string `json:"scope"`
RedirectURI string `json:"redirectURI"`
AuthEndpoint string `json:"authEndpoint"`
TokenEndpoint string `json:"tokenEndpoint"`
JWKSURI string `json:"jwksURI"`
ClientID string `json:"clientID"`
ClientSecret string `json:"clientSecret"`
Scope string `json:"scope"`
RedirectURI string `json:"redirectURI"`
ZoneSyncLeeway *int `json:"zoneSyncLeeway"`
}

// WAF defines an WAF policy.
Expand Down
7 changes: 6 additions & 1 deletion pkg/apis/configuration/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions pkg/apis/configuration/validation/policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,10 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList {
allErrs = append(allErrs, validatePath(oidc.RedirectURI, fieldPath.Child("redirectURI"))...)
}

if oidc.ZoneSyncLeeway != nil {
allErrs = append(allErrs, validatePositiveIntOrZero(*oidc.ZoneSyncLeeway, fieldPath.Child("zoneSyncLeeway"))...)
}

allErrs = append(allErrs, validateURL(oidc.AuthEndpoint, fieldPath.Child("authEndpoint"))...)
allErrs = append(allErrs, validateURL(oidc.TokenEndpoint, fieldPath.Child("tokenEndpoint"))...)
allErrs = append(allErrs, validateURL(oidc.JWKSURI, fieldPath.Child("jwksURI"))...)
Expand Down
58 changes: 45 additions & 13 deletions pkg/apis/configuration/validation/policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,13 @@ func TestValidatePolicy(t *testing.T) {
policy: &v1.Policy{
Spec: v1.PolicySpec{
OIDC: &v1.OIDC{
AuthEndpoint: "https://foo.bar/auth",
TokenEndpoint: "https://foo.bar/token",
JWKSURI: "https://foo.bar/certs",
ClientID: "random-string",
ClientSecret: "random-secret",
Scope: "openid",
AuthEndpoint: "https://foo.bar/auth",
TokenEndpoint: "https://foo.bar/token",
JWKSURI: "https://foo.bar/certs",
ClientID: "random-string",
ClientSecret: "random-secret",
Scope: "openid",
ZoneSyncLeeway: createPointerFromInt(10),
},
},
},
Expand Down Expand Up @@ -191,6 +192,24 @@ func TestValidatePolicyFails(t *testing.T) {
enableAppProtect: false,
msg: "WAF policy with AP disabled",
},
{
policy: &v1.Policy{
Spec: v1.PolicySpec{
OIDC: &v1.OIDC{
AuthEndpoint: "https://foo.bar/auth",
TokenEndpoint: "https://foo.bar/token",
JWKSURI: "https://foo.bar/certs",
ClientID: "random-string",
ClientSecret: "random-secret",
Scope: "openid",
ZoneSyncLeeway: createPointerFromInt(-1),
},
},
},
isPlus: true,
enableOIDC: true,
msg: "OIDC policy with invalid ZoneSyncLeeway",
},
}
for _, test := range tests {
err := ValidatePolicy(test.policy, test.isPlus, test.enableOIDC, test.enableAppProtect)
Expand Down Expand Up @@ -852,13 +871,14 @@ func TestValidateOIDCValid(t *testing.T) {
}{
{
oidc: &v1.OIDC{
AuthEndpoint: "https://accounts.google.com/o/oauth2/v2/auth",
TokenEndpoint: "https://oauth2.googleapis.com/token",
JWKSURI: "https://www.googleapis.com/oauth2/v3/certs",
ClientID: "random-string",
ClientSecret: "random-secret",
Scope: "openid",
RedirectURI: "/foo",
AuthEndpoint: "https://accounts.google.com/o/oauth2/v2/auth",
TokenEndpoint: "https://oauth2.googleapis.com/token",
JWKSURI: "https://www.googleapis.com/oauth2/v3/certs",
ClientID: "random-string",
ClientSecret: "random-secret",
Scope: "openid",
RedirectURI: "/foo",
ZoneSyncLeeway: createPointerFromInt(20),
},
msg: "verify full oidc",
},
Expand Down Expand Up @@ -992,6 +1012,18 @@ func TestValidateOIDCInvalid(t *testing.T) {
},
msg: "invalid chars in clientID",
},
{
oidc: &v1.OIDC{
AuthEndpoint: "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/auth",
TokenEndpoint: "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/token",
JWKSURI: "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/certs",
ClientID: "foobar",
ClientSecret: "secret",
Scope: "openid",
ZoneSyncLeeway: createPointerFromInt(-1),
},
msg: "invalid zoneSyncLeeway value",
},
}

for _, test := range tests {
Expand Down

0 comments on commit d8d4d3e

Please sign in to comment.