[PATCH 0 of 1 stable RFC] Re: url: add --insecure option to bypass verification of ssl certificates
Yuya Nishihara
yuya at tcha.org
Fri Jan 28 15:27:57 UTC 2011
Mads Kiilerich wrote:
> On 01/26/2011 05:47 PM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara<yuya at tcha.org>
> > # Date 1296060076 -32400
> > # Branch stable
> > # Node ID 2d52545f902469ba76524af647bec3ce2a09f5b1
> > # Parent d0e0d3d43e1439d63564ab4dddfe0daa69ae2d86
> > url: add --insecure option to bypass verification of ssl certificates
>
> I assume this is a response to
> http://www.selenic.com/pipermail/mercurial-devel/2011-January/027367.html
>
> > If --insecure specified, it behaves in the same way as no web.cacerts
> > configured, except for the error message.
> >
> > Also shows hint for --insecure option when verification failed.
>
> > diff --git a/mercurial/commands.py b/mercurial/commands.py
> > --- a/mercurial/commands.py
> > +++ b/mercurial/commands.py
> > @@ -3943,6 +3943,8 @@ remoteopts = [
> > _('specify ssh command to use'), _('CMD')),
> > ('', 'remotecmd', '',
> > _('specify hg command to run on the remote side'), _('CMD')),
> > + ('', 'insecure', None,
> > + _('do not verify server certificate')),
>
> should "ignoring web.cacerts configuration" be added?
>
> > ]
> >
> > walkopts = [
> > diff --git a/mercurial/hg.py b/mercurial/hg.py
> > --- a/mercurial/hg.py
> > +++ b/mercurial/hg.py
> > @@ -545,6 +545,10 @@ def remoteui(src, opts):
> > if r:
> > dst.setconfig('bundle', 'mainreporoot', r)
> >
> > + # copy ssl-specific options
> > + if opts.get('insecure'):
> > + dst.setconfig('web', 'insecure', 'True')
>
> Hmm. This seems like a hackish way to transfer command line options down
> to url. It extends the existing ssh hack a bit. Wouldn't it be cleaner
> to do it in dispatch._dispatch together with the real global options?
>
> Note that _if_ we want to support it as a config setting then we would
> have to copy the value from src too in order to support subrepos.
>
> I think the config name should be internal and more obscure than
> web.insecure. If not then it should be documented.
>
> But: Wouldn't it be simpler to implement and explain the functionality
> if it just cleared web.cacerts?
Okay, I changed --insecure to act just like --config web.cacerts= and
add "(ignoring web.cacerts config)" to help message.
It looks much simpler now.
> > # copy selected local settings to the remote ui
> > for sect in ('auth', 'http_proxy'):
> > for key, val in src.configitems(sect):
> > diff --git a/mercurial/url.py b/mercurial/url.py
> > --- a/mercurial/url.py
> > +++ b/mercurial/url.py
> > @@ -530,24 +530,31 @@ if has_https:
> > cacerts = self.ui.config('web', 'cacerts')
> > if cacerts:
> > cacerts = util.expandpath(cacerts)
> > + insecure = self.ui.configbool('web', 'insecure')
> > else:
> > cacerts = None
> > + insecure = False
> >
> > - if cacerts:
> > + if cacerts and not insecure:
> > sock = _create_connection((self.host, self.port))
> > self.sock = _ssl_wrap_socket(sock, self.key_file,
> > self.cert_file, cert_reqs=CERT_REQUIRED,
> > ca_certs=cacerts)
> > msg = _verifycert(self.sock.getpeercert(), self.host)
> > if msg:
> > - raise util.Abort(_('%s certificate error: %s') %
> > - (self.host, msg))
> > + raise util.Abort(_('%s certificate error: %s '
> > + '(use --insecure to connect '
> > + 'insecurely)') % (self.host, msg))
> > self.ui.debug('%s certificate successfully verified\n' %
> > self.host)
> > else:
> > - self.ui.warn(_("warning: %s certificate not verified "
> > - "(check web.cacerts config setting)\n") %
> > - self.host)
> > + if insecure:
> > + self.ui.warn(_("warning: %s certificate not verified\n") %
> > + self.host)
>
> We could be a bit more specific and say something like
> "certificate verification disabled" - and perhaps also "with
> --insecure". (But I don't think that is important enough to justify
> doing anything more complex than clearing cacerts.)
>
> I guess some users would like to see an option for insecure without
> warning. I think it is fine to say no and not have that option.
>
> > + else:
> > + self.ui.warn(_("warning: %s certificate not verified "
> > + "(check web.cacerts config setting)\n") %
>
> Should --insecure be mentioned here too? Or would that be too verbose?
Something like "(check web.cacerts config setting or --insecure command
line option)" ?
> > + self.host)
> > httplib.HTTPSConnection.connect(self)
> >
> > class httpsconnection(BetterHTTPS):
> > diff --git a/tests/test-debugcomplete.t b/tests/test-debugcomplete.t
> > --- a/tests/test-debugcomplete.t
> > +++ b/tests/test-debugcomplete.t
> > @@ -179,16 +179,16 @@ Show all commands + options
> > $ hg debugcommands
> > add: include, exclude, subrepos, dry-run
> > annotate: rev, follow, no-follow, text, user, file, date, number, changeset, line-number, include, exclude
> > - clone: noupdate, updaterev, rev, branch, pull, uncompressed, ssh, remotecmd
> > + clone: noupdate, updaterev, rev, branch, pull, uncompressed, ssh, remotecmd, insecure
> > commit: addremove, close-branch, include, exclude, message, logfile, date, user
> > diff: rev, change, text, git, nodates, show-function, reverse, ignore-all-space, ignore-space-change, ignore-blank-lines, unified, stat, include, exclude, subrepos
> > export: output, switch-parent, rev, text, git, nodates
> > forget: include, exclude
> > - init: ssh, remotecmd
> > + init: ssh, remotecmd, insecure
> > log: follow, follow-first, date, copies, keyword, rev, removed, only-merges, user, only-branch, branch, prune, patch, git, limit, no-merges, stat, style, template, include, exclude
> > merge: force, tool, rev, preview
> > - pull: update, force, rev, branch, ssh, remotecmd
> > - push: force, rev, branch, new-branch, ssh, remotecmd
> > + pull: update, force, rev, branch, ssh, remotecmd, insecure
> > + push: force, rev, branch, new-branch, ssh, remotecmd, insecure
> > remove: after, force, include, exclude
> > serve: accesslog, daemon, daemon-pipefds, errorlog, port, address, prefix, name, web-conf, webdir-conf, pid-file, stdio, templates, style, ipv6, certificate
> > status: all, modified, added, removed, deleted, clean, unknown, ignored, no-status, copies, print0, rev, change, include, exclude, subrepos
> > @@ -200,7 +200,7 @@ Show all commands + options
> > bisect: reset, good, bad, skip, command, noupdate
> > branch: force, clean
> > branches: active, closed
> > - bundle: force, rev, branch, base, all, type, ssh, remotecmd
> > + bundle: force, rev, branch, base, all, type, ssh, remotecmd, insecure
> > cat: output, rev, decode, include, exclude
> > copy: after, force, include, exclude, dry-run
> > debugancestor:
> > @@ -228,10 +228,10 @@ Show all commands + options
> > help:
> > identify: rev, num, id, branch, tags
> > import: strip, base, force, no-commit, exact, import-branch, message, logfile, date, user, similarity
> > - incoming: force, newest-first, bundle, rev, branch, patch, git, limit, no-merges, stat, style, template, ssh, remotecmd, subrepos
> > + incoming: force, newest-first, bundle, rev, branch, patch, git, limit, no-merges, stat, style, template, ssh, remotecmd, insecure, subrepos
> > locate: rev, print0, fullpath, include, exclude
> > manifest: rev
> > - outgoing: force, rev, newest-first, branch, patch, git, limit, no-merges, stat, style, template, ssh, remotecmd, subrepos
> > + outgoing: force, rev, newest-first, branch, patch, git, limit, no-merges, stat, style, template, ssh, remotecmd, insecure, subrepos
> > parents: rev, style, template
> > paths:
> > recover:
> > diff --git a/tests/test-https.t b/tests/test-https.t
> > --- a/tests/test-https.t
> > +++ b/tests/test-https.t
> > @@ -167,7 +167,7 @@ variables in the filename
> > cacert mismatch
> >
> > $ hg -R copy-pull pull --config web.cacerts=pub.pem https://127.0.0.1:$HGPORT/
> > - abort: 127.0.0.1 certificate error: certificate is for localhost
> > + abort: 127.0.0.1 certificate error: certificate is for localhost (use --insecure to connect insecurely)
> > [255]
> > $ hg -R copy-pull pull --config web.cacerts=pub-other.pem
> > abort: error: *:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed (glob)
> > @@ -188,3 +188,26 @@ Test server cert which no longer is vali
> > $ hg -R copy-pull pull --config web.cacerts=pub-expired.pem https://localhost:$HGPORT2/
> > abort: error: *:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed (glob)
>
> If we want to give hints about --insecure then it would be very very
> nice to get in this case too.
Hmm, it should be.
Where's the good place to catch SSLError? Sadly this error is handled at
dispatch.py:_runcatch(). I tried a quick hack, but yes, it looks awful:
--- a/mercurial/url.py
+++ b/mercurial/url.py
@@ -306,8 +306,21 @@ if has_https:
try:
# avoid using deprecated/broken FakeSocket in python 2.6
import ssl
- _ssl_wrap_socket = ssl.wrap_socket
+ _ssl_wrap_socket_orig = ssl.wrap_socket
CERT_REQUIRED = ssl.CERT_REQUIRED
+ SSLError = ssl.SSLError
+
+ def _ssl_wrap_socket(sock, key_file, cert_file,
+ cert_reqs=CERT_REQUIRED, ca_certs=None):
+ try:
+ return _ssl_wrap_socket_orig(sock, key_file, cert_file,
+ cert_reqs=cert_reqs,
+ ca_certs=ca_certs)
+ except SSLError, e:
+ raise SSLError(e.args[0],
+ e.args[1] + ' '
+ + _('(use --insecure to connect insecurely'))
+
> > [255]
> > +
> > +cacert match but pull insecurely
>
> The logic in the tests could perhaps be easier to follow if these tests
> came immediately after the corresponding secure test has failed.
Indeed. Moved it after "cacert mismatch" test.
> > +
> > + $ P=`pwd` hg -R copy-pull pull --insecure
>
> What is this P?
>
> > + warning: localhost certificate not verified
> > + pulling from https://localhost:$HGPORT/
> > + searching for changes
> > + no changes found
> > +
> > +cacert mismatch but pull insecurely
> > +
> > + $ hg -R copy-pull pull --config web.cacerts=pub.pem --insecure \
> > +> https://127.0.0.1:$HGPORT/
> > + warning: 127.0.0.1 certificate not verified
> > + pulling from https://127.0.0.1:$HGPORT/
> > + searching for changes
> > + no changes found
> > + $ hg -R copy-pull pull --config web.cacerts=pub-other.pem --insecure
> > + warning: localhost certificate not verified
> > + pulling from https://localhost:$HGPORT/
> > + searching for changes
> > + no changes found
> > +
>
>
> Finally:
>
> The --insecure option should perhaps also be mentioned in hgrc.5.txt
> when web.cacerts is described.
Is this looks good?
--- a/doc/hgrc.5.txt
+++ b/doc/hgrc.5.txt
@@ -1030,6 +1030,9 @@ The full set of options is:
You can use OpenSSL's CA certificate file if your platform has one.
On most Linux systems this will be ``/etc/ssl/certs/ca-certificates.crt``.
Otherwise you will have to generate this file manually.
+
+ To disable SSL verification temporarily, specify ``--insecure`` from
+ command line.
``contact``
Name or email address of the person in charge of the repository.
Defaults to ui.username or ``$EMAIL`` or "unknown" if unset or empty.
Regards,
Yuya
More information about the Mercurial-devel
mailing list