-
Notifications
You must be signed in to change notification settings - Fork 80
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
[WFLY-15452] - modify ajp-listener to allow specifying pattern for aj… #435
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,130 @@ | ||
= [Preview]modify ajp-listener to allow specifying pattern for ajp request attributes | ||
:author: Bartosz Baranowski | ||
:email: [email protected] | ||
:toc: left | ||
:icons: font | ||
:idprefix: | ||
:idseparator: - | ||
|
||
== Overview | ||
|
||
Since UNDERTOW-1667 one can set additional AJP request attribute parsing permission via env variable. However there is no way to set it in WFLY config/model. This RFE's goal is to make it possible. | ||
|
||
== Issue Metadata | ||
|
||
=== Issue | ||
|
||
* https://issues.redhat.com/browse/WFLY-15452[WFLY-15452] | ||
|
||
=== Related Issues | ||
|
||
* https://issues.redhat.com/browse/UNDERTOW-1667[UNDERTOW-1667] | ||
* https://issues.redhat.com/browse/UNDERTOW-1977[UNDERTOW-1977] | ||
* https://issues.redhat.com/browse/WFLY-15453[WFLY-15453] | ||
|
||
=== Stability Level | ||
// Choose the planned stability level for the proposed functionality | ||
* [ ] Experimental | ||
|
||
* [X] Preview | ||
|
||
* [ ] Community | ||
|
||
* [ ] default | ||
|
||
=== Dev Contacts | ||
|
||
* mailto:{email}[{author}] | ||
|
||
=== QE Contacts | ||
|
||
* mailto:[email protected][Martin Svehla] | ||
|
||
=== Testing By | ||
// Put an x in the relevant field to indicate if testing will be done by Engineering or QE. | ||
// Discuss with QE during the Kickoff state to decide this | ||
* [ ] Engineering | ||
|
||
* [X] QE | ||
|
||
=== Affected Projects or Components | ||
|
||
* undertow | ||
|
||
=== Other Interested Projects | ||
|
||
=== Relevant Installation Types | ||
// Remove the x next to the relevant field if the feature in question is not relevant | ||
// to that kind of WildFly installation | ||
* [x] Traditional standalone server (unzipped or provisioned by Galleon) | ||
|
||
* [x] Managed domain | ||
|
||
* [x] OpenShift s2i | ||
|
||
* [x] Bootable jar | ||
|
||
== Requirements | ||
|
||
=== Hard Requirements | ||
|
||
* Being able to configure pattern via model/xml. | ||
baranowb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
[literal] | ||
<subsystem xmlns="urn:jboss:domain:undertow:14.0" default-server="some-server" default-servlet-container="myContainer" default-virtual-host="default-virtual-host" instance-id="some-id" statistics-enabled="true"> | ||
... | ||
<server default-host="other-host" name="some-server" servlet-container="myContainer"> | ||
... | ||
<ajp-listener ... allowed_request_attr_pattern="(?:apple|banana)" .../> | ||
... | ||
</server> | ||
... | ||
</subsystem> | ||
|
||
Parameters will be present in undertow server element(for standalone: /subsystem=undertow/server=default-server/ajp-listener=myListener): | ||
* allowed_request_attr_pattern | ||
** Default: null | ||
** Type: String(regex - java.util.regex.Pattern) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What should be an outcome when custom attribute doesn't match the pattern? Should it be ignored, but still processed (200) or should the listener return 403? |
||
|
||
=== Nice-to-Have Requirements | ||
|
||
=== Non-Requirements | ||
|
||
== Backwards Compatibility | ||
|
||
Possibly. Subsystem transformers should be able to handle it. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @baranowb @bstansberry may we (Migration feature team) assume that if there is any need for migration this will be handled by the parsers/transformers. "Possibly" is not really reassuring that this feature team is fully handling migration on its own. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
=== Default Configuration | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think default configuration refers to what will be the default configuration of the server, can someone vouch if I'm right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer "standard configuration" when describing the standalone/host/domain.xml files we include in our standard distribution zip. "Default" is ambiguous because many attributes have default values but those are only relevant is a resource with is configured. Plus, our standard configurations may have a value for an attribute that is different from its default value. So I prefer to only use 'default' to refer to default attribute behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps the default value (of the new attribute) could be undefined as in when defining the attribute definition, you don't use the And as for the standard configuration, if the WDYT? @fl4via @baranowb @bstansberry There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @PrarthonaPaul looking at https://github.com/wildfly/wildfly/pull/14789/files I see the attribute has no default value, so it looks like what you describe is what was implemented. AIUI the effect of that is if the ajp-listener is configured with this feature in place the listener behavior is no different from before; i.e. undertow will reject unknown request attributes. So this feature has no 'Default configuration' compatibility concerns. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good! Thanks @bstansberry There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
No change. | ||
|
||
=== Importing Existing Configuration | ||
|
||
No steps should suffice, as it would mean defaulting to 'null', which is default value in undertow source. | ||
baranowb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
=== Deployments | ||
|
||
Not affected. | ||
|
||
=== Interoperability | ||
|
||
Not affected. | ||
|
||
== Implementation Plan | ||
|
||
Done. | ||
|
||
== Security Considerations | ||
|
||
Possibly. UNDERTOW-1667 is a CVE, so this RFE should be documented well, in order to warn users of potential exposure. | ||
baranowb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
== Test Plan | ||
|
||
Unit tests should cover new functionality(there is already test case covering AjpListener). | ||
baranowb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
== Community Documentation | ||
|
||
Task for WFLY documentation already exist - WFLY-15453. HOwever, this is model change and there is model reference doc generated, so its unclear which approach is better? | ||
baranowb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
== Release Note Content | ||
|
||
Allow configuration of AJP request attribute pattern with model entry, rather than only via system property. |
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.
Is this correct? IMO we are aiming for
dafault
with EAP7-1836.