Opened 2 years ago

Closed 22 months ago

#22646 closed Cleanup/optimization (fixed)

MySQL dbshell command should respect the encryption flag

Reported by: zsoldosp Owned by: zsoldosp
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: danielquinn, denis.cornehl@…, zsoldosp Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


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

Change History (22)

comment:1 Changed 2 years ago by zsoldosp

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 2 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 2 years ago by syphar

  • Cc denis.cornehl@… added
  • Component changed from Uncategorized to Database layer (models, ORM)
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

comment:4 Changed 2 years ago by syphar

  • Triage Stage changed from Accepted to 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 Changed 2 years ago by erikr

  • Needs documentation set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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 Changed 2 years ago by syphar

In the doc you provided, there is an parameter ssl which translates the given dict to, 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 2 years ago 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 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 2 years ago 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')

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

comment:11 Changed 2 years ago by erikr

  • 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 2 years ago 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 under "minor changes"
  2. git commit --amend
  3. git push --force

comment:13 Changed 2 years ago by zsoldosp

  • Cc zsoldosp added

comment:14 follow-up: Changed 2 years ago by erikr

Yes zsoldosp, that should be it :)

comment:15 Changed 2 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 2 years ago by timo

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 2 years ago by zsoldosp

@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 22 months ago by zsoldosp

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 22 months ago by zsoldosp

  • Needs documentation unset

comment:20 Changed 22 months ago by timgraham

  • 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 22 months ago by zsoldosp

  • 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 22 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

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