-
Notifications
You must be signed in to change notification settings - Fork 326
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
[NET-11043] crd: support request normalization and header match options to prevent L7 intentions bypass #4385
[NET-11043] crd: support request normalization and header match options to prevent L7 intentions bypass #4385
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmurret this is ready for review, the only blocking change needed is to update the api
version in go.mod
s once it's cut, which should resolve remaining test failures.
I've added backport labels for all active branches, but will likely close the 1.3.x backport in a few days since it's all but certain we won't patch that version again before 1.6.0 GA.
@@ -66,10 +66,56 @@ spec: | |||
http: | |||
description: HTTP defines the HTTP configuration for the service mesh. | |||
properties: | |||
incoming: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs here and below copied from consul
changes. Subject to change based on feedback from Docs team.
required: | ||
- sanitizeXForwardedClientCert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed by adding omitempty
to this field; now that we have multiple fields, requiring this one no longer makes sense. I don't know of any validation we require that a config entry has some explicit fields (service-defaults
is allowed to be empty), so I think just removing this is fine.
control-plane/go.mod
Outdated
// temporary for local dev | ||
replace ( | ||
github.com/hashicorp/consul/api => ../../consul-enterprise/api | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be removed once api
is cut and updated in this file
// Copyright (c) HashiCorp, Inc. | ||
// SPDX-License-Identifier: MPL-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this and metrics_util.go
were missed on the last codegen update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice work!
666ac4e
to
29a6637
Compare
Rebased and squashed for merge |
…t L7 intentions bypass * crd: support L7 intentions header match contains and ignoreCase * crd: support mesh http.incoming.requestNormalization * crd: generate updated CRDs * crd: remove requirement for mesh http.sanitizeXForwardedClientCert This is a boolean field, and should not be required. Removing the requirement allows for it to be omitted when other fields are specified.
29a6637
to
2d3d34e
Compare
Re-initiating backports to fix v2 tests that were disabled on |
Changes proposed in this PR
Supports changes in hashicorp/consul#21816 with CRD updates:
How I've tested this PR
kind
and validated both changes are reflected in xDS / config when created as CRDsHow I expect reviewers to test this PR
👀 + spot check me on the CRD update process in case I missed a step. It's been a while since I've done one of these.
Checklist