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

Update Actions, test certs, test code [changelog skip] #2333

Merged
merged 3 commits into from Sep 9, 2020

Conversation

MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented Aug 7, 2020

Description

Certs used for testing are outdated, and no longer work with Ubuntu 20.04. The certs also fail if OpenSSL::SSL::SSLContext#security_level is set to 2 or higher.

PR updates Actions, updates all certs used, and provides code/instructions for updating them. Minor changes to test_puma_server_ssl.rb.

First commit is done to show that tests are failing with Ubuntu 20.04. All MRI builds using OpenSSL 1.1.1 failed. See CI:
https://github.com/puma/puma/actions/runs/199729320

After two commits added with the fixes, all Ubuntu MRI passed. See:
https://github.com/puma/puma/actions/runs/199798150

At present, both jruby and jruby-head won't compile on Ubuntu-20.04, so their CI is left on Ubuntu-18.04.

Closes #2330
Closes #2147

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Aug 7, 2020

@pvalena

If you have time, could you please check and see if this PR fixes the SSL issues you've had?

@dentarg
Copy link
Member

dentarg commented Aug 7, 2020

@MSP-Greg does this also solve #2147?

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Aug 7, 2020

Last force push updated examples/puma/keystore.jks, which I overlooked. All green...

@MSP-Greg MSP-Greg changed the title Updates Actions, test certs, test code [changelog skip] Update Actions, test certs, test code [changelog skip] Aug 8, 2020
@MSP-Greg MSP-Greg marked this pull request as ready for review September 1, 2020 22:28
@nateberkopec nateberkopec added the waiting-for-review Waiting on review from anyone label Sep 5, 2020
@@ -19,15 +19,16 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [ ubuntu, macos, windows ]
ruby: [ 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, head, jruby, truffleruby-head ]
os: [ ubuntu-20.04, macos-10.15, windows-2019 ]
Copy link
Member

Choose a reason for hiding this comment

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

I see the value in doing some separate Ubuntu 20/18 testing, but why pin our mac and windows tests too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have strong feeling about it.

Right now, ubuntu-latest is 18.04. I don't like pinning Ruby versions, but I think OS's is reasonable, and for a person unfamiliar with Actions, the numeric callout is clear?

I can add some MRI Ruby to ubuntu-18.04. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

It's just kinda a weird situation where ubuntu "latest" is not 20.04.

I think we just test the latest MRI Ruby version on 20.04. That seems like enough coverage IMO.

Copy link
Member Author

@MSP-Greg MSP-Greg Sep 6, 2020

Choose a reason for hiding this comment

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

It's just kinda a weird situation where ubuntu "latest" is not 20.04.

I have a lot of Actions bookmarks, but somewhere they have listings of dates re runner OS's. etc. I'll look for it (later), and add it to the workflow. IOW, I think it's where they'd announce when 20.04 will become 'latest'. I wouldn't be surprised if they deprecate the 'latest' concept. Note that every change here was needed to get 20.04 CI to pass...

Maybe test MRI head, 2.7 & 2.5 on 20.04, and run all the tests on 18.04, as was done previously?

Or, what would you like? I'm working on #2337...

Copy link
Member

Choose a reason for hiding this comment

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

I have a lot of Actions bookmarks

Me too :)

https://docs.github.com/en/actions/reference/software-installed-on-github-hosted-runners says "Note: The Ubuntu 20.04 virtual environment is currently provided as a preview only." (https://github.com/actions/virtual-environments#available-environments says the same thing)

According to actions/runner-images#1430 there doesn't seem to be any date set when the preview period will be over.

Copy link
Member Author

Choose a reason for hiding this comment

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

