New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added SSL for MySQL database connections #2566

Merged
merged 1 commit into from Oct 12, 2013

Conversation

Projects
None yet
9 participants
@louis89
Contributor

louis89 commented Apr 8, 2013

I have added the ability to connect using SSL to a MySQL database.

The following options have been added to the videodatabase/musicdatabase sections of the advanced settings XML file.

    <key></key>
    <cert></cert>
    <ca></ca>
    <capath></capath>
    <ciphers></ciphers>

All of these new options are completely optional and are silently ignored if defined when using SQLite. The options follow the specifications as defined for the mysql_ssl_set() function in MySQL 5.0 and later. See the following link for details http://dev.mysql.com/doc/refman/5.0/en/mysql-ssl-set.html .

I have tested this update on Windows 8 x64 and Fedora 17 x64. I found no problems.

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Apr 9, 2013

@jmarshallnz
do we really want to add more support for mysql?
if we are eventually hoping to drop it i thinks it's bad idea to extend it now

@louis89

This comment has been minimized.

Contributor

louis89 commented Apr 9, 2013

Oh yeah... You're right! Unfortunately it took some time searching to find any mention by an XBMC team member that this is the plan. Specifically, I found your posts about it buried in this thread http://forum.xbmc.org/showthread.php?tid=149489.

It probably would be a good idea to add a note about this to the wiki sections that discuss MySQL...

I trust in the XBMC team that a better solution than MySQL will arrive, eventually, but in the meantime I need SSL working in my setup. The code I have added here isn't very complicated and I am a little surprised it wasn't part of the initial MySQL integration with XBMC. I guess I'll have to merge this code into future updates and build XBMC myself, at least until I can find a better solution.

@akva2

This comment has been minimized.

Contributor

akva2 commented Apr 9, 2013

i really don't see a reason to refuse this in the mean time.

@Memphiz

This comment has been minimized.

Member

Memphiz commented Apr 9, 2013

Me neither. I'm fine with it. (looks straight forward). But i think the initialisation in sqlitedataset and mysqldataset is doubled by the c'tor of the base class and therefore not needed. (i bet someone will correct me on that me is on coffee!)

@louis89

This comment has been minimized.

Contributor

louis89 commented Apr 9, 2013

I believe you may be correct, but C++ isn't my best language and there certainly are a few gotchas hidden in it. I chose to reinitialize those variables to avoid stepping on the toes of whomever made the design choice to reinitialize everything for both MySQL and SQLite. If you notice all variables are reinitialized except "sequence_table". Granted some of the variables are initialized with different values, but some are not.

@jmarshallnz

View changes

xbmc/dbwrappers/dataset.cpp Outdated
cert = "";
ca = "";
capath = "";
ciphers = "";

This comment has been minimized.

@jmarshallnz

jmarshallnz Apr 9, 2013

Member

as they're std::string's the initialisation of empty is a noop anyway.

@jmarshallnz

View changes

xbmc/dbwrappers/sqlitedataset.cpp Outdated
cert = "";
ca = "";
capath = "";
ciphers = "";

This comment has been minimized.

@jmarshallnz

jmarshallnz Apr 9, 2013

Member

block is not needed

@jmarshallnz

View changes

xbmc/dbwrappers/mysqldataset.cpp Outdated
cert = "";
ca = "";
capath = "";
ciphers = "";

This comment has been minimized.

@jmarshallnz

jmarshallnz Apr 9, 2013

Member

block is not needed

@jmarshallnz

This comment has been minimized.

Member

jmarshallnz commented Apr 9, 2013

Looks fine - just remove the initialisation of std::string's in constructors to "" that you've added.

@louis89

This comment has been minimized.

Contributor

louis89 commented Apr 15, 2013

@jmarshallnz Thanks for reviewing my code. Just wanted to mention I made your suggested changes a few days ago. I haven't used github for a large open source project before so I am not certain how or if notifications of additional commits to a pull request propagate. I got the impression from the comments that this addition was simple enough to be added even though MySQL support will eventually be dropped. Since this hasn't been merged yet I am assuming that my last commit may have gone unnoticed.

@Montellese

This comment has been minimized.

Member

Montellese commented Apr 15, 2013

There's no mail notification on additional/changed commits on a PR.

Concerning the merge: we work in a monthly cycle where PRs that add new functionality can only be merged in the first 10 days of a month. The remaining 20/21 days are for bug fix commits/changes only so this PR will have to wait till May.

@louis89

This comment has been minimized.

Contributor

louis89 commented Apr 15, 2013

@Montellese Cool! Thanks a lot for taking the time to explain the process, much appreciated!

@ghost ghost assigned jmarshallnz Apr 15, 2013

@jmarshallnz

This comment has been minimized.

Member

jmarshallnz commented Apr 15, 2013

Changes look fine now. Squash down the commits and we'll pull in May merge window.

@jenkins4xbmc

This comment has been minimized.

Contributor

jenkins4xbmc commented Jun 16, 2013

Is this PR ment to be tested by jenkins?

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Jul 23, 2013

jenkins build this please
@jmarshallnz ok for next window (seems we forgot last time)?

@Memphiz

This comment has been minimized.

Member

Memphiz commented Jul 23, 2013

This one failed on atv2 because it is not rebased to master ...

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Jul 23, 2013

also windows failed

@louis89

This comment has been minimized.

Contributor

louis89 commented Aug 24, 2013

@Memphiz, @MartijnKaijser First off sorry for taking a while to get back to this. I have been pretty busy with another project and am only just now finding some time to figure this out. As far as I can tell this is rebased onto master, but I could easily be missing something. I hadn't ever squashed a series of git commits before being asked to do so here, so it is possible I have unknowingly broken something. That being said, manually building this code for both Windows and Fedora 17 works fine for me. I am using the build scripts provided in the XBMC source. I looked over the build details for the build attempt made here and nothing caught my eye to explain what caused the build to fail, but I also have never used Jenkins before. I have installed and configured my own Jenkins to test this, but it might be a while before I have XBMC build jobs using git working. Thanks in advance for your patience.

@theuni

This comment has been minimized.

Member

theuni commented Aug 24, 2013

I can't speak for windows, but for ios/osx/android, you'll need this: theuni@9a9e64a

Otherwise, ssl isn't enabled for mysql on those platforms.

@Memphiz

This comment has been minimized.

Member

Memphiz commented Aug 24, 2013

No Need to install jenkins - it was a Long Time ago where Building pull requests was still wonky ... We try it again

jenkins build this please

@Memphiz

This comment has been minimized.

Member

Memphiz commented Aug 24, 2013

jenkins build this please

@MartijnKaijser did you abort it? If so why?

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Aug 24, 2013

nope

@louis89

This comment has been minimized.

Contributor

louis89 commented Aug 24, 2013

@theuni Good call on the with-ssl build switch, but I am surprised it works on Windows and Linux without that switch. For the sake of my sanity I confirmed that XBMC is denied access to my database without the options I added in this PR and that I am not using the with-ssl switch.

@Memphiz @MartijnKaijser I'm not sure if you've had a chance to look at the Jenkins build log, but it looks like everything worked except OSX which failed with the message "Unable to look up github.com (port 9418) (nodename nor servname provided, or not known)". Seems odd, but possible. I'm just wondering if there is any chance another error triggered this one? If it's a problem with my PR I will be glad to fix it.

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Aug 24, 2013

jenkins build this please
@Memphiz
davilla really needs to look at his build slave

@davilla

This comment has been minimized.

Contributor

davilla commented Aug 24, 2013

The real cause is my net is slow and if I I have any other downloads running, results from jenkins might be unreliable. There's not much that can be done without paying several 100k dollars to trench in fiber :)

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Sep 19, 2013

is this ok for next merge window?

@louis89

This comment has been minimized.

Contributor

louis89 commented Oct 9, 2013

Any chance this will make the October merge window?

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Oct 12, 2013

jenkins build this please

jmarshallnz added a commit that referenced this pull request Oct 12, 2013

Merge pull request #2566 from louis89/master
Added SSL for MySQL database connections

@jmarshallnz jmarshallnz merged commit ad4ea1d into xbmc:master Oct 12, 2013

1 check passed

default Merged build #342 succeeded in 41 min
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment