MySQL dbshell command should respect the encryption flag

[Peter Zsoldos]

the 'OPTIONS': {'ssl': {'ca': '....'}} database setting should translate to --ssl-ca=... command line option to the mysql executable

[Peter Zsoldos]

See pull request ​https://github.com/django/django/pull/2674

[benjaoming]

No regressions found: Verified that all dbshell tests are running for MySQL and that newly created MySQL project still connects via non-SSL.

[Denis Cornehl]

verified, too (before I set the "ready" flag.

Connect to local, remote, with or without ca is fine (and the CA is used).

[Sasha Romijn]

I can see the use for dbshell here, but ca is not a ​supported parameter in MySQLdb. As far as I can see, all currently supported OPTION's are, i.e. they are both MySQLdb options and can be translated to mysql command line options. However, this change would only apply to dbshell, which may be entirely unexpected by users.

The ​current documentation simply refers to MySQLdb to figure out which options exist, so the ca option also has no documentation in this patch, as MySQLdb doesn't mention it, as it's not a MySQLdb option.

[Denis Cornehl]

In the doc you provided, there is an parameter ssl which translates the given dict to ​http://dev.mysql.com/doc/refman/5.0/en/mysql-ssl-set.html, which accepts a ca as a parameter. Since our backend just adds all provided options to the connect call, I think this is supported by the mysql backend at the moment.

Or did I misread anything? Looks like this is supported at the moment.

[Sasha Romijn]

Well yes, MySQLdb does appear to support this kind of option, but the mapping seems currently incorrect. The existing PR will have Django ​pass a `ca` parameter to MySQLdb. However, it should pass a dict into the ssl parameter, with a ca key, in my interpretation of the docs, and that seems ​confirmed by the source. I think it silently ignores unknown kwargs, causing the tests to still succeed.

[Denis Cornehl]

Perhaps I'm missing the point.

But the OP stated his options are (formatted):

{
    'OPTIONS': {
        'ssl': {
            'ca': '....'
        }
    }
 }

which is exactly the same as he uses for the --ssl-ca command line argument:

cert = settings_dict['OPTIONS'].get('ssl', {}).get('ca')

[anonymous]

So there's a bug (dbshell doesn't work when using MySQL+SSL), there's a patch that solves the issue without breaking anything else. What's the holdup?

[Denis Cornehl]

the holdup is, if the used option (add ssl with ca) is a official supported option by MySQLdb. If not, we don't support it. If it's a supported and used option, we should support it.

[Sasha Romijn]

Apologies, I am completely mistaken. I misread settings_dict['OPTIONS'].get('ssl', {}).get('ca') as settings_dict['OPTIONS'].get('ca'). I completely concur now that the current format should be supported by both MySQLdb and our DatabaseClient.

With that out of the way, there's just one issue: I think this deserves a short mention in the release notes (for 1.8). Could you add that to the PR?

[Peter Zsoldos]

@erikr: I'm quite new so please confirm that "add release notes (for 1.8) to the PR" means:

1. Adding an entry to ​https://github.com/django/django/blob/master/docs/releases/1.8.txt under "minor changes"
2. git commit --amend
3. git push --force

[Sasha Romijn]

Yes zsoldosp, that should be it :)

[Robert]

Out of curiosity: what are the chances for this to be applied (backported?) to 1.6 and/or 1.7?

[Tim Graham]

Robert, this won't be backported. Those branches are feature frozen right now. You can read more about that in ​our release process.

[Peter Zsoldos]

@erikr: just realized I haven't yet finished this. Release notes are added, pull request is updated. Please merge!

[Peter Zsoldos]

the pull request has been updated with all the needed changes - what is preventing it from being merged?
Replying to erikr:

Yes zsoldosp, that should be it :)

[Tim Graham]

Someone reviewing the patch and marking it "Ready for Checkin" is the next step. I have reviewed it and left comments for improvement on the PR. Please uncheck "Patch needs improvement" when you update it, thanks.

[Peter Zsoldos]

adjusted commit and pull request based on @timgraham's feedback. Also added myself to the AUTHORS file as per committing guidelines.

[Tim Graham <timograham@…>]

In 01801edd3760f97a4ebc4d43ca5bbdbbdfebbb0a:

Fixed #22646: Added support for the MySQL ssl-ca option to dbshell.