brooklynDev / airborne

RSpec driven API testing framework
MIT License
1.13k stars 122 forks source link

create verify_ssl config #152

Closed mycargus closed 5 years ago

mycargus commented 5 years ago

This should close issue #118

A few notes on this PR:

  1. I don't quite grok the head and options base methods, so I may have missed some necessary changes there.

  2. I came across the following related report in the rest-client repo, but I couldn't reproduce the reported error with this PR in place. ¯_(ツ)_/¯ https://github.com/rest-client/rest-client/issues/628

  3. I wanted to refactor the base.rb methods to simplify the method parameters (for example, to avoid get '/foo', nil, false ... feels icky) and to allow passing any desired options down to RestClient, but decided against it. I think most users will be served by the Airborne.configuration and RSpec metadata tag for now.

  4. The excellent badssl.com project lets us manually verify these changes actually work:

RSpec.describe 'Bad SSL' do
  it 'does\'t verify the SSL certificate', verify_ssl: false do
    Airborne.configuration.base_url = 'https://self-signed.badssl.com'
    get '/'
    expect_status(200)
  end

  it 'tries to verify the SSL cert and returns a nil response', verify_ssl: true do
    Airborne.configuration.base_url = 'https://self-signed.badssl.com'
    get '/'
    expect(response).to be_nil
  end
end

Note they specifically recommend against using their servers for automated testing, so I didn't code a spec. I think having one would be a good addition though if y'all want to fork their repo and host a "bad ssl" server.

Let me know if anything needs changing!

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.03%) to 99.024% when pulling 2f083eeffca4464be43f877de6670127f2963866 on mycargus:fix-issue-118 into f2bd6771b2001abb17cd1baccde0b1c14fa9e9d1 on brooklynDev:master.

Mythra commented 5 years ago

Perhaps we should document how turning of TLS Verification should only be turned off when you're absolutely sure you need to? And it doesn't actually fix talking to real remote sites? :wink: :wink:

sethpollack commented 5 years ago

What about rack test, does this api work for that too?

mycargus commented 5 years ago

+1 @SecurityInsanity, I'll update the readme.

@sethpollack agh, I didn't think about rack test. I'll look into that.

Thank you both!

mycargus commented 5 years ago

@sethpollack I could very well be wrong as I don't know Rack well, but I think disabling SSL cert verification in the RackTestRequester would require adding a Rack middleware gem. I couldn't find any SSL support in the rack-test gem besides setting a {'HTTPS' => 'on'} env var, which as far as I can tell doesn't help us with this feature PR.

What do you think? Should this be a non-Rack feature only, or should we add a middleware gem dependency? Or might there be a different solution?

sethpollack commented 5 years ago

Not sure, but I think I was trying to keep the api consistent between all the requesters.

mycargus commented 5 years ago

Understandable. As an end user I would prefer consistency as well.

sethpollack commented 5 years ago

So what do you suggest we do?

mycargus commented 5 years ago

@sethpollack My team and I need this feature ASAP, so I'm biased. :) That said, I've been researching all morning and the solution for disabling SSL cert verification in Rack still isn't clear to me. The only semi-living Rack middleware gem for SSL I could find is https://github.com/tobmatth/rack-ssl-enforcer, but I can't find any option to disable cert verification in that code base.

My suggestion is to merge this PR after I add a note to the README to indicate this flag won't work for directly testing Rack apps that aren't running in a server, e.g.

Airborne.configure do |config|
  config.rack_app = MySinatraApp
end
mycargus commented 5 years ago

@sethpollack What do you think? We need to know if this can get merged and released today. Thanks!

mycargus commented 5 years ago

thanks!