Enable PKCE Support #25

tippexs · 2020-12-02T13:45:55Z

This feature updates introduces the PKCE support for the OIDC reference implementation.

enable it by setting $oidc_pkce_enable to 1. To use the authorization code grant set the switch to 0

lcrilly · 2020-12-02T14:42:31Z

openid_connect.js

+    var h = c.createHmac('sha256', r.variables.oidc_hmac_key).update(noncePlain);
+    var nonceHash = h.digest('base64url');
+
+    // Redirect the client to the IdP login page with the cookies we need for state


This comment should be for the r.return line in the auth function

lcrilly · 2020-12-02T14:43:04Z

openid_connect.js

+        "auth_redir=" + r.variables.request_uri + "; " + r.variables.oidc_cookie_flags,
+        "auth_nonce=" + noncePlain + "; " + r.variables.oidc_cookie_flags ];
+
+    if ( r.variables.oidc_pkce_enable == 1 ) {


s/==/=== ?

NGINX handles variables always as strings. Therfore === will not work here. Possible solution would be something like String(r.variables.oidc_pkce_enable) === "1". But looks a little bit to hacky to me.

lcrilly · 2020-12-02T14:43:29Z

openid_connect.js

+        "auth_nonce=" + noncePlain + "; " + r.variables.oidc_cookie_flags ];
+
+    if ( r.variables.oidc_pkce_enable == 1 ) {
+


Style: unnecessary empty line

lcrilly · 2020-12-02T14:43:44Z

openid_connect.js

+    }
+
+    return authZUri;
+


Style: unnecessary empty line

lcrilly · 2020-12-02T14:45:56Z

openid_connect.js

@@ -7,6 +7,36 @@ var newSession = false; // Used by oidcAuth() and validateIdToken()

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

+
+function authZUriHandler(r) {


For discussion, should this function appear below the exported ones? I like the idea of being able to follow the logic by looking at this file. So when you initially see a helper function, it might be confusing?

Its now at the bottom of the js file.

lcrilly · 2020-12-02T14:47:36Z

openid_connect.js

+        var pkce_code_challenge = c.createHash('sha256').update(pkce_code_verifier).digest('base64url');
+        r.variables.pkce_code_verifier = pkce_code_verifier;
+
+        authZUri = "?response_type=code&scope=" + r.variables.oidc_scopes + "&code_challenge_method=S256&code_challenge="+pkce_code_challenge+"&client_id=" + r.variables.oidc_client + "&state="+ r.variables.pkce_id +"&redirect_uri="+ r.variables.redirect_base + r.variables.redir_location + "&nonce=" + nonceHash;


Style: spaces between strings and + are inconsistent. Recommend single whitespace between the + concatenation operator

lcrilly · 2020-12-02T14:48:31Z

openid_connect.js

+
+        authZUri = "?response_type=code&scope=" + r.variables.oidc_scopes + "&code_challenge_method=S256&code_challenge="+pkce_code_challenge+"&client_id=" + r.variables.oidc_client + "&state="+ r.variables.pkce_id +"&redirect_uri="+ r.variables.redirect_base + r.variables.redir_location + "&nonce=" + nonceHash;
+    } else {
+        authZUri = "?response_type=code&scope=" + r.variables.oidc_scopes + "&client_id=" + r.variables.oidc_client + "&state=0&redirect_uri="+ r.variables.redirect_base + r.variables.redir_location + "&nonce=" + nonceHash;


Should the common parts be prepared in advance, so that we're only appending the special parts?

lcrilly · 2020-12-02T14:51:58Z

openid_connect.js

+        var pkce_code_verifier = c.createHmac('sha256', r.variables.oidc_hmac_key).update(String(Math.random())).digest('hex');
+        r.variables.pkce_id = c.createHash('sha256').update(String(Math.random())).digest('base64url');
+        var pkce_code_challenge = c.createHash('sha256').update(pkce_code_verifier).digest('base64url');
+        r.variables.pkce_code_verifier = pkce_code_verifier;


A comment would be helpful to show that this variable assignment is actually creating a keyval entry

refactoring done

lcrilly · 2020-12-02T14:55:18Z

openid_connect.js

+    var internalTokenEndpoint = null;
+    var internalTokenEndpointUri = null;
+
+    if ( r.variables.oidc_pkce_enable == 1 ) {


Why not pass these as query params instead of needing a completely separate location block? Seems to be adding a lot of extra config lines unnecessarily, but I might have missed something.

Totally agree. Let me rewrite this part and use a single location instead of two.

lcrilly · 2020-12-02T14:56:02Z

openid_connect.server_conf

@@ -1,5 +1,6 @@
    # Advanced configuration START
    set $internal_error_message "NGINX / OpenID Connect login failure\n";
+    set $pkce_id "";


Why do we need this? Add a comment to explain

lcrilly · 2020-12-02T14:58:13Z

openid_connect_configuration.conf

@@ -63,11 +67,13 @@ 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 state=conf.d/oidc_id_tokens.json timeout=1h;
 keyval_zone zone=refresh_tokens:1M state=conf.d/refresh_tokens.json timeout=8h;
+keyval_zone zone=pkce:1M state=conf.d/pkce.json timeout=2m;


Do we need a state file for pkce? Suggest separating this from the other keyvals because as ask the customer to adjust the timeouts.

agreed! Will change this.

- Code cleanup and refactoring - New function for setting IdP Client Auth variables

- Added PKCE documentation to README - Adjusted pkce keyvalue zones size and timeout

lcrilly · 2020-12-08T12:15:52Z

Looks good, squashing

Enable PKCE Support

f179e5a

lcrilly reviewed Dec 2, 2020

View reviewed changes

Timo Stark added 3 commits December 8, 2020 11:18

Enable PKCE Support

b06f34d

- Code cleanup and refactoring - New function for setting IdP Client Auth variables

Enable PKCE Support

3aa8baf

- Code cleanup and refactoring - New function for setting IdP Client Auth variables

PKCE Support

47f8427

- Added PKCE documentation to README - Adjusted pkce keyvalue zones size and timeout

tippexs marked this pull request as ready for review December 8, 2020 12:14

lcrilly merged commit 60ea2c2 into nginxinc:master Dec 8, 2020

		"auth_nonce=" + noncePlain + "; " + r.variables.oidc_cookie_flags ];

		if ( r.variables.oidc_pkce_enable == 1 ) {

		@@ -7,6 +7,36 @@ var newSession = false; // Used by oidcAuth() and validateIdToken()

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


		function authZUriHandler(r) {

Enable PKCE Support #25

Enable PKCE Support #25

Uh oh!

Conversation

tippexs commented Dec 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcrilly commented Dec 8, 2020

Uh oh!

Uh oh!