Bug 64180 - secretRequred=false is ignored if secret=<anything>
Summary: secretRequred=false is ignored if secret=<anything>
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Connectors (show other bugs)
Version: 9.0.31
Hardware: PC Linux
: P2 enhancement (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-26 16:31 UTC by Tom
Modified: 2020-02-28 09:52 UTC (History)
2 users (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom 2020-02-26 16:31:12 UTC
I have defined my server.xml as follows to allow the ajp.secretRequired and ajp.secret values as command line properties so I don't have to edit the server.xml on each server.

    <!-- Define an AJP 1.3 Connector on port 8009 -->
    <Connector protocol="AJP/1.3"
        address="${ajp.address}"
        connectionTimeout="20000"
        acceptCount="100"
        maxThreads="1600"
        minSpareThreads="100"
        port="8009"
        redirectPort="8443"
        secretRequired="${ajp.secretRequired}"
        secret="${ajp.secret}"
    />

If ajp.secretRequired=true and ajp.secret is set to something it all works as expected.  However, if ajp.secretRequired=false and ajp.secret="" (or is not set at all) the AJP connector still requires the secret to be passed.  

Steps to reproduce:  

1.  Set secretRequired=false secret="" in the server.xml 
2.  Try to connect from a client that does not pass a secret
Comment 1 Remy Maucherat 2020-02-26 16:38:15 UTC
If a secret is configured, it needs to be submitted by the client. secretRequired only forces the user (by default) to configure a secret in server.xml.
Comment 2 Tom 2020-02-26 17:44:24 UTC
Would this be a change that you would be willing to consider?  

The current behaviour is non-intuitive and makes working with parametrized server.xml files impossible.  

As soon as secret is in there anywhere (even an invalid one, it accepts "") then it's mandatory.   

As an admin and package maintainer I feel that the the secretRequired field should trump any setting in secret based on the current description in the documentation.
Comment 3 Mark Thomas 2020-02-26 17:55:44 UTC
The current behaviour is as expected / intended.

I can see merit in changing the behaviour so empty string and null (not set) are equivalent for secret. We could also clarify that secretRequired indicates whether the secret attribute MUST be set, not whether the client must provide a secret and that (independent of secretRequired) the client MUST provide the correct secret if secret is non-null and non-zero length.

Moving this to a proposed enhancement request for the changes described in this comment.
Comment 4 Tom 2020-02-26 18:59:07 UTC
The proposed enhancement still does not address the ability to parametrize the server.xml.  

Once the keyword secret= is there, it becomes mandatory even if it's "".  

With your proposal would my server.xml below work as expected if secretRequired=false? (no secret required) or would tomcat fail to start because secret="" is invalid?
Comment 5 mgrigorov 2020-02-27 08:09:18 UTC
(In reply to Tom from comment #4)
> The proposed enhancement still does not address the ability to parametrize
> the server.xml.  
> 
> Once the keyword secret= is there, it becomes mandatory even if it's "".  
> 
> With your proposal would my server.xml below work as expected if
> secretRequired=false? (no secret required) or would tomcat fail to start
> because secret="" is invalid?


If secret="" is treated the same way as missing attribute, i.e. =null, then it will work.

I haven't tried it myself but maye you can script it today with:

ajp.secretAttribute=secret=abcd1234

or

ajp.secretAttribute=           # no value, i.e. ""

and then


<Connector protocol="AJP/1.3"
        ...
        ${ajp.secretAttribute}
    />
Comment 6 Remy Maucherat 2020-02-27 09:37:38 UTC
This is mostly wontfix as I am certain the request of the reporter is that secretRequired disables the need for the client to send the secret.

10.0.0-M2, 9.0.32, 8.5.52 and 7.0.101 will now allow setting a secret as "", and it will be treated as it it had not been set (= null). No doc fix was needed, secretRequired was properly documented already.
Comment 7 Mark Thomas 2020-02-27 09:45:12 UTC
I agree the wording was correct but I had some text already committed locally to make it even more explicit so I push those changes it as they can't do any harm and may help as it is clear that some users were reading the docs and taking away a different meaning from the one intended.
Comment 8 Remy Maucherat 2020-02-27 09:49:03 UTC
After reading it, I agree. It's always possible to document things better :)
Comment 9 Christopher Schultz 2020-02-27 22:09:18 UTC
(In reply to Remy Maucherat from comment #6)
> This is mostly wontfix as I am certain the request of the reporter is that
> secretRequired disables the need for the client to send the secret.
> 
> 10.0.0-M2, 9.0.32, 8.5.52 and 7.0.101 will now allow setting a secret as "",
> and it will be treated as it it had not been set (= null). No doc fix was
> needed, secretRequired was properly documented already.

Is this how mod_jk/mod_proxy_ajp are working as well?

For example, in mod_* is it possible to use a "secret" which is the empty-string? Or do they also treat empty-secret as "no secret"?
Comment 10 Tom 2020-02-27 23:09:33 UTC
(In reply to Remy Maucherat from comment #6)
> This is mostly wontfix as I am certain the request of the reporter is that
> secretRequired disables the need for the client to send the secret.
> 
> 10.0.0-M2, 9.0.32, 8.5.52 and 7.0.101 will now allow setting a secret as "",
> and it will be treated as it it had not been set (= null). No doc fix was
> needed, secretRequired was properly documented already.

This should fix my usecase perfectly.  

Thank you.
Comment 11 mgrigorov 2020-02-28 09:52:55 UTC
(In reply to Christopher Schultz from comment #9)
> (In reply to Remy Maucherat from comment #6)
> > This is mostly wontfix as I am certain the request of the reporter is that
> > secretRequired disables the need for the client to send the secret.
> > 
> > 10.0.0-M2, 9.0.32, 8.5.52 and 7.0.101 will now allow setting a secret as "",
> > and it will be treated as it it had not been set (= null). No doc fix was
> > needed, secretRequired was properly documented already.
> 
> Is this how mod_jk/mod_proxy_ajp are working as well?
> 
> For example, in mod_* is it possible to use a "secret" which is the
> empty-string? Or do they also treat empty-secret as "no secret"?

https://github.com/apache/httpd/commit/d8b6d798c177dfdb90cef1a29395afcc043f3c86#diff-8992fe85968a8915e13ad663eb47d62fR206

I.e. if the value is "" it will be set.
If *conn->worker->s->secret is NULL it will set "secret" to NULL, i.e. do nothing. I haven't coded in C for many years but I have the feeling this 'if' is not really needed.