[Django] #22646: MySQL dbshell command should respect the encryption flag
Django
-------------------------------+----------------------
Reporter: zsoldosp | Owner: zsoldosp
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------
the {{{'OPTIONS': {'ssl': {'ca': '....'}}}}} database setting should
translate to {{{--ssl-ca=...}}} command line option to the mysql
executable
--
Ticket URL: <https://code.djangoproject.com/ticket/22646>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by zsoldosp):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
Comment:
See pull request https://github.com/django/django/pull/2674
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:1>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by benjaoming):
No regressions found: Verified that all dbshell tests are running for
MySQL and that newly created MySQL project still connects via non-SSL.
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:2>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by syphar):
* cc: denis.cornehl@… (added)
* type: Uncategorized => Cleanup/optimization
* component: Uncategorized => Database layer (models, ORM)
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:3>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
Severity: Normal | checkin
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by syphar):
* stage: Accepted => Ready for checkin
Comment:
verified, too (before I set the "ready" flag.
Connect to local, remote, with or without ca is fine (and the CA is used).
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:4>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
Keywords: | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by erikr):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
* needs_docs: 0 => 1
Comment:
I can see the use for `dbshell` here, but `ca` is not a [[http://mysql-
python.sourceforge.net/MySQLdb.html|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 [[https://docs.djangoproject.com/en/dev/ref/databases/#connecting-to-
the-database|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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:5>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by syphar):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:6>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by erikr):
Well yes, MySQLdb does appear to support this kind of option, but the
mapping seems currently incorrect. The existing PR will have Django
[[https://github.com/django/django/blob/master/django/db/backends/mysql/base.py#L467|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
[[https://github.com/farcepest/MySQLdb1/blob/master/_mysql.c#L620|confirmed
by the source]]. I think it silently ignores unknown kwargs, causing the
tests to still succeed.
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:7>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by syphar):
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')
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:8>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:9>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by syphar):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:10>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:11>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by zsoldosp):
@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
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:12>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: zsoldosp (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:13>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by erikr):
Yes zsoldosp, that should be it :)
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:14>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Robert):
Out of curiosity: what are the chances for this to be applied
(backported?) to 1.6 and/or 1.7?
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:15>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
Robert, this won't be backported. Those branches are feature frozen right
now. You can read more about that in
[https://docs.djangoproject.com/en/dev/internals/release-process
/#supported-versions our release process].
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:16>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by zsoldosp):
@erikr: just realized I haven't yet finished this. Release notes are
added, pull request is updated. Please merge!
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:17>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 1
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by zsoldosp):
the pull request has been updated with all the needed changes - what is
preventing it from being merged?
Replying to [comment:14 erikr]:
> Yes zsoldosp, that should be it :)
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:18>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:19>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:20>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
adjusted commit and pull request based on @timgraham's feedback. Also
added myself to the AUTHORS file as per committing guidelines.
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:21>
Django
Reporter: zsoldosp | Owner: zsoldosp
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Database layer | Resolution: fixed
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"01801edd3760f97a4ebc4d43ca5bbdbbdfebbb0a"]:
{{{
#!CommitTicketReference repository=""
revision="01801edd3760f97a4ebc4d43ca5bbdbbdfebbb0a"
Fixed #22646: Added support for the MySQL ssl-ca option to dbshell.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22646#comment:22>