Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[maistra-1.1] MAISTRA-224: Support for authorisation regex matching on request headers #65

Merged
merged 2 commits into from
Jan 22, 2020

Conversation

dgn
Copy link
Contributor

@dgn dgn commented Dec 13, 2019

This adds back the regex matching on request headers functionality we introduced in #3. I changed it to use the SafeRegex matchers introduced in newer Envoy versions to mitigate the Regex matching CVE.

Upstream, there's a feature flag PILOT_ENABLE_UNSAFE_REGEX=false that by default sets SafeRegex as the default but provides a fall-back for users who cannot change their regular expressions to be compatible with RE2. While this is a sensible migration strategy, I think we should not do the same, for multiple reasons:

  1. Setting the feature flag would expose our users to the CVE. Upstream changed the std library so they can safely use libstdc++::regex, we however are stuck with std::regex.
  2. Envoy has deprecated the use of the old regex field, so migration has to happen at some point anyway.
  3. RE2 covers a lot of ground when it comes to regular expressions. From reading the docs, it appears that the only things that don't work the same as std::regex are complicated back-tracking and 'magic' mechanisms

So there are two approaches here; I suggest we follow 1:

  1. We default to SafeRegex everywhere and remove the feature flag, because setting it exposes users to the CVE.
    This is my preferred approach and is followed in this PR: no feature flag is read. A follow-up PR will be needed to remove the feature flag from Route generation.
  2. We read the feature flag in our header regex matching code, just as upstream does in the Route generation, to decide which RegEx library to use.
    This would need changes in this PR (adding an if-clause).

Co-authored-by: Daniel Grimm <dgrimm@redhat.com>
@dgn dgn changed the title MAISTRA-224: Support for authorisation regex matching on request headers [maistra-1.1-future] MAISTRA-224: Support for authorisation regex matching on request headers Dec 19, 2019
@twghu twghu requested review from twghu and removed request for twghu January 10, 2020 09:45
@dgn dgn changed the title [maistra-1.1-future] MAISTRA-224: Support for authorisation regex matching on request headers [maistra-1.1] MAISTRA-224: Support for authorisation regex matching on request headers Jan 10, 2020
@dgn dgn changed the base branch from maistra-1.1-future to maistra-1.1 January 10, 2020 15:45
Copy link
Member

@jwendell jwendell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any doc that should be updated as well?

@dgn
Copy link
Contributor Author

dgn commented Jan 21, 2020

Is there any doc that should be updated as well?

I'm not sure. I guess, because theoretically, old regexes might break. I created an issue in our 1.1.0 sprint to make sure we don't forget that it needs to be mentioned in docs: https://issues.redhat.com/browse/MAISTRA-1116

@mergify mergify bot merged commit b0f47f2 into maistra:maistra-1.1 Jan 22, 2020
dgn pushed a commit to dgn/istio-maistra that referenced this pull request Jan 28, 2020
@dgn dgn deleted the header-regex-matching branch January 23, 2023 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants