Hi Phillip,
On 11/7/09 2:14 PM, Phillip Moore wrote:
> I'm not sure that this ever showed up on the list, so my apologies if
> you're seeing this for the second time. I figured someone would at
> least have an opinion on my approach to the API changes. I can do
> all of the coding involved here, and am perfectly happy volunteering
> to do so, but I need feedback on the API changes, at the very least.
> I don't want to put a lot more effort into something that will just
> end up getting rejected.
>
> I am of the opinion that without CRL support, the SSL support in MySQL
> is really incomplete, and I think this is an important enhancement,
> from a security perspective as well. The ability to revoke a
> certificate (which I know most people don't worry about, since they
> get certs with lifetimes that span many years) is dependent on the
> ability to publish the CRL to your server infrastructure.
>
> All I'm asking for is help working out an acceptable design for this
> change (both to the code, and to the test suite), and then you can all
> sit back and wait for the patch.
>
OK.
>
> I recently implemented SSL for an application I own, and automated
> everything from start to finish. I can recreate the entire set of
> certificates (from the root up) with a couple of commands. I can
> refresh the certs for a given server transparently, which includes
> revoking the old ones, and publishing a new CRL file to all of my
> servers, effectively making it impossible to use the old revoked
> certs.
Makes sense.
> I wanted to extend this infrastructure to secure access to my
> database, and found that the lack of CRL support in MySQL makes it
> effectively impossible. With no support for CRL files, a revoked
> certificate is actually accepted by the MySQL server up until it
> expires.
Hum, bad.
> I have added CRL support to other software recently, as part of this
> effort. Perl's IO::Socket::SSL didn't support a simple CRL file, and
> that was trivial to patch, so I looked into making the same
> enhancements to MySQL. Starting with 5.1.40, I was able to get SSL
> CRLs working, proving that it's relatively straight forward, but I
> need to get some feedback on the details of the patch before I am
> willing to submit this for inclusion in the product. The patch is
> available as an attachment to this bug, on which I commented:
>
> http://bugs.mysql.com/bug.php?id=31224
A nitpick comment about the patch: please use unified diff.
> My approach was very simple: search the code for all references to
> "ssl_ca" and add an additional option/variable/whatever called
> "ssl_crl". Extending the configuration file to add an "ssl_crl"
> parameter was trivial, and fully backwards compatible. However, a
> number of functions take a list of arguments, including the ssl_ca,
> ssl_capath, ssl_key, etc. It seemed logical to simple extend these by
> adding an additional argument, since that was probably how they should
> have been designed in the first place.
Yes.
> For the internal functions, such as new_VioSSLConnectorFd() or
> new_VioSSLAcceptFd(), function I don't think are used outside of mysql
> itself (not entirely sure, to be honest) it probably doesn't matter.
> For functions like mysql_ssl_set(), which are used by applications
> linking against MySQL libraries (eg. perl's DBD::mysql, next on my
> list to patch :-) this represents a major headache.
>
> My patch changed mysql_ssl_set(), but I am convinced this is wrong.
Yes, kind of. We can't change the ABI in a stable release.
> This is one of two areas where I'd like some feedback before reworking
> the patch for submission. The pain introduced by changing an
> exported function's signature doesn't seem worth it, since this has
> been a stable interface for about 2 years or more. Therefore, I'm
> suggesting a replacement function in order to maintain backwards
> compatibility. Since apps that use this function will require code
> changes to use a CRL anyway, this seems like the Right Thing.
>
> Therefore, I've concluded we need a new function, but what should it be named?
>
> mysql_ssl_setup()
> mysql_ssl_configure()
>
> If it were up to me, I'd pick the latter.
Either one is fine. But how would this function work?
> The same judgment call has to be made for any other function call that
> is part of the exported programmatic API. Here's a complete list of
> the other functions I changed, to which this applies:
>
> new_VioSSLConnectorFd()
> new_VioSSLAcceptorFd()
> new_VioSSLFd()
Those are private functions, no need to worry.
> That's it. Now, the next issue I have with this code is that the CRL
> file is read into memory and the contents stashed in the SSL context
> data structures when SSL is initialized. Unlike the ca, cert, and key
> files, which are re-read for each connection, the CRL file is only
> read once. If the CRL file is updated, then it requires a restart of
> mysqld to take effect. All of the other SSL-related config files are
> read dynamically when a connection is made.
>
> That's just wrong -- if updating one file takes effect in realtime,
> updating ALL of the SSL files should take effect in realtime.
> Requiring a server restart is so... 20th century. I think it makes
> sense to re-read the CRL file and stash it in the SSL context data
> prior to the authentication of each SSL connection, to ensure that the
> file is read dynamically. I'm not entirely sure WHERE this should be
> done in the code.
It appears to be done in the right place. But it's all about trade offs,
there is a penalty in reading a file for every new connection..
> Finally, the automated test suite needs to be enhanced, and I've
> barely even looked at it. I'd like to work with whoever wrote the
> original SSL test suite to make those enhancements, if they are still
> working on MySQL. If not, then I have the necessary expertise to take
> that over, and against my better judgment, will volunteer to do so, in
> order to make the world a safer and more secure place. :-P
It should be quite simple to augment a new test. Just take a look at the
ssl tests in mysql-test/t/ssl_*
Regards,
-- Davi Arnaut
