Opened 3 years ago

Closed 2 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: master
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 Changed 3 years ago by Peter Zsoldos

Has patch: set

comment:2 Changed 3 years ago 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.

comment:3 Changed 3 years ago by Denis Cornehl

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

comment:4 Changed 3 years ago by Denis Cornehl

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 Changed 3 years ago by Erik Romijn

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 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by Erik 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.

comment:8 Changed 3 years ago by 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')

comment:9 Changed 3 years ago 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?

comment:10 Changed 3 years ago by 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.

comment:11 Changed 3 years ago by Erik Romijn

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 Changed 3 years ago by 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

comment:13 Changed 3 years ago by Peter Zsoldos

Cc: Peter Zsoldos added

comment:14 Changed 3 years ago by Erik Romijn

Yes zsoldosp, that should be it :)

comment:15 Changed 3 years ago by Robert

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

comment:16 Changed 3 years ago by 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.

comment:17 Changed 3 years ago by Peter Zsoldos

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

comment:18 in reply to:  14 Changed 2 years ago by 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 :)

comment:19 Changed 2 years ago by Peter Zsoldos

Needs documentation: unset

comment:20 Changed 2 years ago by Tim Graham

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 Changed 2 years ago by Peter Zsoldos

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 Changed 2 years ago by Tim Graham <timograham@…>

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