URI Signing Strips token from upstream if configured and string buffers are dynamically allocated #4895

dsouza93 · 2019-01-30T18:58:33Z

Add a configuration option in the URI signing plugin to strip the signing token from requests made upstream and the URL used as the cache key. The option defaults to false.

Note that this change also changes when the auth_dir gets checked. I've placed that check along with token stripping from the upstream/cache key immediately after a valid JWS is found in URL.
The functionality:
Valid token whitelisted -> Skip JWT validation and strip if configured
Valid token not whitelisted -> Validate JWT and strip if configured
Invalid token whitelisted -> Skip JWT validation and strip if configured
Invalid token not whitelisted -> TS_HTTP_STATUS_FORBIDDEN

Additionally, it was pointed out in the PR that some statically declared string buffers in normalizing and stripping functions are not buffer-safe. These buffers are now dynamically allocated.

scw00 · 2019-01-31T02:35:44Z

@alficles Can you please take a loop when have the time . [approve ci]

dsouza93 · 2019-01-31T16:43:20Z

Can we get a WIP slapped on here? I'd like to change this to strip tokens even if the path is whitelisted. (That is not currently the case)

plugins/experimental/uri_signing/uri_signing.c

alficles · 2019-01-31T21:30:29Z

plugins/experimental/uri_signing/uri_signing.c

+    /* There has been a JWS found in the url */
+    /* Strip the token from the URL for upstream if configured to do so */
+    if (config_strip_token((struct config *)ih)) {
+      if (strncmp(url, strip_uri, strip_ct) != 0) {


Could this be simplified to checking strip_ct != url_ct? I don't think it's possible to remove a token without changing the length and that would be a much more efficient check.

Yep good point. Changed it

plugins/experimental/uri_signing/uri_signing.c

alficles · 2019-01-31T21:34:28Z

plugins/experimental/uri_signing/uri_signing.c

+      if (url != NULL) {
+        free(strip_uri);
+      }
+      return TSREMAP_NO_REMAP;


Should this return status, in case a token was stripped?

If I modify the URL using TSUrlParse and return TSREMAP_NO_REMAP I am seeing the expected behavior of the token being stripped and the stripped URL reaching the origin. This doesn't seem to be the correct usage though.

If I modify the URL using TSUrlParse and then return a TSREMAP_DID_REMAP as I believe I should, I am getting 405s and nothing hits the origin. I am trying to track down what is going on.

I believe the plugin is setting the working URL to the stripped pristine URL and then returning a TSREMAP_DID_REMAP is preventing the core from remapping to whatever is in remap.config. We are then trying to send the pristine URL upstream and failing.

Leaving as TSREMAP_NO_REMAP will work for what I've tested thus far, but who knows how that will behave with other configurations/plugins.

I think you're right and we should be parsing and stripping from the remapped URL rather than pristine (however we can get that).

#4026
The above issue might complicate things if uri_signing is to be the first plugin.

A discussion is currently being had on the dev mailing list regarding if the core should remap before or after the plugins. If the core runs before the plugins, I would be able to assume that the requestUrl has been remapped, and return TS_DID_REMAP here with the code as is.

If the plugins run before the core, it would be very difficult to get hands on the remapped request with query strings and paths in a remap plugin. I think I would have to piece together the URL I'd like to set from a mix of remap config values and the path/query strings from the existing url. I would then be able to return TS_DID_REMAP. This second way seems cumbersome and like something could easily be forgotten or left out when piecing together the remapped URL. Additionally, we'd be moving integral processing usually handled by the core into the plugin.

I am going to wait a bit to see how that pans out.

randall · 2019-02-07T01:22:26Z

[approve ci]

randall · 2019-03-12T20:57:55Z

[approve ci]

…ing buffers Adds a configuration option to strip uri signing tokens from both the cache key URL and the upstream URL. Additionally it was pointed out that some statically allocated buffers were too small in some of the string manipulating functions (normalize and strip token). These buffers are now dynamically allocated since the maximum buffer size is known for these.

dsouza93 · 2019-04-12T16:25:06Z

I believe the plugin ordering issue has been resolved and this is ready to merge pending approval

ezelkow1 · 2019-04-12T16:26:23Z

[approve ci]

List of included PRs: - apache#6363 (partial pick) - apache#6420 - apache#6419 - apache#6354 - apache#6252 - apache#4513 - apache#4603 - apache#4750 (partial pick) - apache#4604 - apache#4540 - apache#4777 - apache#4862 - apache#4814 - apache#4802 - apache#4897 - apache#4988 - apache#5034 - apache#5140 - apache#5112 - apache#4895 - apache#5834 (partial pick) - apache#6061 - apache#6210 (partial pick) - apache#6265 (partial pick) - apache#6282 (partial pick) Updating uri_signing docs to reflect new RFC changes (cherry picked from commit 90e51a2) Add normalization the URI before cdniuc validation (cherry picked from commit b39b0f7) JWT Parser strips token from URI and places in buffer (cherry picked from commit 5f9d358) Use POSIX ERE for uri signing regex evaluation (cherry picked from commit be56b3a) Implement nbf claim in Uri Signing Plugin (cherry picked from commit d9dc0f4) Implement aud claim in Uri Signing Plugin The Aud claim is implemented as per the RFC version 16 that can be found here:https://tools.ietf.org/html/draft-ietf-cdni-uri-signing-16 As per the specification, the aud claim can be either a JSON array or a string. The aud claim is stored as raw json in the jwt class in this implementation. It is converted either to an array or a string at validation time. This commit also expands the unit tests quite a bit. Test configs can be provided in the unit_tests directory and parsed in the test framework. JWS validation is also testable now. This commit also fixes two memory leaks 1. Issuers were never being freed on configuration cleanup. 2. Token renewal allocates a tmp json_object without freeing. (cherry picked from commit 012d437) cdniuc is not a manditory claim With Internet Draft 16 for uri signing, the cdniuc claim is not manditory. It took the place of the manditory sub claim in draft 12, and the manditory nature of the sub claim was still in effect. This change allows for tokens to not contain the cdniuc claim and also renews the cdniuc and cdnistd claim on token renewal. (cherry picked from commit fa53771) add --with-jansson and --with-cjose options, document sample commands for building and configuring both locally (cherry picked from commit 0cce83c) Strip token from upstream if conifigured and dynamically allocate string buffers Adds a configuration option to strip uri signing tokens from both the cache key URL and the upstream URL. Additionally it was pointed out that some statically allocated buffers were too small in some of the string manipulating functions (normalize and strip token). These buffers are now dynamically allocated since the maximum buffer size is known for these. (cherry picked from commit 192dc83) Cherry-pick from commit 4cfd5a7 Add Example URI Signer Python Script Provide an example script to be used in conjunction with the uri signing plugin. This script is meant to serve as an example of how to get started with uri signing and could be useful in testing various configs. (cherry picked from commit 3632eb7) Cherry-pick from commit 9c1b88a Cherry-pick from commit a139fd1 Cherry-pick from commit c07474d Add simple autest and subsequent fixes (cherry picked from commit ea3aa04) Cherry-pick from commit 6d64842 URI Sig Null Check for Clang Warning (apache#6419) This commit adds a missing null check in the uri normalization function. This was caught by the clang analyzer. (cherry picked from commit 2de1c35) Syntax Error fixed in URI sig Plugin (apache#6420) (cherry picked from commit c154d40) Change gold files to be less restrictive since some of the headers include can be in a different order (apache#6410) (cherry picked from commit 4bdde5d) Add a dummy cachekey usage to handle the effective vs pristine url issue that exists in 8x where the first plugin gets a different url then subsequent ones. This is not needed on 9x+

List of included PRs: - #6363 (partial pick) - #6420 - #6419 - #6354 - #6252 - #4513 - #4603 - #4750 (partial pick) - #4604 - #4540 - #4777 - #4862 - #4814 - #4802 - #4897 - #4988 - #5034 - #5140 - #5112 - #4895 - #5834 (partial pick) - #6061 - #6210 (partial pick) - #6265 (partial pick) - #6282 (partial pick) Updating uri_signing docs to reflect new RFC changes (cherry picked from commit 90e51a2) Add normalization the URI before cdniuc validation (cherry picked from commit b39b0f7) JWT Parser strips token from URI and places in buffer (cherry picked from commit 5f9d358) Use POSIX ERE for uri signing regex evaluation (cherry picked from commit be56b3a) Implement nbf claim in Uri Signing Plugin (cherry picked from commit d9dc0f4) Implement aud claim in Uri Signing Plugin The Aud claim is implemented as per the RFC version 16 that can be found here:https://tools.ietf.org/html/draft-ietf-cdni-uri-signing-16 As per the specification, the aud claim can be either a JSON array or a string. The aud claim is stored as raw json in the jwt class in this implementation. It is converted either to an array or a string at validation time. This commit also expands the unit tests quite a bit. Test configs can be provided in the unit_tests directory and parsed in the test framework. JWS validation is also testable now. This commit also fixes two memory leaks 1. Issuers were never being freed on configuration cleanup. 2. Token renewal allocates a tmp json_object without freeing. (cherry picked from commit 012d437) cdniuc is not a manditory claim With Internet Draft 16 for uri signing, the cdniuc claim is not manditory. It took the place of the manditory sub claim in draft 12, and the manditory nature of the sub claim was still in effect. This change allows for tokens to not contain the cdniuc claim and also renews the cdniuc and cdnistd claim on token renewal. (cherry picked from commit fa53771) add --with-jansson and --with-cjose options, document sample commands for building and configuring both locally (cherry picked from commit 0cce83c) Strip token from upstream if conifigured and dynamically allocate string buffers Adds a configuration option to strip uri signing tokens from both the cache key URL and the upstream URL. Additionally it was pointed out that some statically allocated buffers were too small in some of the string manipulating functions (normalize and strip token). These buffers are now dynamically allocated since the maximum buffer size is known for these. (cherry picked from commit 192dc83) Cherry-pick from commit 4cfd5a7 Add Example URI Signer Python Script Provide an example script to be used in conjunction with the uri signing plugin. This script is meant to serve as an example of how to get started with uri signing and could be useful in testing various configs. (cherry picked from commit 3632eb7) Cherry-pick from commit 9c1b88a Cherry-pick from commit a139fd1 Cherry-pick from commit c07474d Add simple autest and subsequent fixes (cherry picked from commit ea3aa04) Cherry-pick from commit 6d64842 URI Sig Null Check for Clang Warning (#6419) This commit adds a missing null check in the uri normalization function. This was caught by the clang analyzer. (cherry picked from commit 2de1c35) Syntax Error fixed in URI sig Plugin (#6420) (cherry picked from commit c154d40) Change gold files to be less restrictive since some of the headers include can be in a different order (#6410) (cherry picked from commit 4bdde5d) Add a dummy cachekey usage to handle the effective vs pristine url issue that exists in 8x where the first plugin gets a different url then subsequent ones. This is not needed on 9x+

dsouza93 changed the title ~~Strips token from upstream if configured~~ URI Signing Strips token from upstream if configured Jan 30, 2019

scw00 assigned dsouza93 Jan 31, 2019

scw00 added the HTTP label Jan 31, 2019

scw00 added this to the 9.0.0 milestone Jan 31, 2019

scw00 added the Plugins label Jan 31, 2019

dsouza93 force-pushed the uri_signing_strip_config branch from a6dce7a to a1a960f Compare January 31, 2019 15:58

alficles suggested changes Jan 31, 2019

View reviewed changes

plugins/experimental/uri_signing/uri_signing.c Show resolved Hide resolved

SolidWallOfCode added the WIP label Jan 31, 2019

dsouza93 force-pushed the uri_signing_strip_config branch from 074a95c to fa2deee Compare January 31, 2019 19:29

dsouza93 changed the title ~~URI Signing Strips token from upstream if configured~~ URI Signing Strips token from upstream if configured and string buffers are dynamically allocated Jan 31, 2019

alficles suggested changes Jan 31, 2019

View reviewed changes

dsouza93 force-pushed the uri_signing_strip_config branch 2 times, most recently from 6efacb6 to 945daae Compare February 4, 2019 23:23

dsouza93 force-pushed the uri_signing_strip_config branch from b68a08d to 82aafbe Compare February 21, 2019 22:22

dsouza93 force-pushed the uri_signing_strip_config branch from 82aafbe to c6ca45f Compare March 8, 2019 15:43

dsouza93 force-pushed the uri_signing_strip_config branch from c6ca45f to 81ce213 Compare April 12, 2019 16:20

dsouza93 force-pushed the uri_signing_strip_config branch from fab992b to 6a2a3c1 Compare April 12, 2019 16:22

ezelkow1 approved these changes Apr 15, 2019

View reviewed changes

ezelkow1 merged commit 192dc83 into apache:master Apr 15, 2019

dsouza93 deleted the uri_signing_strip_config branch April 15, 2019 15:35

ezelkow1 mentioned this pull request Apr 6, 2020

Backport of all uri signing commits from master to 8.1.x #6584

Merged

URI Signing Strips token from upstream if configured and string buffers are dynamically allocated #4895

URI Signing Strips token from upstream if configured and string buffers are dynamically allocated #4895

Uh oh!

Conversation

dsouza93 commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scw00 commented Jan 31, 2019

Uh oh!

dsouza93 commented Jan 31, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alficles Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

dsouza93 Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alficles Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

dsouza93 Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

dsouza93 Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

dsouza93 Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

randall commented Feb 7, 2019

Uh oh!

randall commented Mar 12, 2019

Uh oh!

dsouza93 commented Apr 12, 2019

Uh oh!

ezelkow1 commented Apr 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dsouza93 commented Jan 30, 2019 •

edited

Loading