Opened 10 years ago
Closed 10 years ago
#22646 closed Cleanup/optimization (fixed)
MySQL dbshell command should respect the encryption flag
Reported by: | Peter Zsoldos | Owned by: | Peter Zsoldos |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Daniel Quinn, denis.cornehl@…, Peter Zsoldos | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
the 'OPTIONS': {'ssl': {'ca': '....'
}} database setting should translate to --ssl-ca=...
command line option to the mysql executable
Change History (22)
comment:1 by , 10 years ago
Has patch: | set |
---|
comment:2 by , 10 years ago
No regressions found: Verified that all dbshell tests are running for MySQL and that newly created MySQL project still connects via non-SSL.
comment:3 by , 10 years ago
Cc: | added |
---|---|
Component: | Uncategorized → Database layer (models, ORM) |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
comment:4 by , 10 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
verified, too (before I set the "ready" flag.
Connect to local, remote, with or without ca is fine (and the CA is used).
comment:5 by , 10 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Ready for checkin → Accepted |
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.
comment:6 by , 10 years ago
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.
comment:7 by , 10 years ago
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.
comment:8 by , 10 years ago
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')
comment:9 by , 10 years ago
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?
comment:10 by , 10 years ago
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.
comment:11 by , 10 years ago
Patch needs improvement: | unset |
---|
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?
comment:12 by , 10 years ago
@erikr: I'm quite new so please confirm that "add release notes (for 1.8) to the PR" means:
- Adding an entry to https://github.com/django/django/blob/master/docs/releases/1.8.txt under "minor changes"
- git commit --amend
- git push --force
comment:13 by , 10 years ago
Cc: | added |
---|
comment:15 by , 10 years ago
Out of curiosity: what are the chances for this to be applied (backported?) to 1.6 and/or 1.7?
comment:16 by , 10 years ago
Robert, this won't be backported. Those branches are feature frozen right now. You can read more about that in our release process.
comment:17 by , 10 years ago
@erikr: just realized I haven't yet finished this. Release notes are added, pull request is updated. Please merge!
comment:18 by , 10 years ago
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 :)
comment:19 by , 10 years ago
Needs documentation: | unset |
---|
comment:20 by , 10 years ago
Patch needs improvement: | set |
---|
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.
comment:21 by , 10 years ago
Patch needs improvement: | unset |
---|
adjusted commit and pull request based on @timgraham's feedback. Also added myself to the AUTHORS file as per committing guidelines.
comment:22 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
See pull request https://github.com/django/django/pull/2674