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.

Sign up for GitHub

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

Jump to bottom

Revise SSL verification handling for debugging #9944

Merged
merged 2 commits into from
Aug 3, 2017
Merged

Revise SSL verification handling for debugging #9944

merged 2 commits into from
Aug 3, 2017

Conversation

nafu
Copy link
Collaborator

@nafu nafu commented Aug 3, 2017

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

The execution of OpenSSL::SSL::VERIFY_PEER = OpenSSL::SSL::VERIFY_NONE should be revised.

Description

Use Faraday option to use OpenSSL::SSL::VERIFY_NONE instead of using OpenSSL::SSL::VERIFY_PEER = OpenSSL::SSL::VERIFY_NONE.

ohayon
ohayon reviewed Aug 3, 2017
View reviewed changes
Copy link
Contributor

@ohayon ohayon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @nafu - I'm not sure I totally understand this PR, is this going to set VERIFY_NONE for all requests because we are not checking for the SPACESHIP_DEBUG flag anymore?

@nafu
Copy link
Collaborator Author

nafu commented Aug 3, 2017

Hi @ohayon We still use SPACESHIP_DEBUG flag and set VERIFY_NONE only for debugging.

https://github.com/fastlane/fastlane/pull/9944/files#diff-a9086da2d7e8a66b647fdabdb0a7cca7R257

Now SPACESHIP_DEBUG setup is in one place like below. 👍

if ENV['SPACESHIP_DEBUG']
    # for debugging only
    # This enables tracking of networking requests using Charles Web Proxy
    c.proxy "https://127.0.0.1:8888"
    c.ssl[:verify_mode] = OpenSSL::SSL::VERIFY_NONE
end

ohayon
ohayon approved these changes Aug 3, 2017
View reviewed changes
Copy link
Contributor

@ohayon ohayon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! My bad! I missed that if statement. Thanks for the information! 🚀

ohayon
ohayon reviewed Aug 3, 2017
View reviewed changes
Copy link
Contributor

@ohayon ohayon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, @nafu I missed a couple of things there that I just have questions about! 😄

fastlane/lib/fastlane/actions/hockey.rb
if ENV['SPACESHIP_DEBUG']
# for debugging only
# This enables tracking of networking requests using Charles Web Proxy
builder.proxy "https://127.0.0.1:8888"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to set builder.ssl[:verify_mode] = OpenSSL::SSL::VERIFY_NONE Here also? It feels like we are deleting something here that we might want?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this isn't a part of spaceship. I assume these might be copy and pasted from spaceship faraday code. So I delete this from hockey action.

fastlane/lib/fastlane/actions/hockey.rb
end

if ENV["DEBUG"]
puts "To run _spaceship_ through a local proxy, use SPACESHIP_DEBUG"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we are deleting this here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason above, but if I should revert this let me know 👍

@ohayon
Copy link
Contributor

ohayon commented Aug 3, 2017

I think you're probably right for both of those things @nafu ! Lets go ahead and merge! 🚀 Do you want to press merge?

@nafu
Copy link
Collaborator Author

nafu commented Aug 3, 2017

👍 @ohayon Thanks. I'll press 🚀

@nafu nafu merged commit 1f19c1a into fastlane:master Aug 3, 2017
@nafu nafu deleted the revise-ssl-verify branch August 3, 2017 18:44
@fastlane-bot
Copy link

Hey @nafu 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

@KrauseFx KrauseFx mentioned this pull request Aug 7, 2017
Merged
@fastlane-bot
Copy link

Congratulations! 🎉 This was released as part of fastlane 2.51.0 🚀

dvdchr pushed a commit to dvdchr/fastlane that referenced this pull request Sep 5, 2017
@nafu
Revise SSL verification handling for debugging (fastlane#9944)
b77f524 
* Revise SSL verify handling for debugging

See more about lostisland/faraday:
https://github.com/lostisland/faraday/blob/e1e50f5eb4a03cde91fd7827f9df46b4f58c19b2/lib/faraday/adapter/net_http.rb#L124-L132

* Remove spaceship debugging codes from hockey action
@fastlane fastlane locked and limited conversation to collaborators Oct 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ohayon ohayon ohayon approved these changes

Assignees
No one assigned
Labels
cla: yes status: released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nafu @ohayon @fastlane-bot @googlebot