-
Notifications
You must be signed in to change notification settings - Fork 81
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
Conversation
@@ -0,0 +1,121 @@ | |||
= <INSERT TITLE HERE> |
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.
It appears you have the title at the wrong line :-)
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.
yeah.
|
||
== 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 nor directly in UndertowOptions. Later is being addressed in UNDERTOW-1978, to which this RFE is a follow up. |
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.
We need to update this, since the UndertowOptions is also already available without any PRs for UNDERTOW-1978
|
||
* [x] Managed domain | ||
|
||
* [~x] OpenShift s2i |
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 a maybe?
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.
yep
|
||
=== Hard Requirements | ||
|
||
* Being able to configure pattern via XML configuration file. |
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.
via model, it could be directly in the XML file or it could be via CLI
|
||
Possibly. Subsystem transformers should be able to handle it. | ||
|
||
=== Default Configuration |
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.
I think default configuration refers to what will be the default configuration of the server, can someone vouch if I'm right?
So, by default, are we having a allowed_request_attr_pattern in the server? Nope, because we don't have an ajp-listener. Still, the example of the xml could be valid in Hard Requirements, and very helpful for the test plan
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.
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 comment
The 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 SimpleAttributeDefinitionBuilder.setDefaultValue()
function. And the meaning of that is that there is no allowed_request_attr_pattern
, even is ajp-listener
is defined. Or if there is a default value recommended by the specification (or a value that is commonly used by other such solutions), then we can use that as the default value.
And as for the standard configuration, if the ajp-listener
is not defined by default, the value for allowed_request_attr_pattern
would also be undefined regardless of what we used for SimpleAttributeDefinitionBuilder.setDefaultValue()
WDYT? @fl4via @baranowb @bstansberry
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.
@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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Hi @baranowb
The proposal looks good! I have added some minor comments, please feel free to let me know if you have any questions.
|
||
Possibly. Subsystem transformers should be able to handle it. | ||
|
||
=== Default Configuration |
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.
Perhaps the default value (of the new attribute) could be undefined as in when defining the attribute definition, you don't use the SimpleAttributeDefinitionBuilder.setDefaultValue()
function. And the meaning of that is that there is no allowed_request_attr_pattern
, even is ajp-listener
is defined. Or if there is a default value recommended by the specification (or a value that is commonly used by other such solutions), then we can use that as the default value.
And as for the standard configuration, if the ajp-listener
is not defined by default, the value for allowed_request_attr_pattern
would also be undefined regardless of what we used for SimpleAttributeDefinitionBuilder.setDefaultValue()
WDYT? @fl4via @baranowb @bstansberry
…p request attributes
Looks good to me! |
Thanks @baranowb @fl4via and @PrarthonaPaul |
// Choose the planned stability level for the proposed functionality | ||
* [ ] Experimental | ||
|
||
* [X] Preview |
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.
|
||
== Backwards Compatibility | ||
|
||
Possibly. Subsystem transformers should be able to handle it. |
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.
@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 comment
The reason will be displayed to describe this comment to others. Learn more.
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 comment
The 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?
…p request attributes
Issue: https://issues.redhat.com/browse/WFLY-15452