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

regex: remove safe regex validation and default program size #19212

Merged
merged 1 commit into from Nov 26, 2019

Conversation

@ramaraochavali
Copy link
Contributor

ramaraochavali commented Nov 26, 2019

This PR removes the regex length validation (it is actually incorrect to validate length) and default the max program size to 1024.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali ramaraochavali requested review from geeknoid, howardjohn, linsun, rshriram and istio/wg-networking-maintainers-pilot as code owners Nov 26, 2019
@googlebot googlebot added the cla: yes label Nov 26, 2019
@ramaraochavali

This comment has been minimized.

Copy link
Contributor Author

ramaraochavali commented Nov 26, 2019

@howardjohn As discussed changed, PTAL.

// for most cases. We should look to make it configurable if this is not sufficient.
// Note that this is different from length of regex.
// Refer to https://github.com/google/re2/blob/a98fad02c421896bc75d97f49ccd245cdce7dd55/re2/re2.h#L287 for details.
const maxRegExProgramSize = 1024

This comment has been minimized.

Copy link
@ramaraochavali

ramaraochavali Nov 26, 2019

Author Contributor

@howardjohn r2e fuzzers actually validate till 9999 https://github.com/google/re2/blob/a98fad02c421896bc75d97f49ccd245cdce7dd55/re2/fuzzing/re2_fuzzer.cc#L50 - But I think we can start with 1024 to be safe and if any one complains - we can make it configurable/set it to 9999. WDYT?

This comment has been minimized.

Copy link
@howardjohn

howardjohn Nov 26, 2019

Member

The non-re2 limit was 1024 though. So prior to 1.3 the hard max was 1024 as far as I know. And no one has complained about this, so I don't think we have any need to make it configurable. They can always do an EnvoyFilter if they really want to shoot themselves in the foot and make it so large.

This comment has been minimized.

Copy link
@howardjohn

howardjohn Nov 26, 2019

Member

Oh program size is not length.. I didn't realize that.

This comment has been minimized.

Copy link
@ramaraochavali

ramaraochavali Nov 26, 2019

Author Contributor

Yeah. It is not. But I guess 1024 should satisfy most cases - if it is not as I mentioned we can extend or they have an option of Envoy filter anyways as you mentioned.

// for most cases. We should look to make it configurable if this is not sufficient.
// Note that this is different from length of regex.
// Refer to https://github.com/google/re2/blob/a98fad02c421896bc75d97f49ccd245cdce7dd55/re2/re2.h#L287 for details.
const maxRegExProgramSize = 1024

This comment has been minimized.

Copy link
@howardjohn

howardjohn Nov 26, 2019

Member

The non-re2 limit was 1024 though. So prior to 1.3 the hard max was 1024 as far as I know. And no one has complained about this, so I don't think we have any need to make it configurable. They can always do an EnvoyFilter if they really want to shoot themselves in the foot and make it so large.

// for most cases. We should look to make it configurable if this is not sufficient.
// Note that this is different from length of regex.
// Refer to https://github.com/google/re2/blob/a98fad02c421896bc75d97f49ccd245cdce7dd55/re2/re2.h#L287 for details.
const maxRegExProgramSize = 1024

This comment has been minimized.

Copy link
@howardjohn

howardjohn Nov 26, 2019

Member

Oh program size is not length.. I didn't realize that.

@istio-testing istio-testing merged commit 9b19bb2 into istio:master Nov 26, 2019
28 of 29 checks passed
28 of 29 checks passed
tide Not mergeable. Retesting: integ-security-k8s-tests_istio
Details
cla/google All necessary CLAs are signed
e2e-bookInfoTests-envoyv2-v1alpha3_istio Job succeeded.
Details
e2e-dashboard_istio Job succeeded.
Details
e2e-mixer-no_auth_istio Job succeeded.
Details
e2e-simpleTests-cni_istio Job succeeded.
Details
e2e-simpleTests-distroless_istio Job succeeded.
Details
e2e-simpleTestsMinProfile_istio Job succeeded.
Details
e2e-simpleTests_istio Job succeeded.
Details
gencheck_istio Job succeeded.
Details
integ-framework-k8s-tests_istio Job succeeded.
Details
integ-framework-local-tests_istio Job succeeded.
Details
integ-galley-k8s-tests_istio Job succeeded.
Details
integ-galley-local-tests_istio Job succeeded.
Details
integ-istioctl-k8s-tests_istio Job succeeded.
Details
integ-istioio-k8s-tests_istio Job succeeded.
Details
integ-mixer-k8s-tests_istio Job succeeded.
Details
integ-new-install-k8s-tests_istio Job succeeded.
Details
integ-pilot-k8s-tests_istio Job succeeded.
Details
integ-pilot-local-tests_istio Job succeeded.
Details
integ-security-k8s-tests_istio Job succeeded.
Details
integ-security-local-tests_istio Job succeeded.
Details
integ-telemetry-k8s-tests_istio Job succeeded.
Details
istio_e2e_cloudfoundry_istio Job succeeded.
Details
lint_istio Job succeeded.
Details
pilot-e2e-envoyv2-v1alpha3_istio Job succeeded.
Details
pilot-multicluster-e2e_istio Job succeeded.
Details
release-test_istio Job succeeded.
Details
unit-tests_istio Job succeeded.
Details
@ramaraochavali ramaraochavali deleted the ramaraochavali:fix/safe_regex branch Nov 26, 2019
sdake added a commit to sdake/istio that referenced this pull request Dec 1, 2019
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.