I may have been mistaken, but they often pin issues (https://github.com/actions/virtual-environments/issues), I think that was the case when they thought they'd remove windows-2016, which people complained about...

@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Sep 5, 2020
@voxik
Copy link
Contributor

voxik commented Sep 7, 2020

The "All certs updated except examples/puma/keystore.jks" of 2nd commit is not true anymore I believe.

@voxik
Copy link
Contributor

voxik commented Sep 7, 2020

@pvalena

If you have time, could you please check and see if this PR fixes the SSL issues you've had?

I have tried to backport this PR for puma 4.3.6, but I am battling with TestPumaServerSSLClient#test_verify_client_cert. It is throwing #<OpenSSL::SSL::SSLError: SSL_connect returned=1 errno=0 state=error: certificate verify failed (unspecified certificate verification error)> exception. Any hint what I am missing?

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Sep 7, 2020

@voxik

Re JRuby, I can't get the TestPumaServerSSLClient tests to pass on Actions or locally, but the TestPumaServerSSL tests seem ok.

Working on it. I think some of the JRuby OpenSSL tests weren't run when I opened this PR...

@voxik
Copy link
Contributor

voxik commented Sep 7, 2020

JRuby

Just FTR, I am testing on MRI

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Sep 7, 2020

@nateberkopec

Updated and passing (fixed a typo or two, JRuby issue, and rebased). It's running MRI 2.5, 2.7 & head on Ubuntu 20.04, and left the 18.04 jobs as is. Personally, I'd like to stop using job 'includes', so all jobs sort by OS. Thoughts? Be happy to do one more push...

MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 8, 2020
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 8, 2020
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 8, 2020
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 8, 2020
@MSP-Greg
Copy link
Member Author

MSP-Greg commented Sep 8, 2020

@voxik

See the commits listed on 'Sept 7' at https://github.com/MSP-Greg/puma/commits/4.3.6-ssl-fixtures for a 4.3.6 backport. CI passes with a few Ubuntu 20.04 jobs.

@voxik
Copy link
Contributor

voxik commented Sep 9, 2020

It seems I have managed to make it work on Fedora Rawhide. I was probably missing this hunk:

   def assert_ssl_client_error_match(error, subject=nil, &blk)
-    host = "127.0.0.1"
+    host = "localhost"
     port = UniquePort.call

because I had some issues with previous versions of this patch. But now it LGTM.

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Sep 9, 2020

@voxik

I had some issues with previous versions of this patch

Running it both locally and on Actions (with Ubuntu 20.04), I did make a change recently:

host_addrs = server.binder.ios.map { |io| io.to_io.addr[2] }

assert_includes host_addrs, events.addr if error

Without that, on some systems, the old assert failed, as both IPv4 & IPv6 hosts existed...

Anyway, glad it's passing on Fedora Rawhide.

@nateberkopec

The commits as included may cause a failure if one needed to bisect. I'll rebase & reorder them.

@voxik
Copy link
Contributor

voxik commented Sep 9, 2020

Without that, on some systems, the old assert failed, as both IPv4 & IPv6 hosts existed...

Yep, I mildly remember ~3 issues with ::1 referenced somewhere in place of localhost. So that must be it.

@nateberkopec nateberkopec merged commit 6f0caa5 into puma:master Sep 9, 2020
@nateberkopec
Copy link
Member

Brilliant work as usual, Greg!

@MSP-Greg MSP-Greg deleted the test-cert-update branch September 20, 2020 14:34
nateberkopec pushed a commit that referenced this pull request Sep 24, 2020
* Backport ssl fixtures/changes from #2333

* RuboCop server.rb

* Update Actions workflow, add Ubuntu 20.04

* Update extconf.rb

* Backport #2121

Co-authored-by: wjordan <will@code.org>

Co-authored-by: wjordan <will@code.org>
c960657 added a commit to c960657/httpi that referenced this pull request Nov 15, 2020
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Nov 28, 2020
…#2380)

* Backport ssl fixtures/changes from puma#2333

* RuboCop server.rb

* Update Actions workflow, add Ubuntu 20.04

* Update extconf.rb

* Backport puma#2121

Co-authored-by: wjordan <will@code.org>

Co-authored-by: wjordan <will@code.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance waiting-for-changes Waiting on changes from the requestor
Projects
None yet
4 participants