Skip to content
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

Add HTTPS support #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add HTTPS support #14

wants to merge 1 commit into from

Conversation

mineo
Copy link
Member

@mineo mineo commented Jan 28, 2017

See https://tickets.metabrainz.org/browse/MBH-363 - mb.org is at some
point going to move to HTTPS (even for the webservice). Without calling
ne_ssl_trust_default_ca, connections to caa.org fail because the
certificate issuer is not trusted.

With this patch, ne_ssl_trust_default_ca is called whenever the scheme
used to create the ne_session object is "https". This is the case if
the port used is 443.

Unconditionally calling ne_ssl_trust_default_ca does not work because
its implementation relies on an ssl context being set up on the session,
which is only done by libneon if the scheme is "https". If
ne_ssl_trust_default_ca is called on an ne_session without an ssl
context, a segfault happens.

The condition is hardcoded to port 443 at the moment because I can't
test anything else. Also, a quick look at ne_session_create showed
that it internally sets a port to 443 as well, so I can't be sure that
it would really work with ports other than 443.

Fixes https://tickets.metabrainz.org/browse/LMB-47

See https://tickets.metabrainz.org/browse/MBH-363 - mb.org is at some
point going to move to HTTPS (even for the webservice). Without calling
`ne_ssl_trust_default_ca`, connections to caa.org fail because the
certificate issuer is not trusted.

With this patch, `ne_ssl_trust_default_ca` is called whenever the scheme
used to create the `ne_session` object is "https". This is the case if
the port used is 443.

Unconditionally calling `ne_ssl_trust_default_ca` does not work because
its implementation relies on an ssl context being set up on the session,
which is only done by libneon if the scheme is "https". If
`ne_ssl_trust_default_ca` is called on an `ne_session` without an ssl
context, a segfault happens.

The condition is hardcoded to port 443 at the moment because I can't
test anything else. Also, a quick look at `ne_session_create` showed
that it internally sets a port to 443 as well, so I can't be sure that
it would really work with ports other than 443.
@mineo
Copy link
Member Author

mineo commented Jan 28, 2017

Oh, one thing to note it that since the check is hardcoded to 443, I doubt this will work for redirects from HTTP to HTTPS, but I have to admit that I'm not really sure what to do about this. We can change the default port to 443 now and hope that all linux distributions and other people that package libmb have changed to a new version of libmb by the time the HTTPS switch is made, but I can't judge how realistic that is/which timeframe that would imply for the HTTPS switch on the MB side.

@Freso
Copy link
Member

Freso commented Aug 11, 2017

The sooner we get it in, the better the chance of it being available in all major distributions by the time of the switch. Considering how we may also be making a ws/2.5 soon and may be starting Real™ talks of ws/3 in the coming year or two, maybe HTTP for ws/2 will be kept around and future WS versions may be HTTPS-only.

@adhawkins
Copy link
Member

Is the call to ne_ssl_trust_default_ca necessary in libmusicbrainz? The musicbrainz.org servers use valid certificates don't they?

Perhaps it should be a configuration property for both (whether or not to trust 'invalid' certificates)?

@mineo
Copy link
Member Author

mineo commented Aug 11, 2017

This question has been answered on IRC already, but if anyone else is interested: without calling ne_ssl_trust_default_ca, libneon (or its SSL backend) doesn't trust any certificate, so the connection to MB will not be established correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants