Bug 65169 - Add escaped cert env variables
Summary: Add escaped cert env variables
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_ssl (show other bugs)
Version: 2.4.46
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-03-05 17:56 UTC by Michael Osipov
Modified: 2021-03-19 20:44 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Osipov 2021-03-05 17:56:01 UTC
A recent Tomcat change rejects all headers with a LF in the value. nginx has added a ssl_client_escaped_cert which does simple URI encoding. Add HTTPd equivalents of all variables which contain LFs. Here is the support for Tomcat: https://github.com/apache/tomcat/pull/406

I consider at least these are affected:
* SSL_CLIENT_CERT
* SSL_CLIENT_CERT_CHAIN_n
* SSL_SERVER_CERT
Comment 1 Joe Orton 2021-03-08 11:24:01 UTC
The obvious way to do this would be to use the base64url encoding of the DER, rather than URL-encoding the PEM, which seems kind of daft.  Can Tomcat decode the former?
Comment 2 Michael Osipov 2021-03-08 12:00:02 UTC
(In reply to Joe Orton from comment #1)
> The obvious way to do this would be to use the base64url encoding of the
> DER, rather than URL-encoding the PEM, which seems kind of daft.  Can Tomcat
> decode the former?

Not without additional code. would you prefer to introduce new variables or rather a format co fig option?
Comment 3 Mark Thomas 2021-03-08 12:20:21 UTC
Remove myself from CC.
Comment 4 Michael Osipov 2021-03-08 12:33:45 UTC
(In reply to Mark Thomas from comment #3)
> Remove myself from CC.

I intentionally added you to discuss how tomcat should handle it.
Comment 5 Joe Orton 2021-03-08 12:38:15 UTC
Adding SSL_{SERVER,CLIENT}_B64CERT variables (or whatever the name) is trivial, I  would not want to add a config option for this definitely.
Comment 6 Michael Osipov 2021-03-08 12:52:13 UTC
(In reply to Joe Orton from comment #5)
> Adding SSL_{SERVER,CLIENT}_B64CERT variables (or whatever the name) is
> trivial, I  would not want to add a config option for this definitely.

One thing needs to be considered since +ExportCertData would provide both it would consume the double amount of memory even if the admin needs only one format.  What us your opinion on it?
Comment 7 Joe Orton 2021-03-08 13:03:42 UTC
(In reply to Michael Osipov from comment #6)
> (In reply to Joe Orton from comment #5)
> > Adding SSL_{SERVER,CLIENT}_B64CERT variables (or whatever the name) is
> > trivial, I  would not want to add a config option for this definitely.
> 
> One thing needs to be considered since +ExportCertData would provide both it
> would consume the double amount of memory even if the admin needs only one
> format.  What us your opinion on it?

I don't know what the usually recommended configuration is here but it shouldn't use ExportCertData exactly because it's so expensive.  Better to use one of the configs using e.g. mod_headers which just extracts the variables which are required.

If really required, we could add a +ExportBase64CertData or similar which just does the base64url(der), that is not as painful as adding a new config options at least.

BTW, does Tomcat really want/need the CLIENT_CERT_CHAIN_n as well here?

Proof of concept here - https://github.com/apache/httpd/pull/177
Comment 8 Michael Osipov 2021-03-08 14:00:53 UTC
(In reply to Michael Osipov from comment #6)
> (In reply to Joe Orton from comment #5)
> > Adding SSL_{SERVER,CLIENT}_B64CERT variables (or whatever the name) is
> > trivial, I  would not want to add a config option for this definitely.
> 
> One thing needs to be considered since +ExportCertData would provide both it
> would consume the double amount of memory even if the admin needs only one
> format.  What us your opinion on it?

Having a Base64 version of the export option makes the system consistent. For the same reason I would export this chain too, this isn't much Tomcat specific, but for all upstream servers which want to inspect certs.

Why do you want to use Base64 URL instead of plain Base64? Other headers work with that too and Tomcat would need to add support first.
Comment 9 Joe Orton 2021-03-08 14:11:40 UTC
(In reply to Michael Osipov from comment #8)
> (In reply to Michael Osipov from comment #6)
> > (In reply to Joe Orton from comment #5)
> > > Adding SSL_{SERVER,CLIENT}_B64CERT variables (or whatever the name) is
> > > trivial, I  would not want to add a config option for this definitely.
> > 
> > One thing needs to be considered since +ExportCertData would provide both it
> > would consume the double amount of memory even if the admin needs only one
> > format.  What us your opinion on it?
> 
> Having a Base64 version of the export option makes the system consistent.
> For the same reason I would export this chain too, this isn't much Tomcat
> specific, but for all upstream servers which want to inspect certs.

Makes sense.

> Why do you want to use Base64 URL instead of plain Base64? Other headers
> work with that too and Tomcat would need to add support first.

I've a mild preference for base64url since it would allow safe use in more contexts, but plain base64 would work too if that's difficult for Tomcat.
Comment 10 Michael Osipov 2021-03-08 14:51:38 UTC
(In reply to Joe Orton from comment #9)
> (In reply to Michael Osipov from comment #8)
> > (In reply to Michael Osipov from comment #6)
> > > (In reply to Joe Orton from comment #5)
> > > > Adding SSL_{SERVER,CLIENT}_B64CERT variables (or whatever the name) is
> > > > trivial, I  would not want to add a config option for this definitely.
> > > 
> > > One thing needs to be considered since +ExportCertData would provide both it
> > > would consume the double amount of memory even if the admin needs only one
> > > format.  What us your opinion on it?
> > 
> > Having a Base64 version of the export option makes the system consistent.
> > For the same reason I would export this chain too, this isn't much Tomcat
> > specific, but for all upstream servers which want to inspect certs.
> 
> Makes sense.
> 
> > Why do you want to use Base64 URL instead of plain Base64? Other headers
> > work with that too and Tomcat would need to add support first.
> 
> I've a mild preference for base64url since it would allow safe use in more
> contexts, but plain base64 would work too if that's difficult for Tomcat.

I'd prefer plain Base64 for the sake of simplicity.
Comment 11 Joe Orton 2021-03-09 13:07:26 UTC
I've updated https://github.com/apache/httpd/pull/177 to use plain base64 and to allow "SSLOptions +ExportBase64CertData" to export both server & client cert.

What about the CERT_CHAIN_n? Are these actually useful/used/needed by Tomcat??
Comment 12 Michael Osipov 2021-03-09 21:45:36 UTC
(In reply to Joe Orton from comment #11)
> I've updated https://github.com/apache/httpd/pull/177 to use plain base64
> and to allow "SSLOptions +ExportBase64CertData" to export both server &
> client cert.

Looks better now, added a few comments

> What about the CERT_CHAIN_n? Are these actually useful/used/needed by
> Tomcat??

I would retain them too, Tomcat exposes the as an array of X509Certificate objects. It might be up to the developer to do anything fruitful with that chain. It should be there for the sake of completeness and consistency.
Comment 13 Joe Orton 2021-03-18 09:13:11 UTC
Interesting note from testing: when looking at SSL_CLIENT_CERT_CHAIN_* these are really the "set of certificates in the cert chain sent by the client" and *not* the set of certificates in the verified cert chain as built up by OpenSSL/mod_ssl.

So the client could send (client cert A, unrelated CA cert B, unrelated CA cert C), and certs B&C show up in _CERT_CHAIN_0&1 even if (A, CA X, CA Y, CA Z) was the actual client cert chain built and verified by OpenSSL.

The mod_ssl docs merely describe this _CHAIN_n as:

"PEM-encoded certificates in client certificate chain"

which is ambiguous.  Does Tomcat present/expect anything in particular here?

We could switch to the alternate OpenSSL 1.1.0 API SSL_get0_verified_chain():

https://www.openssl.org/docs/man1.1.0/man3/SSL_get0_verified_chain.html

which seems a more sensible/useful way to do it, except there is a possible behaviour difference here in the case where the client cert is *not* verified successfully, in which case we may not get a complete chain.
Comment 14 Joe Orton 2021-03-19 15:46:01 UTC
Will consider that issue separately, I think it's worth addressing too.

Fixed this in: https://svn.apache.org/viewvc?view=revision&revision=1887811

Thanks so much Michael for all the review here, appreciate it.
Comment 15 Michael Osipov 2021-03-19 20:43:45 UTC
(In reply to Joe Orton from comment #13)
> Interesting note from testing: when looking at SSL_CLIENT_CERT_CHAIN_* these
> are really the "set of certificates in the cert chain sent by the client"
> and *not* the set of certificates in the verified cert chain as built up by
> OpenSSL/mod_ssl.
> 
> So the client could send (client cert A, unrelated CA cert B, unrelated CA
> cert C), and certs B&C show up in _CERT_CHAIN_0&1 even if (A, CA X, CA Y, CA
> Z) was the actual client cert chain built and verified by OpenSSL.

This is definitively an issue. Never trust the client.

> The mod_ssl docs merely describe this _CHAIN_n as:
> 
> "PEM-encoded certificates in client certificate chain"
> 
> which is ambiguous.  Does Tomcat present/expect anything in particular here?

Tomcat does not even read the chain: https://github.com/apache/tomcat/blob/681f2afccc2f22ff5fc3d80ad7e77dbeecd083b2/java/org/apache/catalina/valves/SSLValve.java#L159-L162
But this does not mean that people don't read it in custom valves or other upstream servers.

> We could switch to the alternate OpenSSL 1.1.0 API SSL_get0_verified_chain():
> 
> https://www.openssl.org/docs/man1.1.0/man3/SSL_get0_verified_chain.html
> 
> which seems a more sensible/useful way to do it, except there is a possible
> behaviour difference here in the case where the client cert is *not*
> verified successfully, in which case we may not get a complete chain.

The proposal would be to 

* leave the chain as-is in 2.4
* Introduce a new config option for mod_ssl which lets the admin choose which chain to obtain (enum value)
* Default value for 2.4: current behavior, future value: SSL_get0_verified_chain()
Comment 16 Michael Osipov 2021-03-19 20:44:33 UTC
(In reply to Joe Orton from comment #14)
> Will consider that issue separately, I think it's worth addressing too.

It is!

> Fixed this in: https://svn.apache.org/viewvc?view=revision&revision=1887811
> 
> Thanks so much Michael for all the review here, appreciate it.

Thanks for the quick fix!