Adding support for sslhostconfig options

[malikparvez]

Summary

Adding support for sslhostconfig options.

 <SSLHostConfig>
    <Certificate
        certificateKeyFile="conf/localhost-rsa-key.pem"
        certificateFile="conf/localhost-rsa-cert.pem"
        certificateChainFile="conf/localhost-rsa-chain.pem"
        type="RSA"
        />
  </SSLHostConfig>

read more about it in tomcat docs

Additional Context

Add any additional context about the problem here.

• Root cause and the steps to reproduce. (If applicable)
• Thought process behind the implementation.

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

• 🟢 Spec tests.
• 🟢 Acceptance tests.
• Manually verified. (For example puppet apply)

[Ramesh7]

The changes looks good, would like to have another review from cummunity.

[tedgarb]

Thanks, this looks like a good first pass at fixing #545, have a couple pieces of immediate feedback.

I will need to get a working test environment together to validate my belief, but at first blush this does not look right to me. To use the doc example, it should be:

<Connector
    protocol="org.apache.coyote.http11.Http11NioProtocol"
    port="8443"
    maxThreads="150"
    SSLEnabled="true"
    maxParameterCount="1000"
    >
  <SSLHostConfig>
    <Certificate
        certificateKeyFile="conf/localhost-rsa-key.pem"
        certificateFile="conf/localhost-rsa-cert.pem"
        certificateChainFile="conf/localhost-rsa-chain.pem"
        type="RSA"
        />
  </SSLHostConfig>
</Connector>

It looks like the code here is referring to a path with bits like

$sslhostconfig_path = "Server/Service/Connector[#attribute/port='${port}']"
"set ${sslhostconfig_path}/Certificate/#attribute/certificateKeyFile ${cert_key_file}",

So the implied path is Server/Service/Connector[#attribute/port='${port}']/Certificate/#attribute/certificateKeyFile ${cert_key_file}, I think it is supposed to be Server/Service/Connector[#attribute/port='${port}']/SSLHostConfig/Certificate/#attribute/certificateKeyFile ${cert_key_file}.

Other obvious issue is if $cert_key_file and $cert_file and $cert_chain_file in the logic, which means if you specify some but not all of the values, it silently does nothing but does not produce any hint unless you read the code here.

[malikparvez]

Thanks, this looks like a good first pass at fixing #545, have a couple pieces of immediate feedback.

I will need to get a working test environment together to validate my belief, but at first blush this does not look right to me. To use the doc example, it should be:

<Connector
    protocol="org.apache.coyote.http11.Http11NioProtocol"
    port="8443"
    maxThreads="150"
    SSLEnabled="true"
    maxParameterCount="1000"
    >
  <SSLHostConfig>
    <Certificate
        certificateKeyFile="conf/localhost-rsa-key.pem"
        certificateFile="conf/localhost-rsa-cert.pem"
        certificateChainFile="conf/localhost-rsa-chain.pem"
        type="RSA"
        />
  </SSLHostConfig>
</Connector>

It looks like the code here is referring to a path with bits like

$sslhostconfig_path = "Server/Service/Connector[#attribute/port='${port}']"
"set ${sslhostconfig_path}/Certificate/#attribute/certificateKeyFile ${cert_key_file}",

So the implied path is Server/Service/Connector[#attribute/port='${port}']/Certificate/#attribute/certificateKeyFile ${cert_key_file}, I think it is supposed to be ``Server/Service/Connector[#attribute/port='${port}']/SSLHostConfig/Certificate/#attribute/certificateKeyFile ${cert_key_file}.

Other obvious issue is if $cert_key_file and $cert_file and $cert_chain_file in the logic, which means if you specify some but not all of the values, it silently does nothing but does not produce any hint unless you read the code here.

Hello @tedgarb thank you so much for your review, your right somehow I got wrapped into the xml and missed this.
here is the updated config file

<Server>
	<Service>
		<Connector port="8443">
			<SSLHostConfig>
				<Certificate certificateKeyFile="/path/me/key.pem" certificateFile="/path/me/cert.pem" certificateChainFile="/path/me/chain.pem" type="PEM"></Certificate>
			</SSLHostConfig>
		</Connector>
	</Service>
	<Service>
		<Connector protocol="HTTP/1.1"></Connector>
	</Service>
</Server>

and sure i will add notify to let the user know if the all required params are not passed and the changes has not been applied
.

Thank you so much for your review

[malikparvez]

@tedgarb Please review with the changes as you suggested...

[tedgarb]

The fixes to the XML pass the eyeball test, but I haven't tested on an actual machine yet.

I am still not sure the notice is quite right for two reasons:

1. it emits on any connector without these params, which is a bit aggressive/noisy in general
2. What I was hoping for is more "you have defined some but not all of these, either define all or none," potentially with this state being an outright error, though I'd welcome others to express opinions here

[malikparvez]

The fixes to the XML pass the eyeball test, but I haven't tested on an actual machine yet.

I am still not sure the notice is quite right for two reasons:

1. it emits on any connector without these params, which is a bit aggressive/noisy in general
2. What I was hoping for is more "you have defined some but not all of these, either define all or none," potentially with this state being an outright error, though I'd welcome others to express opinions here

Agree with the first point this is going to be noise so removed the notify. About 2nd point I am not exactly sure how to handle this.