PEM key file support #56

penumbra23 · 2020-07-24T19:47:46Z

added auth_jwt_use_keyfile and auth_jwt_keyfile_path for loading the PEM encoded public key from file (defaults to "/app/pub_key")

Add loading RS256 key from file

penumbra23 · 2020-07-24T19:49:08Z

Dockerfile

 # for compiling for epel7
 RUN yum -y install libxml2 libxslt libxml2-devel libxslt-devel gd gd-devel perl-ExtUtils-Embed geoip geoip-devel google-perftools google-perftools-devel

-# Jansson requires new cmake
-RUN yum -y install cmake3 && \


I've removed this as the Jansson build passes with cmake installed from previous step (I can return it, but it works also without it)

penumbra23 · 2020-07-24T19:50:11Z

Dockerfile

@@ -85,7 +69,7 @@ RUN wget http://nginx.org/download/nginx-$NGINX_VERSION.tar.gz && \
    rm nginx-$NGINX_VERSION.tar.gz && \
    ln -sf nginx-$NGINX_VERSION nginx && \
    cd /root/dl/nginx && \
-    ./configure --add-dynamic-module=../ngx-http-auth-jwt-module --prefix=/usr/share/nginx --sbin-path=/usr/sbin/nginx --modules-path=/usr/lib64/nginx/modules --conf-path=/etc/nginx/nginx.conf --error-log-path=/var/log/nginx/error.log --http-log-path=/var/log/nginx/access.log --http-client-body-temp-path=/var/lib/nginx/tmp/client_body --http-proxy-temp-path=/var/lib/nginx/tmp/proxy --http-fastcgi-temp-path=/var/lib/nginx/tmp/fastcgi --http-uwsgi-temp-path=/var/lib/nginx/tmp/uwsgi --http-scgi-temp-path=/var/lib/nginx/tmp/scgi --pid-path=/run/nginx.pid --lock-path=/run/lock/subsys/nginx --user=nginx --group=nginx --with-file-aio --with-ipv6 --with-http_ssl_module --with-http_v2_module --with-http_realip_module --with-stream_ssl_preread_module --with-http_addition_module --with-http_xslt_module=dynamic --with-http_image_filter_module=dynamic --with-http_sub_module --with-http_dav_module --with-http_flv_module --with-http_mp4_module --with-http_gunzip_module --with-http_gzip_static_module --with-http_random_index_module --with-http_secure_link_module --with-http_degradation_module --with-http_slice_module --with-http_stub_status_module --with-http_perl_module=dynamic --with-http_auth_request_module --with-mail=dynamic --with-mail_ssl_module --with-pcre --with-pcre-jit --with-stream=dynamic --with-stream_ssl_module --with-google_perftools_module --with-debug --with-cc-opt='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -std=gnu99' --with-ld-opt='-Wl,-z,relro -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -Wl,-E' && \


Here I've just added -Wno-unused-variable -Wno-error=unused-but-set-variable to suppress errors regarding unused variables (return value of fread isn't used)

penumbra23 · 2020-07-24T19:51:14Z

README.md

@@ -45,11 +45,13 @@ auth_jwt_loginurl "https://yourdomain.com/loginpage";
 auth_jwt_enabled on;
 auth_jwt_algorithm HS256; # or RS256
 auth_jwt_validate_email on;  # or off
+auth_jwt_use_keyfile off; # or on
+auth_jwt_keyfile_path "/app/pub_key";


Is this a valid default path? All my docker images are inside /app working directory, just a habit 😄

I think there perhaps shouldn't be a default, and it would be an error case to have auth_jwt_use_keyfile on and auth_jwt_keyfile_path not supplied (e.g. force the user to supply a path).

Makes sense, but there could also be a auth_jwt_key together with the auth_jwt_keyfile_path which adds ambiguity to what key is being used. Maybe an enum field auth_jwt_key_location (with values inline or file) would solve it also for future situations (for example having a remote keystore where the key is located).

fitzyjoe

Thanks so much @penumbra23 for the PR! Sorry it has taken me so long to get around to reviewing it. I think it would be an excellent addition to be able to refer to a PEM file instead of including the key's text in the nginx.conf. I left some comments below... I'm most concerned that your PR reads the key from the file every request. I think this would be best handled during the loading of the configuration instead. Also, I each new scenario needs a test in test.sh - maybe something like this:

test_jwt "Secure test with rs256 file" "/secure-rs256-file/" "200" "--header \"Authorization: Bearer ${VALID_RS256_JWT}\""

  test_jwt "Secure test with rs256 file and invalid JWT" "/secure-rs256-file/" "401" "--header \"Authorization: Bearer ${INVALID_RS256_JWT}\""

fitzyjoe · 2021-08-24T22:07:36Z

src/ngx_http_auth_jwt_module.c

@@ -177,8 +199,45 @@ static ngx_int_t ngx_http_auth_jwt_handler(ngx_http_request_t *r)
 	else if ( auth_jwt_algorithm.len == sizeof("RS256") - 1 && ngx_strncmp(auth_jwt_algorithm.data, "RS256", sizeof("RS256") - 1) == 0 )
 	{
 		// in this case, 'Binary' is a misnomer, as it is the public key string itself
-		keyBinary = jwtcf->auth_jwt_key.data;
-		keylen = jwtcf->auth_jwt_key.len;
+		ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, "failed to find a jwt");


I think you might have left this debug statement in by mistake... this message will get printed even if a JWT is present.

Yup, by mistake while I was testing something. Gonna remove it 👍

fitzyjoe · 2021-08-24T22:09:24Z

src/ngx_http_auth_jwt_module.c

+			}
+
+			// Read file length
+			fseek(file, 0, SEEK_END);


This is going to read the public key from a file for every request that NGINX receives. I'm afraid that this would offer poor performance. I think we would need to read from a file when the configuration is loaded instead.

Until this I've never worked with custom Nginx plugins, it's definitely poor performance as the key won't change frequently (changing the key would require a server restart, which makes sense).

I've tried the init_module and postconfiguration hooks, but it seems they don't load the blocks at that time (since when logging the values from the config it displays the default -1 value). I'll try to put it in the merge_location hook, since it gets called once (maybe a better idea?).

fitzyjoe · 2021-08-24T22:10:32Z

src/ngx_http_auth_jwt_module.c

+			{
+				char err[100];
+				strcpy(err, "failed to open pub key file: ");
+				strcat(err, KEY_FILE_PATH);


This isn't necessarily the value that was specified for the location of the public key... this error outputs the default location of the file.

fitzyjoe · 2021-08-24T22:12:15Z

src/ngx_http_auth_jwt_module.c

+			}
+
+			// Read pub key
+			pub_key = malloc(key_size + 1);


Where is this memory free'd? I think you need to use the ngx_* wrapped versions of malloc to allocate memory.

I saw this mem leak before and changed it on some internal branch, gonna merge it

…t-module into feature/pem-key-file

penumbra23 · 2021-08-25T14:10:57Z

@fitzyjoe I've updated the code and refactored (a bit).
First idea was to have a static keyBin variable to hold the key upon config merging. That was quite okay for one location, but as soon as I added two location with different keyfile, both locations were using the last one loaded...

Now the location config has a (kinda) private _auth_jwt_keyfile field holding the key data and keysize for each specified location block. It's loaded on config merging and reused on each request to improve the performance a bit.

Hope it's okay now, leave more comments because we're using this plugin heavily!

fitzyjoe

Looks good @penumbra23. Thanks for the contribution!

penumbra23 added 9 commits July 24, 2020 12:09

Fix newlines (sry win) in config

a457b98

Add reading key file name, minify docker image size

d81405c

Add key file path, minor name change

c1b9321

Fix memory leak

e7c0728

Fix indentation

ce0b25d

Add RS256 key file documentation

83f839f

Merge pull request #1 from penumbra23/feature/rs256-key-file

6ccbb9f

Add loading RS256 key from file

Fix tests and config

b8f17d1

Fix newlines

be7786e

penumbra23 commented Jul 24, 2020

View reviewed changes

penumbra23 mentioned this pull request Jul 24, 2020

Loading RS256 key from file #55

Closed

fitzyjoe suggested changes Aug 24, 2021

View reviewed changes

branimirbamfan and others added 4 commits August 25, 2021 11:04

Merge branch 'master' of https://github.com/TeslaGov/ngx-http-auth-jw…

f3f3029

…t-module into feature/pem-key-file

Add private keyfile fields inside location config

54a0f05

Add tests for rsa256 keyfile scenarios

3b8bb6f

Update readme

b55c670

penumbra23 requested a review from fitzyjoe August 25, 2021 14:06

fitzyjoe approved these changes Aug 25, 2021

View reviewed changes

fitzyjoe merged commit 1653ef1 into TeslaGov:master Aug 25, 2021

goldyfruit mentioned this pull request Apr 30, 2022

Unable to compile since PR #56 #68

Closed

PEM key file support #56

PEM key file support #56

Uh oh!

Conversation

penumbra23 commented Jul 24, 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!

fitzyjoe left a comment

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!

penumbra23 commented Aug 25, 2021

Uh oh!

fitzyjoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!