fix: drop path normalization to MERGE_SLASHES to allow apps to handle encoded slashes by Racer159 · Pull Request #330 · defenseunicorns/uds-core

Racer159 · 2024-04-05T02:34:23Z

Description

PR to address DECODE_AND_MERGE_SLASHES causing issues in certain applications. Gives the ability to selectively turn this off in a namespace with the UDSPackage CR.

Related Issue

Fixes #288

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Other (security config, docs update, etc)

Checklist before merging

Test, docs, adr added or updated as needed
Contributor Guide Steps(https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md#submitting-a-pull-request) followed

Racer159 · 2024-04-05T03:17:22Z

Post changes with retain slashes set to true:

Pre changes (and without retain slashes):

(Decided to leave path normalization on except for decoding slashes since it shouldn't really hurt anything - I don't know of any apps that expect double slashes or other weird slash configs)

src/pepr/operator/controllers/istio/injection.ts

jeff-mccoy

Holding this one hostage until we discuss further, I really don't think we should do this and would rather remove the Istio Path Normalization than add all this complexity.

Racer159 · 2024-04-07T21:23:22Z

Holding this one hostage until we discuss further, I really don't think we should do this and would rather remove the Istio Path Normalization than add all this complexity.

Yep got no problem with that, was fun learning more about Istio anyway 😅

bburky · 2024-04-08T17:43:12Z

@jeff-mccoy

Holding this one hostage until we discuss further, I really don't think we should do this and would rather remove the Istio Path Normalization than add all this complexity.

Ok.

We'll likely be unable to use path-based AuthorizationPolices with any confidence across all of UDS without careful review of each app individually. I would have liked to have had the ability to use AuthorizationPolices to apply CVE mitigations on UDS apps, see #288 (comment) for more context. In theory we can still do this, but we'll have to review each app's URL decoding implementation before doing so.

For Keycloak specifically, MERGE_SLASHES seems to be sufficient. I did some light manual testing, and //admin is still blocked (redirected) with MERGE_SLASHES, /%2fadmin is now a keycloak error (good, not allowed through). A cursory review of the source code (in vertx via quarkus via keycloak), suggests only alpha, didgit, hyphen, period, underscore and tilde characters are URL decoded:
https://github.com/eclipse-vertx/vert.x/blob/5832acf2fb22ed0301c396d4c3a7c316005e2939/src/main/java/io/vertx/core/http/impl/HttpUtils.java#L317-L329

At least MERGE_SLASHES level of normalization is required if we use path-based AuthorizationPolicies on Keycloak, because Keycloak merges // into / (example: //admin is a valid URL). This is because Keycloak implements this non-standard (but common, as they note) normalization:
https://github.com/eclipse-vertx/vert.x/blob/5832acf2fb22ed0301c396d4c3a7c316005e2939/src/main/java/io/vertx/core/http/impl/HttpUtils.java#L390-L393

mjnagel

LGTM! Thanks for the fix here!

Dismissing this as the target change pivoted from the original envoyfilter approach.

🤖 I have created a release *beep* *boop* --- ## [0.19.0](v0.18.0...v0.19.0) (2024-04-12) ### Features * add nightly testing eks ([#250](#250)) ([543b09d](543b09d)) ### Bug Fixes * drop path normalization to MERGE_SLASHES to allow apps to handle encoded slashes ([#330](#330)) ([26e965f](26e965f)) * loki bucket configuration service_account and namespace ([#332](#332)) ([9518634](9518634)) ### Miscellaneous * **deps:** update grafana ([#257](#257)) ([c98e566](c98e566)) * **deps:** update metrics-server ([#298](#298)) ([691fd87](691fd87)) * **deps:** update pepr ([#324](#324)) ([2ef0f96](2ef0f96)) * **deps:** update pepr to v0.28.7 ([#321](#321)) ([e7206bb](e7206bb)) * **deps:** update promtail ([#74](#74)) ([6a112b5](6a112b5)) * **deps:** update zarf to v0.32.6 ([#282](#282)) ([443426d](443426d)) * **deps:** update zarf to v0.33.0 ([#325](#325)) ([f2a2a66](f2a2a66)) * update codeowners ([#338](#338)) ([c419574](c419574)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

… encoded slashes (#330) ## Description PR to address `DECODE_AND_MERGE_SLASHES` causing issues in certain applications. Gives the ability to selectively turn this off in a namespace with the `UDSPackage` CR. ## Related Issue Fixes #288 ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [X] New feature (non-breaking change which adds functionality) - [ ] Other (security config, docs update, etc) ## Checklist before merging - [x] Test, docs, adr added or updated as needed - [X] [Contributor Guide Steps](https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md)(https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md#submitting-a-pull-request) followed --------- Co-authored-by: Micah Nagel <micah.nagel@defenseunicorns.com>

🤖 I have created a release *beep* *boop* --- ## [0.19.0](v0.18.0...v0.19.0) (2024-04-12) ### Features * add nightly testing eks ([#250](#250)) ([543b09d](543b09d)) ### Bug Fixes * drop path normalization to MERGE_SLASHES to allow apps to handle encoded slashes ([#330](#330)) ([26e965f](26e965f)) * loki bucket configuration service_account and namespace ([#332](#332)) ([9518634](9518634)) ### Miscellaneous * **deps:** update grafana ([#257](#257)) ([c98e566](c98e566)) * **deps:** update metrics-server ([#298](#298)) ([691fd87](691fd87)) * **deps:** update pepr ([#324](#324)) ([2ef0f96](2ef0f96)) * **deps:** update pepr to v0.28.7 ([#321](#321)) ([e7206bb](e7206bb)) * **deps:** update promtail ([#74](#74)) ([6a112b5](6a112b5)) * **deps:** update zarf to v0.32.6 ([#282](#282)) ([443426d](443426d)) * **deps:** update zarf to v0.33.0 ([#325](#325)) ([f2a2a66](f2a2a66)) * update codeowners ([#338](#338)) ([c419574](c419574)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

… encoded slashes (defenseunicorns#330) ## Description PR to address `DECODE_AND_MERGE_SLASHES` causing issues in certain applications. Gives the ability to selectively turn this off in a namespace with the `UDSPackage` CR. ## Related Issue Fixes defenseunicorns#288 ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [X] New feature (non-breaking change which adds functionality) - [ ] Other (security config, docs update, etc) ## Checklist before merging - [x] Test, docs, adr added or updated as needed - [X] [Contributor Guide Steps](https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md)(https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md#submitting-a-pull-request) followed --------- Co-authored-by: Micah Nagel <micah.nagel@defenseunicorns.com>

🤖 I have created a release *beep* *boop* --- ## [0.19.0](defenseunicorns/uds-core@v0.18.0...v0.19.0) (2024-04-12) ### Features * add nightly testing eks ([defenseunicorns#250](defenseunicorns#250)) ([543b09d](defenseunicorns@543b09d)) ### Bug Fixes * drop path normalization to MERGE_SLASHES to allow apps to handle encoded slashes ([defenseunicorns#330](defenseunicorns#330)) ([26e965f](defenseunicorns@26e965f)) * loki bucket configuration service_account and namespace ([defenseunicorns#332](defenseunicorns#332)) ([9518634](defenseunicorns@9518634)) ### Miscellaneous * **deps:** update grafana ([defenseunicorns#257](defenseunicorns#257)) ([c98e566](defenseunicorns@c98e566)) * **deps:** update metrics-server ([defenseunicorns#298](defenseunicorns#298)) ([691fd87](defenseunicorns@691fd87)) * **deps:** update pepr ([defenseunicorns#324](defenseunicorns#324)) ([2ef0f96](defenseunicorns@2ef0f96)) * **deps:** update pepr to v0.28.7 ([defenseunicorns#321](defenseunicorns#321)) ([e7206bb](defenseunicorns@e7206bb)) * **deps:** update promtail ([#74](defenseunicorns#74)) ([6a112b5](defenseunicorns@6a112b5)) * **deps:** update zarf to v0.32.6 ([defenseunicorns#282](defenseunicorns#282)) ([443426d](defenseunicorns@443426d)) * **deps:** update zarf to v0.33.0 ([defenseunicorns#325](defenseunicorns#325)) ([f2a2a66](defenseunicorns@f2a2a66)) * update codeowners ([defenseunicorns#338](defenseunicorns#338)) ([c419574](defenseunicorns@c419574)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

feat: add configurable encoded slash handling behavior to s

93e1e07

Racer159 changed the title ~~feat: add configurable encoded slash handling behavior to s~~ feat: add configurable encoded slash handling behavior to UDSPackages Apr 5, 2024

Racer159 changed the title ~~feat: add configurable encoded slash handling behavior to UDSPackages~~ feat: add configurable encoded slash handling behavior to UDSPackage Apr 5, 2024

Merge branch 'main' into 288-allow-configure-decode-slashes

645013a

Racer159 added 3 commits April 5, 2024 10:27

fix linting for generated files

458ff1c

add simple tests

a867a59

fix lint

40955d4

Racer159 commented Apr 5, 2024

View reviewed changes

src/pepr/operator/controllers/istio/injection.ts Outdated Show resolved Hide resolved

Racer159 and others added 4 commits April 5, 2024 10:59

fix tests

107d39c

fix tests

2f65727

fix lint

63138de

Merge branch 'main' into 288-allow-configure-decode-slashes

2e979fc

Racer159 marked this pull request as ready for review April 5, 2024 17:24

Racer159 requested a review from a team as a code owner April 5, 2024 17:24

Racer159 added 2 commits April 5, 2024 11:44

fix accidental yolo

d752baf

take the keyboard now

5eeae9e

jeff-mccoy previously requested changes Apr 6, 2024

View reviewed changes

Racer159 added 3 commits April 8, 2024 11:56

bust down after feedback

bb5ce2f

fix more

79eebe5

fix more

4109e80

Racer159 changed the title ~~feat: add configurable encoded slash handling behavior to UDSPackage~~ fix: drop path normalization to MERGE_SLASHES to allow apps to handle encoded slashes Apr 8, 2024

Racer159 requested a review from jeff-mccoy April 8, 2024 18:08

mjnagel and others added 2 commits April 10, 2024 14:14

Merge branch 'main' into 288-allow-configure-decode-slashes

487faae

make the logic betta

efaa7ce

mjnagel approved these changes Apr 10, 2024

View reviewed changes

mjnagel merged commit 26e965f into main Apr 11, 2024

mjnagel deleted the 288-allow-configure-decode-slashes branch April 11, 2024 13:56

github-actions bot mentioned this pull request Apr 11, 2024

chore(main): release 0.19.0 #320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: drop path normalization to MERGE_SLASHES to allow apps to handle encoded slashes#330

fix: drop path normalization to MERGE_SLASHES to allow apps to handle encoded slashes#330
mjnagel merged 16 commits intomainfrom
288-allow-configure-decode-slashes

Racer159 commented Apr 5, 2024 •

edited

Loading

Uh oh!

Racer159 commented Apr 5, 2024

Uh oh!

Uh oh!

jeff-mccoy left a comment

Uh oh!

Racer159 commented Apr 7, 2024

Uh oh!

bburky commented Apr 8, 2024 •

edited

Loading

Uh oh!

mjnagel left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Racer159 commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Type of change

Checklist before merging

Uh oh!

Racer159 commented Apr 5, 2024

Uh oh!

Uh oh!

jeff-mccoy left a comment

Choose a reason for hiding this comment

Uh oh!

Racer159 commented Apr 7, 2024

Uh oh!

bburky commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjnagel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Racer159 commented Apr 5, 2024 •

edited

Loading

bburky commented Apr 8, 2024 •

edited

Loading