Skip to content

Add support for RuleGroup CRD #6

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

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

TiberiuGC
Copy link
Contributor

@TiberiuGC TiberiuGC commented Aug 8, 2024

Issue #, if available: aws-controllers-k8s/community#1300

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

ack-prow bot commented Aug 8, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ack-prow ack-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2024
@ack-prow ack-prow bot requested a review from a-hilaly August 8, 2024 10:19
@TiberiuGC
Copy link
Contributor Author

/test all

1 similar comment
@TiberiuGC
Copy link
Contributor Author

/test all

ack-prow bot pushed a commit to aws-controllers-k8s/code-generator that referenced this pull request Aug 14, 2024
Issue #, if available: prerequisite for WAFv2 [RuleGroup](aws-controllers-k8s/wafv2-controller#6) and [WebACL](aws-controllers-k8s/wafv2-controller#7) CRDs

Description of changes:

Some WAFv2 API fields have empty json specs `{}`, e.g. https://docs.aws.amazon.com/waf/latest/APIReference/API_AllQueryArguments.html

For these type of fields, codegen currently errors out because it infers their gotypes as e.g. 
`AllQueryArguments *AllQueryArguments`

but does not generate a `type AllQueryArguments struct` since the struct itself is empty, and not picked up by `newFieldRecurse` function.

The solution proposed in this PR is to allow users to define `marker-shapes`, which instruct codegen to overwrite the type of these empty structs as `[]byte`, both when generating the APIs and when setting up the SDK. 

e.g. [generator.yaml](https://github.com/aws-controllers-k8s/wafv2-controller/blob/bb682409da8f96c5035783602b9d948c8cc8e21f/apis/v1alpha1/generator.yaml#L15-L24) for WAFv2, and inline:

```
empty_shapes:
- All
- Method
- UriPath
- QueryString
- AllQueryArguments
- RateLimitIP
- RateLimitForwardedIP
- RateLimitHTTPMethod
- NoneAction
```

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@TiberiuGC TiberiuGC force-pushed the resource/rule-groups branch from bb68240 to 7ed2689 Compare August 14, 2024 16:28
@TiberiuGC TiberiuGC marked this pull request as ready for review August 14, 2024 16:29
@ack-prow ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2024
@TiberiuGC TiberiuGC force-pushed the resource/rule-groups branch from 7ed2689 to 239a645 Compare August 14, 2024 20:59
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Great stuff Tibi! i left few comments in line

// use in web requests in an effort to bypass detection.
type TextTransformation struct {
Priority *int64 `json:"priority,omitempty"`
Type *string `json:"type_,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

We can rename this json tag to type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you point me to a similar example 🙏🏻 ?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@TiberiuGC TiberiuGC Aug 20, 2024

Choose a reason for hiding this comment

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

// A rule statement that inspects for cross-site scripting (XSS) attacks. In
// XSS attacks, the attacker uses vulnerabilities in a benign website as a vehicle
// to inject malicious client-site scripts into other legitimate web browsers.
XssMatchStatement *XssMatchStatement `json:"xssMatchStatement,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Should we add an exception in pkg/names about Xss/XSS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// A rule statement that inspects for malicious SQL code. Attackers insert malicious
// SQL code into web requests to do things like modify your database or extract
// data from it.
SQLiMatchStatement *SQLiMatchStatement `json:"sqliMatchStatement,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

SQLi or SQLI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SQLI sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

//
// Provide the JA3 fingerprint string from the logs in your string match statement
// specification, to match with any future requests that have the same TLS configuration.
JA3Fingerprint *JA3Fingerprint `json:"jA3Fingerprint,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

s/jA3/JA3/g

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TiberiuGC TiberiuGC force-pushed the resource/rule-groups branch from 27267cd to d9616ee Compare August 20, 2024 11:33
@a-hilaly
Copy link
Member

/test all

@a-hilaly
Copy link
Member

/retest

Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Super neat 👍

@ack-prow ack-prow bot added the approved label Aug 27, 2024
@a-hilaly
Copy link
Member

/lgtm

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2024
Copy link

ack-prow bot commented Aug 27, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, TiberiuGC

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-prow ack-prow bot merged commit 57d8a33 into aws-controllers-k8s:main Aug 27, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants