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 Peter Zsoldos, 10 years ago

Has patch: set

comment:2 by benjaoming, 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 Denis Cornehl, 10 years ago

Cc: denis.cornehl@… added
Component: UncategorizedDatabase layer (models, ORM)
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:4 by Denis Cornehl, 10 years ago

Triage Stage: AcceptedReady 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 Sasha Romijn, 10 years ago

Needs documentation: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 Denis Cornehl, 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 Sasha Romijn, 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 Denis Cornehl, 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 anonymous, 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 Denis Cornehl, 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 Sasha Romijn, 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 Peter Zsoldos, 10 years ago

@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

comment:13 by Peter Zsoldos, 10 years ago

Cc: Peter Zsoldos added

comment:14 by Sasha Romijn, 10 years ago

Yes zsoldosp, that should be it :)

comment:15 by Robert, 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 Tim Graham, 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 Peter Zsoldos, 10 years ago

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

in reply to:  14 comment:18 by Peter Zsoldos, 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 Peter Zsoldos, 10 years ago

Needs documentation: unset

comment:20 by Tim Graham, 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 Peter Zsoldos, 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 Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 01801edd3760f97a4ebc4d43ca5bbdbbdfebbb0a:

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

Note: See TracTickets for help on using tickets.
Back to Top