Issue 4425052: Adds optional support for SSL server certificate validation
|
Created:
13 years, 11 months ago by xtof.g Modified:
13 years, 11 months ago Reviewers:
Mike Schwartz, mitch.garnaat Visibility:
Public. |
Patch Set 1 #
Total comments: 24
Patch Set 2 : Address Mike's code review feedback. #
Total comments: 5
Patch Set 3 : Address Mike's 2nd round of feedback. #Patch Set 4 : Formatting cleanup #
MessagesTotal messages: 12
I suggest renaming this issue to "Adds optional support for SSL server certificate validation", to make it clearer in the change history that server cert validation didn't suddenly start being supported at this point. This comment doesn't matter for the Rietveld review but likely we'll commit @ github with this same description, and it does matter there. Is there somewhere I can get the files from this CL, so I can try running the tests myself? (I didn't see a way to download easily from Rietveld.) Thanks http://codereview.appspot.com/4425052/diff/1/boto/connection.py File boto/connection.py (right): http://codereview.appspot.com/4425052/diff/1/boto/connection.py#newcode72 boto/connection.py:72: 'No ssl module found. Server certificate validation is disabled') Another possible approach to consider would be to define a boto config flag to give the user control over whether cert validation is required - and not try to recover from this import error if they set the flag to true. That way the user is explicitly configuing "I don't care about this vulnerability" rather than the code falling back on the less secure mode. http://codereview.appspot.com/4425052/diff/1/boto/connection.py#newcode191 boto/connection.py:191: # Whether or not to validate server certificates. At some point in the Dr. Hanson would contest your excessive use of whitespace :-) http://codereview.appspot.com/4425052/diff/1/boto/connection.py#newcode379 boto/connection.py:379: connection = https_connection.CertValidatingHTTPSConnection(host, 80 char wrap http://codereview.appspot.com/4425052/diff/1/boto/connection.py#newcode451 boto/connection.py:451: # Wrap the socket in an SSL socket You also wrap in the other branch of this 'if' statement, so consider either adding the same comment there too or maybe just remove this comment (it's pretty obvious from the code) http://codereview.appspot.com/4425052/diff/1/boto/https_connection.py File boto/https_connection.py (right): http://codereview.appspot.com/4425052/diff/1/boto/https_connection.py#newcode17 boto/https_connection.py:17: # http://code.google.com/p/googleappengine/source/browse/trunk/python/google/ap... I get a 404 when I try this URL http://codereview.appspot.com/4425052/diff/1/boto/https_connection.py#newcode30 boto/https_connection.py:30: class InvalidCertificateException(httplib.HTTPException): Most of the rest of the boto code puts the exceptions in boto/boto/exception.py I guess since it's inconsistently done, it's your call. http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... File tests/s3/test_https_cert_validation.py (right): http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:1: # Copyright 2010 Google Inc. 2011 http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:31: does not match the CN in that certificate. By default, this test usees host uses http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:35: address for, say. www.google.com or www.amazon.com to /etc/hosts. say, http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:47: # Other CA certs contains a valid CA certificate of a CA that is used by neither Do you mean "other_cacerts.txt" ? If so it would be easier to parse this sentence if you use that instead. My grammar eyeball is trying to figure out whether the current sentence should be "certs contain" vs "cert contains"... http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:57: # This test assumes that this host returs a certificate signed by one of the returns http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:59: # the server should return a certificate with CN 'www.<somedomain>.com'. missing close paren http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:69: # Enable https_validate_certificates (which is not the default yet). any reason to include this parenthetical? Even after it's the default I'd think you still want this setup, to make the test not be brittle.
Sign in to reply to this message.
On 2011/04/19 23:49:13, Mike Schwartz wrote: > I suggest renaming this issue to "Adds optional support for SSL server > certificate validation", to make it clearer in the change history that server > cert validation didn't suddenly start being supported at this point. This > comment doesn't matter for the Rietveld review but likely we'll commit @ github > with this same description, and it does matter there. Done. > > Is there somewhere I can get the files from this CL, so I can try running the > tests myself? (I didn't see a way to download easily from Rietveld.) > There's a "Download raw patch set" link in the upper right?
Sign in to reply to this message.
Thanks for your feedback, PTAL. http://codereview.appspot.com/4425052/diff/1/boto/connection.py File boto/connection.py (right): http://codereview.appspot.com/4425052/diff/1/boto/connection.py#newcode72 boto/connection.py:72: 'No ssl module found. Server certificate validation is disabled') On 2011/04/19 23:49:13, Mike Schwartz wrote: > Another possible approach to consider would be to define a boto config flag to > give the user control over whether cert validation is required - and not try to > recover from this import error if they set the flag to true. That way the user > is explicitly configuing "I don't care about this vulnerability" rather than the > code falling back on the less secure mode. Done. http://codereview.appspot.com/4425052/diff/1/boto/connection.py#newcode191 boto/connection.py:191: # Whether or not to validate server certificates. At some point in the On 2011/04/19 23:49:13, Mike Schwartz wrote: > Dr. Hanson would contest your excessive use of whitespace :-) Heh, blame it on vim. http://codereview.appspot.com/4425052/diff/1/boto/connection.py#newcode379 boto/connection.py:379: connection = https_connection.CertValidatingHTTPSConnection(host, On 2011/04/19 23:49:13, Mike Schwartz wrote: > 80 char wrap Done. http://codereview.appspot.com/4425052/diff/1/boto/connection.py#newcode451 boto/connection.py:451: # Wrap the socket in an SSL socket On 2011/04/19 23:49:13, Mike Schwartz wrote: > You also wrap in the other branch of this 'if' statement, so consider either > adding the same comment there too or maybe just remove this comment (it's pretty > obvious from the code) Done. http://codereview.appspot.com/4425052/diff/1/boto/https_connection.py File boto/https_connection.py (right): http://codereview.appspot.com/4425052/diff/1/boto/https_connection.py#newcode30 boto/https_connection.py:30: class InvalidCertificateException(httplib.HTTPException): On 2011/04/19 23:49:13, Mike Schwartz wrote: > Most of the rest of the boto code puts the exceptions in boto/boto/exception.py > > I guess since it's inconsistently done, it's your call. Well, this one is a subclass of a httplib exception, so I'm not sure it belongs into boto/exception.py. http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... File tests/s3/test_https_cert_validation.py (right): http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:1: # Copyright 2010 Google Inc. On 2011/04/19 23:49:13, Mike Schwartz wrote: > 2011 Done. http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:31: does not match the CN in that certificate. By default, this test usees host On 2011/04/19 23:49:13, Mike Schwartz wrote: > uses Done. http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:47: # Other CA certs contains a valid CA certificate of a CA that is used by neither On 2011/04/19 23:49:13, Mike Schwartz wrote: > Do you mean "other_cacerts.txt" ? > If so it would be easier to parse this sentence if you use that instead. My > grammar eyeball is trying to figure out whether the current sentence should be > "certs contain" vs "cert contains"... Thanks, fixed. http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:57: # This test assumes that this host returs a certificate signed by one of the On 2011/04/19 23:49:13, Mike Schwartz wrote: > returns Thanks... Sorry -- I should have spell checked... http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:59: # the server should return a certificate with CN 'www.<somedomain>.com'. On 2011/04/19 23:49:13, Mike Schwartz wrote: > missing close paren Done. http://codereview.appspot.com/4425052/diff/1/tests/s3/test_https_cert_validat... tests/s3/test_https_cert_validation.py:69: # Enable https_validate_certificates (which is not the default yet). On 2011/04/19 23:49:13, Mike Schwartz wrote: > any reason to include this parenthetical? > Even after it's the default I'd think you still want this setup, to make the > test not be brittle. Good point, removed.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/4425052/diff/7001/tests/s3/test_https_cert_vali... File tests/s3/test_https_cert_validation.py (right): http://codereview.appspot.com/4425052/diff/7001/tests/s3/test_https_cert_vali... tests/s3/test_https_cert_validation.py:47: # File 'other_cacerts.txt' contains a valid CA certificate of a CA that is used blank at EOL http://codereview.appspot.com/4425052/diff/7001/tests/s3/test_https_cert_vali... tests/s3/test_https_cert_validation.py:48: # by neither S3 nor Google Storage. Validation against this CA cert should blank at EOL
Sign in to reply to this message.
http://codereview.appspot.com/4425052/diff/7001/boto/connection.py File boto/connection.py (right): http://codereview.appspot.com/4425052/diff/7001/boto/connection.py#newcode77 boto/connection.py:77: os.path.dirname(os.path.abspath(cacerts.__file__ )), "cacerts.txt") I wish I could think of a better way of doing this but I really can't. Python doesn't provide any standard mechanisms for accessing data files, AFAIK. This is a clever hack but it's still a hack. Is there any precedence for this in other packages?
Sign in to reply to this message.
Hi Mitch, thanks for taking a look! On Thu, Apr 21, 2011 at 08:38, <Mitch.Garnaat@gmail.com> wrote: > > http://codereview.appspot.com/4425052/diff/7001/boto/connection.py > > File boto/connection.py (right): > > > http://codereview.appspot.com/4425052/diff/7001/boto/connection.py#newcode77 > boto/connection.py:77: os.path.dirname(os.path.abspath(cacerts.__file__ > )), "cacerts.txt") > I wish I could think of a better way of doing this but I really can't. > Python doesn't provide any standard mechanisms for accessing data files, > AFAIK. This is a clever hack but it's still a hack. Is there any > precedence for this in other packages? I think it's pretty common to use __file__ to find the directory a module came from. The trick of making the directory with the cacerts file an otherwise empty module is something I thought of; I'm not sure I've seen this before. I suppose there are lots of things in python that one could call both "elegant" and "a terrible hack" at the same time :) I could get rid of that cacerts package and reference the file relative to the boto root directory, but I don't know of any other way besides __file__ to find out where the boto package is installed. > > > http://codereview.appspot.com/4425052/ >
Sign in to reply to this message.
On 2011/04/21 16:36:53, xtof.g wrote: > Hi Mitch, > > thanks for taking a look! > > On Thu, Apr 21, 2011 at 08:38, <mailto:Mitch.Garnaat@gmail.com> wrote: > > > > > http://codereview.appspot.com/4425052/diff/7001/boto/connection.py > > > > File boto/connection.py (right): > > > > > > http://codereview.appspot.com/4425052/diff/7001/boto/connection.py#newcode77 > > boto/connection.py:77: os.path.dirname(os.path.abspath(cacerts.__file__ > > )), "cacerts.txt") > > I wish I could think of a better way of doing this but I really can't. > > Python doesn't provide any standard mechanisms for accessing data files, > > AFAIK. This is a clever hack but it's still a hack. Is there any > > precedence for this in other packages? > > I think it's pretty common to use __file__ to find the directory a module > came from. The trick of making the directory with the cacerts file an > otherwise empty module is something I thought of; I'm not sure I've seen > this before. > > I suppose there are lots of things in python that one could call both > "elegant" and "a terrible hack" at the same time :) > I didn't call it a "terrible hack", just a hack. And a clever one at that. If there was some nice, clean, standard mechanism for doing this then I would argue for using it but there isn't so this seems as good a solution as any and better than most. > I could get rid of that cacerts package and reference the file relative to > the boto root directory, but I don't know of any other way besides __file__ > to find out where the boto package is installed. > > > > > > > > > http://codereview.appspot.com/4425052/ > >
Sign in to reply to this message.
On Thu, Apr 21, 2011 at 09:45, <Mitch.Garnaat@gmail.com> wrote: > On 2011/04/21 16:36:53, xtof.g wrote: > >> Hi Mitch, >> > > thanks for taking a look! >> > > On Thu, Apr 21, 2011 at 08:38, <mailto:Mitch.Garnaat@gmail.com> wrote: >> > > > >> > http://codereview.appspot.com/4425052/diff/7001/boto/connection.py >> > >> > File boto/connection.py (right): >> > >> > >> > >> > > http://codereview.appspot.com/4425052/diff/7001/boto/connection.py#newcode77 > >> > boto/connection.py:77: >> > os.path.dirname(os.path.abspath(cacerts.__file__ > >> > )), "cacerts.txt") >> > I wish I could think of a better way of doing this but I really >> > can't. > >> > Python doesn't provide any standard mechanisms for accessing data >> > files, > >> > AFAIK. This is a clever hack but it's still a hack. Is there any >> > precedence for this in other packages? >> > > I think it's pretty common to use __file__ to find the directory a >> > module > >> came from. The trick of making the directory with the cacerts file an >> otherwise empty module is something I thought of; I'm not sure I've >> > seen > >> this before. >> > > I suppose there are lots of things in python that one could call both >> "elegant" and "a terrible hack" at the same time :) >> > > > I didn't call it a "terrible hack", just a hack. And a clever one at > that. If there was some nice, clean, standard mechanism for doing this > then I would argue for using it but there isn't so this seems as good a > solution as any and better than most. > > Thanks, I will stick with it then. Just be clear, I certainly was not in any way offended by this being called a hack :) cheers --xtof > > I could get rid of that cacerts package and reference the file >> > relative to > >> the boto root directory, but I don't know of any other way besides >> > __file__ > >> to find out where the boto package is installed. >> > > > > > >> > >> > http://codereview.appspot.com/4425052/ >> > >> > > > > http://codereview.appspot.com/4425052/ >
Sign in to reply to this message.
Thanks for looking over the code Mitch. Ok if I commit this to boto later today/tonight? Mike On Thu, Apr 21, 2011 at 9:45 AM, <Mitch.Garnaat@gmail.com> wrote: > On 2011/04/21 16:36:53, xtof.g wrote: > >> Hi Mitch, >> > > thanks for taking a look! >> > > On Thu, Apr 21, 2011 at 08:38, <mailto:Mitch.Garnaat@gmail.com> wrote: >> > > > >> > http://codereview.appspot.com/4425052/diff/7001/boto/connection.py >> > >> > File boto/connection.py (right): >> > >> > >> > >> > > http://codereview.appspot.com/4425052/diff/7001/boto/connection.py#newcode77 > >> > boto/connection.py:77: >> > os.path.dirname(os.path.abspath(cacerts.__file__ > >> > )), "cacerts.txt") >> > I wish I could think of a better way of doing this but I really >> > can't. > >> > Python doesn't provide any standard mechanisms for accessing data >> > files, > >> > AFAIK. This is a clever hack but it's still a hack. Is there any >> > precedence for this in other packages? >> > > I think it's pretty common to use __file__ to find the directory a >> > module > >> came from. The trick of making the directory with the cacerts file an >> otherwise empty module is something I thought of; I'm not sure I've >> > seen > >> this before. >> > > I suppose there are lots of things in python that one could call both >> "elegant" and "a terrible hack" at the same time :) >> > > > I didn't call it a "terrible hack", just a hack. And a clever one at > that. If there was some nice, clean, standard mechanism for doing this > then I would argue for using it but there isn't so this seems as good a > solution as any and better than most. > > > I could get rid of that cacerts package and reference the file >> > relative to > >> the boto root directory, but I don't know of any other way besides >> > __file__ > >> to find out where the boto package is installed. >> > > > > > >> > >> > http://codereview.appspot.com/4425052/ >> > >> > > > > http://codereview.appspot.com/4425052/ >
Sign in to reply to this message.
Yes, please do! Mitch On Thu, Apr 21, 2011 at 1:24 PM, Michael Schwartz <mfschwartz@google.com>wrote: > Thanks for looking over the code Mitch. > Ok if I commit this to boto later today/tonight? > > Mike > > > On Thu, Apr 21, 2011 at 9:45 AM, <Mitch.Garnaat@gmail.com> wrote: > >> On 2011/04/21 16:36:53, xtof.g wrote: >> >>> Hi Mitch, >>> >> >> thanks for taking a look! >>> >> >> On Thu, Apr 21, 2011 at 08:38, <mailto:Mitch.Garnaat@gmail.com> wrote: >>> >> >> > >>> > http://codereview.appspot.com/4425052/diff/7001/boto/connection.py >>> > >>> > File boto/connection.py (right): >>> > >>> > >>> > >>> >> >> http://codereview.appspot.com/4425052/diff/7001/boto/connection.py#newcode77 >> >>> > boto/connection.py:77: >>> >> os.path.dirname(os.path.abspath(cacerts.__file__ >> >>> > )), "cacerts.txt") >>> > I wish I could think of a better way of doing this but I really >>> >> can't. >> >>> > Python doesn't provide any standard mechanisms for accessing data >>> >> files, >> >>> > AFAIK. This is a clever hack but it's still a hack. Is there any >>> > precedence for this in other packages? >>> >> >> I think it's pretty common to use __file__ to find the directory a >>> >> module >> >>> came from. The trick of making the directory with the cacerts file an >>> otherwise empty module is something I thought of; I'm not sure I've >>> >> seen >> >>> this before. >>> >> >> I suppose there are lots of things in python that one could call both >>> "elegant" and "a terrible hack" at the same time :) >>> >> >> >> I didn't call it a "terrible hack", just a hack. And a clever one at >> that. If there was some nice, clean, standard mechanism for doing this >> then I would argue for using it but there isn't so this seems as good a >> solution as any and better than most. >> >> >> I could get rid of that cacerts package and reference the file >>> >> relative to >> >>> the boto root directory, but I don't know of any other way besides >>> >> __file__ >> >>> to find out where the boto package is installed. >>> >> >> >> >> > >>> > >>> > http://codereview.appspot.com/4425052/ >>> > >>> >> >> >> >> http://codereview.appspot.com/4425052/ >> > >
Sign in to reply to this message.
http://codereview.appspot.com/4425052/diff/7001/tests/s3/test_https_cert_vali... File tests/s3/test_https_cert_validation.py (right): http://codereview.appspot.com/4425052/diff/7001/tests/s3/test_https_cert_vali... tests/s3/test_https_cert_validation.py:47: # File 'other_cacerts.txt' contains a valid CA certificate of a CA that is used On 2011/04/20 17:04:10, Mike Schwartz wrote: > blank at EOL Done. http://codereview.appspot.com/4425052/diff/7001/tests/s3/test_https_cert_vali... tests/s3/test_https_cert_validation.py:48: # by neither S3 nor Google Storage. Validation against this CA cert should On 2011/04/20 17:04:10, Mike Schwartz wrote: > blank at EOL Done.
Sign in to reply to this message.
Hi Mitch, I just committed<https://github.com/boto/boto/commit/ad0f4aa82a8f30522b0ce3e40e40f145080bf955>Christoph's changes. I suggest announcing this change on the boto mailing list, noting that: - this change addresses a man-in-the-middle vulnerability of the Python httplib. (I think that vulnerability is fixed in one of the more recent 3.x libraries.) - by default cert validation is off - it can be enabled by adding "https_validate_certificates = True" to the [Boto] section of the boto config file - enabling cert validation may cause your app to fail if you were dependent on invalid certs - at some point we hope to make cert validation the default for HTTPS connections Thanks, Mike On Thu, Apr 21, 2011 at 10:41 AM, Mitchell Garnaat <mitch.garnaat@gmail.com>wrote: > Yes, please do! > > Mitch > > > On Thu, Apr 21, 2011 at 1:24 PM, Michael Schwartz <mfschwartz@google.com>wrote: > >> Thanks for looking over the code Mitch. >> Ok if I commit this to boto later today/tonight? >> >> Mike >> >> >> On Thu, Apr 21, 2011 at 9:45 AM, <Mitch.Garnaat@gmail.com> wrote: >> >>> On 2011/04/21 16:36:53, xtof.g wrote: >>> >>>> Hi Mitch, >>>> >>> >>> thanks for taking a look! >>>> >>> >>> On Thu, Apr 21, 2011 at 08:38, <mailto:Mitch.Garnaat@gmail.com> wrote: >>>> >>> >>> > >>>> > http://codereview.appspot.com/4425052/diff/7001/boto/connection.py >>>> > >>>> > File boto/connection.py (right): >>>> > >>>> > >>>> > >>>> >>> >>> http://codereview.appspot.com/4425052/diff/7001/boto/connection.py#newcode77 >>> >>>> > boto/connection.py:77: >>>> >>> os.path.dirname(os.path.abspath(cacerts.__file__ >>> >>>> > )), "cacerts.txt") >>>> > I wish I could think of a better way of doing this but I really >>>> >>> can't. >>> >>>> > Python doesn't provide any standard mechanisms for accessing data >>>> >>> files, >>> >>>> > AFAIK. This is a clever hack but it's still a hack. Is there any >>>> > precedence for this in other packages? >>>> >>> >>> I think it's pretty common to use __file__ to find the directory a >>>> >>> module >>> >>>> came from. The trick of making the directory with the cacerts file an >>>> otherwise empty module is something I thought of; I'm not sure I've >>>> >>> seen >>> >>>> this before. >>>> >>> >>> I suppose there are lots of things in python that one could call both >>>> "elegant" and "a terrible hack" at the same time :) >>>> >>> >>> >>> I didn't call it a "terrible hack", just a hack. And a clever one at >>> that. If there was some nice, clean, standard mechanism for doing this >>> then I would argue for using it but there isn't so this seems as good a >>> solution as any and better than most. >>> >>> >>> I could get rid of that cacerts package and reference the file >>>> >>> relative to >>> >>>> the boto root directory, but I don't know of any other way besides >>>> >>> __file__ >>> >>>> to find out where the boto package is installed. >>>> >>> >>> >>> >>> > >>>> > >>>> > http://codereview.appspot.com/4425052/ >>>> > >>>> >>> >>> >>> >>> http://codereview.appspot.com/4425052/ >>> >> >> >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
