diff --git a/README.md b/README.md index 95c83af..f6b09d6 100644 --- a/README.md +++ b/README.md @@ -192,41 +192,43 @@ Once you save your changes to `.vscode/c_cpp_properties.json`, you should see th ### Building and Testing -The `./scripts.sh` file contains multiple commands to make things easy: +The `./scripts` file contains multiple commands to make things easy: | Command | Description | | --------------------- | ----------------------------------------------------------------- | | `build_module` | Builds the NGINX image. | | `rebuild_module` | Re-builds the NGINX image. | -| `start_nginx` | Starts the NGINX container. | -| `stop_nginx` | Stops the NGINX container. | +| `start` | Starts the NGINX container. | +| `stop` | Stops the NGINX container. | | `cp_bin` | Copies the compiled binaries out of the NGINX container. | -| `build_test_runner` | Builds the images used by the test stack (uses Docker compose). | -| `rebuild_test_runner` | Re-builds the images used by the test stack. | -| `test` | Runs `test.sh` against the NGINX container (uses Docker compose). | +| `build_test` | Builds the images used by the test stack. | +| `rebuild_test` | Re-builds the images used by the test stack. | +| `test` | Runs `test.sh` against the NGINX container. | | `test_now` | Runs `test.sh` without rebuilding. | You can run multiple commands in sequence by separating them with a space, e.g.: ```shell -./scripts.sh build_module test +./scripts build_module +./scripts test ``` -To build the Docker images, module, start NGINX, and run the tests against, you can simply do: +To build the Docker images, module, start NGINX, and run the tests against it for all versions, you can simply do: ```shell -./scripts.sh all +./scripts all ``` -When you make a change to the module run `./scripts.sh build_module test` to build a fresh module and run the tests. Note that `rebuild_module` is not often needed as `build_module` hashes the module's source files which will cause a cache miss while building the container, causing the module to be rebuilt. +When you make a change to the module, running `./scripts test` should build a fresh module and run the tests. Note that `rebuild_module` is not often needed as Docker will automatically rebuild the image if the source files have +changed. -When you make a change to the test NGINX config or `test.sh`, run `./scripts.sh test` to run the tests. Similar to above, the test sources are hashed and the containers will be rebuilt as needed. +When you make a change to the test NGINX config or `test.sh`, run `./scripts test` to run the tests. -The image produced with `./scripts.sh build_module` only differs from the official NGINX image in two ways: +The image produced with `./scripts build_module` only differs from the official NGINX image in two ways: - the JWT module itself, and - the `nginx.conf` file is overwritten with our own. -The tests use a customized NGINX image, distinct from the main image, as well as a test runner image. By running `./scripts.sh test`, the two test containers will be stood up via Docker compose, then they'll be started, and the tests will run. At the end of the test run, both containers will be automatically stopped and destroyed. See below to learn how to trace test failures across runs. +The tests use a customized NGINX image, distinct from the main image, as well as a test runner image. By running `./scripts test`, the two test containers will be stood up via Docker Compose, then they'll be started, and the tests will run. At the end of the test run, both containers will be automatically stopped and destroyed. See below to learn how to trace test failures across runs. #### Tracing Test Failures @@ -236,20 +238,23 @@ If you'd like to persist logs across test runs, you can configure the log driver ```shell # need to rebuild the test runner with the proper log driver -LOG_DRIVER=journald ./scripts.sh rebuild_test_runner +export LOG_DRIVER=journald + +# rebuild the test images +./scripts rebuild_test # run the tests -./scripts.sh test +./scripts test -# check the logs -journalctl -eu docker CONTAINER_NAME=jwt-nginx-test +# check the logs -- adjust the container name as needed +journalctl -eu docker CONTAINER_NAME=nginx-auth-jwt-test-nginx ``` Now you'll be able to see logs from previous test runs. The best way to make use of this is to open two terminals, one where you run the tests, and one where you follow the logs: ```shell # terminal 1 -./scripts.sh test +./scripts test # terminal 2 journalctl -fu docker CONTAINER_NAME=jwt-nginx-test diff --git a/scripts b/scripts index 9d90185..59fc5c3 100755 --- a/scripts +++ b/scripts @@ -31,6 +31,7 @@ IMAGE_NAME=${IMAGE_NAME:-nginx-auth-jwt} FULL_IMAGE_NAME=${ORG_NAME:-teslagov}/${IMAGE_NAME} TEST_CONTAINER_NAME_PREFIX="${IMAGE_NAME}-test" +TEST_COMPOSE_FILE='test/docker-compose-test.yml' all() { build_module @@ -162,7 +163,8 @@ build_test() { printf "${MAGENTA}Building test NGINX & runner using port ${port}...${NC}\n" docker compose \ -p ${TEST_CONTAINER_NAME_PREFIX} \ - -f ./test/docker-compose-test.yml build \ + -f ${TEST_COMPOSE_FILE} \ + build \ --build-arg RUNNER_BASE_IMAGE=${runnerBaseImage} \ --build-arg PORT=${port} \ --build-arg SSL_PORT=${sslPort} \ @@ -188,12 +190,11 @@ test() { printf "${MAGENTA}Running tests...${NC}\n" docker compose \ -p ${TEST_CONTAINER_NAME_PREFIX} \ - -f ./test/docker-compose-test.yml up \ + -f ${TEST_COMPOSE_FILE} up \ --no-start - - trap 'docker compose -f ./test/docker-compose-test.yml down' 0 - + trap test_cleanup 0 + test_now } @@ -218,6 +219,12 @@ test_now() { echo " NGINX Version: ${NGINX_VERSION}" } +test_cleanup() { + docker compose \ + -p ${TEST_CONTAINER_NAME_PREFIX} \ + -f ${TEST_COMPOSE_FILE} down +} + get_port() { startPort=${1:-8000} endPort=$((startPort + 100)) diff --git a/src/ngx_http_auth_jwt_module.c b/src/ngx_http_auth_jwt_module.c index fe428b4..59b84ac 100644 --- a/src/ngx_http_auth_jwt_module.c +++ b/src/ngx_http_auth_jwt_module.c @@ -290,13 +290,13 @@ static char *merge_extract_var_claims(ngx_conf_t *cf, ngx_command_t *cmd, void * { // add this claim's name to the config struct ngx_str_t *element = ngx_array_push(claims); - + *element = values[i]; // add an http variable for this claim size_t var_name_len = 10 + element->len; u_char *buf = ngx_palloc(cf->pool, sizeof(u_char) * var_name_len); - + if (buf == NULL) { return NGX_CONF_ERROR; @@ -305,7 +305,7 @@ static char *merge_extract_var_claims(ngx_conf_t *cf, ngx_command_t *cmd, void * { ngx_sprintf(buf, "jwt_claim_%V", element); ngx_str_t *var_name = ngx_palloc(cf->pool, sizeof(ngx_str_t)); - + if (var_name == NULL) { return NGX_CONF_ERROR; @@ -314,31 +314,31 @@ static char *merge_extract_var_claims(ngx_conf_t *cf, ngx_command_t *cmd, void * { var_name->data = buf; var_name->len = var_name_len; - + // NGX_HTTP_VAR_CHANGEABLE simplifies the required logic by assuming a JWT claim will always be the same for a given request ngx_http_variable_t *http_var = ngx_http_add_variable(cf, var_name, NGX_HTTP_VAR_CHANGEABLE); - + if (http_var == NULL) { ngx_log_error(NGX_LOG_ERR, cf->log, 0, "failed to add variable %V", var_name); - + return NGX_CONF_ERROR; } else { http_var->get_handler = get_jwt_var_claim; - + // store the index of this new claim in the claims array as the "data" that will be passed to the getter ngx_uint_t *claim_idx = ngx_palloc(cf->pool, sizeof(ngx_uint_t)); - + if (claim_idx == NULL) { - return NGX_CONF_ERROR; + return NGX_CONF_ERROR; } else { *claim_idx = claims->nelts - 1; - http_var->data = (uintptr_t) claim_idx; + http_var->data = (uintptr_t)claim_idx; } } } @@ -350,26 +350,26 @@ static char *merge_extract_var_claims(ngx_conf_t *cf, ngx_command_t *cmd, void * static ngx_int_t get_jwt_var_claim(ngx_http_request_t *r, ngx_http_variable_value_t *v, uintptr_t data) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "getting jwt value for var index %l", *((ngx_uint_t*) data)); + ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "getting jwt value for var index %l", *((ngx_uint_t *)data)); auth_jwt_ctx_t *ctx = get_request_jwt_ctx(r); - + if (ctx == NULL) { ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "no module context found while getting jwt value"); - + return NGX_ERROR; } else { - ngx_uint_t *claim_idx = (ngx_uint_t*) data; - ngx_str_t claim_value = ((ngx_str_t*) ctx->claim_values->elts)[*claim_idx]; - + ngx_uint_t *claim_idx = (ngx_uint_t *)data; + ngx_str_t claim_value = ((ngx_str_t *)ctx->claim_values->elts)[*claim_idx]; + v->valid = 1; v->no_cacheable = 0; v->not_found = 0; v->len = claim_value.len; v->data = claim_value.data; - + return NGX_OK; } } @@ -420,7 +420,7 @@ static char *merge_extract_response_claims(ngx_conf_t *cf, ngx_command_t *cmd, v static auth_jwt_ctx_t *get_or_init_jwt_module_ctx(ngx_http_request_t *r, auth_jwt_conf_t *jwtcf) { auth_jwt_ctx_t *ctx = ngx_http_get_module_ctx(r, ngx_http_auth_jwt_module); - + if (ctx != NULL) { return ctx; @@ -434,7 +434,8 @@ static auth_jwt_ctx_t *get_or_init_jwt_module_ctx(ngx_http_request_t *r, auth_jw ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, "error allocating jwt module context"); return ctx; } - else { + else + { if (jwtcf->extract_var_claims != NULL) { ctx->claim_values = ngx_array_create(r->pool, jwtcf->extract_var_claims->nelts, sizeof(ngx_str_t)); @@ -460,7 +461,7 @@ static auth_jwt_ctx_t *get_request_jwt_ctx(ngx_http_request_t *r) { auth_jwt_conf_t *jwtcf = ngx_http_get_module_loc_conf(r, ngx_http_auth_jwt_module); - if(!jwtcf->enabled) + if (!jwtcf->enabled) { return NULL; } @@ -587,7 +588,8 @@ static int validate_alg(auth_jwt_conf_t *jwtcf, jwt_t *jwt) { const jwt_alg_t alg = jwt_get_alg(jwt); - if (alg != JWT_ALG_HS256 && alg != JWT_ALG_HS384 && alg != JWT_ALG_HS512 && alg != JWT_ALG_RS256 && alg != JWT_ALG_RS384 && alg != JWT_ALG_RS512 && alg != JWT_ALG_ES256 && alg != JWT_ALG_ES384 && alg != JWT_ALG_ES512) { + if (alg != JWT_ALG_HS256 && alg != JWT_ALG_HS384 && alg != JWT_ALG_HS512 && alg != JWT_ALG_RS256 && alg != JWT_ALG_RS384 && alg != JWT_ALG_RS512 && alg != JWT_ALG_ES256 && alg != JWT_ALG_ES384 && alg != JWT_ALG_ES512) + { return 1; } @@ -625,7 +627,7 @@ static int validate_sub(auth_jwt_conf_t *jwtcf, jwt_t *jwt) static ngx_int_t extract_var_claims(ngx_http_request_t *r, auth_jwt_conf_t *jwtcf, jwt_t *jwt, auth_jwt_ctx_t *ctx) { ngx_array_t *claims = jwtcf->extract_var_claims; - + if (claims == NULL || claims->nelts == 0) { return NGX_OK; @@ -633,22 +635,22 @@ static ngx_int_t extract_var_claims(ngx_http_request_t *r, auth_jwt_conf_t *jwtc else { const ngx_str_t *claimsPtr = claims->elts; - + for (uint i = 0; i < claims->nelts; ++i) { const ngx_str_t claim = claimsPtr[i]; const char *claimValue = jwt_get_grant(jwt, (char *)claim.data); ngx_str_t value = ngx_string(""); - + if (claimValue != NULL && strlen(claimValue) > 0) { value = char_ptr_to_ngx_str_t(r->pool, claimValue); } - - ((ngx_str_t*) ctx->claim_values->elts)[i] = value; + + ((ngx_str_t *)ctx->claim_values->elts)[i] = value; ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "set var %V to JWT claim value %s", &claim, value.data); } - + return NGX_OK; } } @@ -708,11 +710,16 @@ static ngx_int_t redirect(ngx_http_request_t *r, auth_jwt_conf_t *jwtcf) if (r->method == NGX_HTTP_GET) { const int loginlen = jwtcf->loginurl.len; - const char *scheme = (r->connection->ssl) ? "https" : "http"; + const char *scheme = r->connection->ssl ? "https" : "http"; + ngx_str_t port_variable_name = ngx_string("server_port"); + ngx_int_t port_variable_hash = ngx_hash_key(port_variable_name.data, port_variable_name.len); + ngx_http_variable_value_t *port_var = ngx_http_get_variable(r, &port_variable_name, port_variable_hash); + char *port_str = ""; + uint port_str_len = 0; const ngx_str_t server = r->headers_in.server; ngx_str_t uri_variable_name = ngx_string("request_uri"); ngx_int_t uri_variable_hash = ngx_hash_key(uri_variable_name.data, uri_variable_name.len); - ngx_http_variable_value_t *request_uri_var = ngx_http_get_variable(r, &uri_variable_name, uri_variable_hash); + ngx_http_variable_value_t *uri_var = ngx_http_get_variable(r, &uri_variable_name, uri_variable_hash); ngx_str_t uri; ngx_str_t uri_escaped; uintptr_t escaped_len; @@ -720,12 +727,12 @@ static ngx_int_t redirect(ngx_http_request_t *r, auth_jwt_conf_t *jwtcf) int return_url_idx; // get the URI - if (request_uri_var && !request_uri_var->not_found && request_uri_var->valid) + if (uri_var && !uri_var->not_found && uri_var->valid) { // ideally we would like the URI with the querystring parameters - uri.data = ngx_palloc(r->pool, request_uri_var->len); - uri.len = request_uri_var->len; - ngx_memcpy(uri.data, request_uri_var->data, request_uri_var->len); + uri.data = ngx_palloc(r->pool, uri_var->len); + uri.len = uri_var->len; + ngx_memcpy(uri.data, uri_var->data, uri_var->len); } else { @@ -733,31 +740,59 @@ static ngx_int_t redirect(ngx_http_request_t *r, auth_jwt_conf_t *jwtcf) uri = r->uri; } + if (port_var && !port_var->not_found && port_var->valid) + { + const ngx_uint_t port_num = ngx_atoi(port_var->data, port_var->len); + const bool is_default_port_80 = !r->connection->ssl && port_num == 80; + const bool is_default_port_443 = r->connection->ssl && port_num == 443; + const bool is_non_default_port = !is_default_port_80 && !is_default_port_443; + + if (is_non_default_port) + { + port_str = ngx_palloc(r->pool, NGX_INT_T_LEN + 2); + + ngx_snprintf((u_char *)port_str, sizeof(port_str), ":%d", port_num); + port_str_len = strlen(port_str); + } + } + + // escape the URI escaped_len = 2 * ngx_escape_uri(NULL, uri.data, uri.len, NGX_ESCAPE_ARGS) + uri.len; uri_escaped.data = ngx_palloc(r->pool, escaped_len); uri_escaped.len = escaped_len; ngx_escape_uri(uri_escaped.data, uri.data, uri.len, NGX_ESCAPE_ARGS); - r->headers_out.location->value.len = loginlen + strlen("?return_url=") + strlen(scheme) + strlen("://") + server.len + uri_escaped.len; + // Add up the lengths of: login URL, "?return_url=", scheme, "://", server, port, uri (path) + r->headers_out.location->value.len = loginlen + 12 + strlen(scheme) + 3 + server.len + port_str_len + uri_escaped.len; return_url = ngx_palloc(r->pool, r->headers_out.location->value.len); - ngx_memcpy(return_url, jwtcf->loginurl.data, jwtcf->loginurl.len); + ngx_memcpy(return_url, jwtcf->loginurl.data, jwtcf->loginurl.len); return_url_idx = jwtcf->loginurl.len; - ngx_memcpy(return_url + return_url_idx, "?return_url=", strlen("?return_url=")); - return_url_idx += strlen("?return_url="); - ngx_memcpy(return_url + return_url_idx, scheme, strlen(scheme)); + ngx_memcpy(return_url + return_url_idx, "?return_url=", 12); + return_url_idx += 12; + ngx_memcpy(return_url + return_url_idx, scheme, strlen(scheme)); return_url_idx += strlen(scheme); - ngx_memcpy(return_url + return_url_idx, "://", strlen("://")); - return_url_idx += strlen("://"); - ngx_memcpy(return_url + return_url_idx, server.data, server.len); + ngx_memcpy(return_url + return_url_idx, "://", 3); + return_url_idx += 3; + ngx_memcpy(return_url + return_url_idx, server.data, server.len); return_url_idx += server.len; - ngx_memcpy(return_url + return_url_idx, uri_escaped.data, uri_escaped.len); + + if (port_str_len > 0) + { + ngx_memcpy(return_url + return_url_idx, port_str, port_str_len); + return_url_idx += port_str_len; + } + + if (uri_escaped.len > 0) + { + ngx_memcpy(return_url + return_url_idx, uri_escaped.data, uri_escaped.len); + } r->headers_out.location->value.data = (u_char *)return_url; } diff --git a/test/docker-compose-test.yml b/test/docker-compose-test.yml index 14c88da..72ff710 100644 --- a/test/docker-compose-test.yml +++ b/test/docker-compose-test.yml @@ -1,20 +1,19 @@ services: nginx: - container_name: ${TEST_CONTAINER_NAME_PREFIX}-nginx + container_name: ${TEST_CONTAINER_NAME_PREFIX:?required}-nginx build: context: . dockerfile: test-nginx.dockerfile args: - BASE_IMAGE: ${FULL_IMAGE_NAME}:${NGINX_VERSION} + BASE_IMAGE: ${FULL_IMAGE_NAME}:${NGINX_VERSION:?required} logging: driver: ${LOG_DRIVER:-journald} runner: - container_name: ${TEST_CONTAINER_NAME_PREFIX}-runner + container_name: ${TEST_CONTAINER_NAME_PREFIX:?required}-runner build: context: . dockerfile: test-runner.dockerfile - depends_on: - nginx \ No newline at end of file diff --git a/test/etc/nginx/conf.d/test.conf b/test/etc/nginx/conf.d/test.conf index 5359434..4e5d764 100644 --- a/test/etc/nginx/conf.d/test.conf +++ b/test/etc/nginx/conf.d/test.conf @@ -405,6 +405,7 @@ vXjq39xtcIBRTO1c2zs= if ($jwt_claim_sub = 'some-long-uuid') { return 200; } + return 401; } @@ -439,4 +440,9 @@ vXjq39xtcIBRTO1c2zs= return 301 /profile/$jwt_claim_sub; } } + + location /return-url { + auth_jwt_enabled on; + auth_jwt_redirect on; + } } diff --git a/test/test-nginx.dockerfile b/test/test-nginx.dockerfile index e12acb4..1065558 100644 --- a/test/test-nginx.dockerfile +++ b/test/test-nginx.dockerfile @@ -1,13 +1,11 @@ ARG BASE_IMAGE -ARG PORT -ARG SSL_PORT -FROM ${BASE_IMAGE} AS NGINX +FROM ${BASE_IMAGE:?required} AS NGINX ARG PORT ARG SSL_PORT + COPY etc/ /etc/ -RUN sed -i "s|%{PORT}|${PORT}|" /etc/nginx/conf.d/test.conf -RUN sed -i "s|%{SSL_PORT}|${SSL_PORT}|" /etc/nginx/conf.d/test.conf + COPY <<` /usr/share/nginx/html/index.html Test @@ -16,3 +14,6 @@ COPY <<` /usr/share/nginx/html/index.html ` + +RUN sed -i "s|%{PORT}|${PORT:?required}|" /etc/nginx/conf.d/test.conf +RUN sed -i "s|%{SSL_PORT}|${SSL_PORT:?required}|" /etc/nginx/conf.d/test.conf diff --git a/test/test-runner.dockerfile b/test/test-runner.dockerfile index 0aca095..18fc3d3 100644 --- a/test/test-runner.dockerfile +++ b/test/test-runner.dockerfile @@ -1,16 +1,18 @@ ARG RUNNER_BASE_IMAGE -ARG PORT -ARG SSL_PORT -FROM ${RUNNER_BASE_IMAGE} +FROM ${RUNNER_BASE_IMAGE:?required} ARG PORT ARG SSL_PORT -ENV PORT=${PORT} -ENV SSL_PORT=${SSL_PORT} + +ENV PORT=${PORT:?required} +ENV SSL_PORT=${SSL_PORT:?required} + RUN <<` set -e apt-get update apt-get install -y curl bash ` + COPY test.sh . -CMD ./test.sh ${PORT} ${SSL_PORT} + +CMD ./test.sh diff --git a/test/test.sh b/test/test.sh index 747124c..c726a75 100755 --- a/test/test.sh +++ b/test/test.sh @@ -29,7 +29,7 @@ run_test () { local response= local testNum="${GRAY}${NUM_TESTS}${NC}\t" - while getopts "n:sp:r:c:x:" option; do + while getopts "n:asp:r:c:x:" option; do case $option in n) name=$OPTARG;; @@ -73,7 +73,7 @@ run_test () { fi if [ "${okay}" == '1' ] && [ "${expectedResponseRegex}" != "" ] && ! [[ "${response}" =~ ${expectedResponseRegex} ]]; then - printf "${RED}${name} -- regex not found in response\n\tPath: ${path}\n\tRegEx: ${expectedResponseRegex}" + printf "${RED}${name} -- regex not found in response\n\tPath: ${path}\n\tRegEx: ${expectedResponseRegex//%/%%}" NUM_FAILED=$((${NUM_FAILED} + 1)) okay=0 fi @@ -113,8 +113,8 @@ main() { -p '/' \ -c '200' - run_test -s \ - -n '[SSL] when auth disabled, should return 200' \ + run_test -n '[SSL] when auth disabled, should return 200' \ + -s \ -p '/' \ -c '200' @@ -350,6 +350,26 @@ main() { -r '< Location: http://nginx:8000/profile/some-long-uuid' \ -x '--header "Authorization: Bearer ${JWT_HS256_VALID}"' + run_test -n 'returns 302 if auth enabled and no JWT provided' \ + -p '/return-url' \ + -c '302' + + run_test -n 'redirects to login if auth enabled and no JWT provided' \ + -p '/return-url' \ + -r '< Location: https://example\.com/login.*' + + run_test -n 'adds return_url to login URL when redirected to login' \ + -p '/return-url' \ + -r '< Location: https://example\.com/login\?return_url=http://nginx.*' + + run_test -n 'return_url includes port when redirected to login' \ + -p '/return-url' \ + -r "< Location: https://example\.com/login\?return_url=http://nginx:${PORT}/return-url" + + run_test -n 'return_url includes query when redirected to login' \ + -p '/return-url?test=123' \ + -r '< Location: https://example\.com/login\?return_url=http://nginx.*/return-url%3Ftest=123' + if [[ "${NUM_FAILED}" = '0' ]]; then printf "\nRan ${NUM_TESTS} tests successfully (skipped ${NUM_SKIPPED}).\n" return 0