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

Remove deprecated Ruby. Upgrade bundler. Fix failing tests #67

Merged
merged 1 commit into from
Jul 2, 2020

Conversation

phumpal
Copy link
Contributor

@phumpal phumpal commented Jun 5, 2020

Pull Request Checklist

Is this in reference to an existing issue?

General

  • Update Changelog following the conventions laid out here

  • Update README with any necessary configuration snippets

  • Binstubs are created if needed

  • RuboCop passes

  • Existing tests pass

New Plugins

  • Tests

  • Add the plugin to the README

  • Does it have a complete header as outlined here

Purpose

Known Compatibility Issues

@phumpal phumpal force-pushed the remove-legacy-ruby branch 3 times, most recently from c707bdb to ecc08e0 Compare June 5, 2020 07:06
@phumpal
Copy link
Contributor Author

phumpal commented Jun 5, 2020

The HSTS Preload check is failing because the cert expired for https://oskuro.net/

Loos like an oversight (missing cron renewal?) https://www.sslshopper.com/ssl-checker.html#hostname=oskuro.net

@phumpal phumpal force-pushed the remove-legacy-ruby branch 2 times, most recently from df1ebc6 to d67205b Compare June 5, 2020 07:30
@phumpal
Copy link
Contributor Author

phumpal commented Jun 5, 2020

I kinda cheated here and removed a test as the cert for the domain expired.

Tests are passing again. I opened an issue on chromium/hstspreload#115

@phumpal phumpal force-pushed the remove-legacy-ruby branch from d67205b to ef29a7d Compare June 5, 2020 22:59
@phumpal
Copy link
Contributor Author

phumpal commented Jun 6, 2020

@majormoses sorry to bug you again.

This fixes some of the test issues but not the failing HSTS test which I've removed in ac7196a. I suspect that's a happy medium as the parent issue has been in limbo for some time.

Also, having passing tests would be nice so we can add this #66

@phumpal
Copy link
Contributor Author

phumpal commented Jun 8, 2020

Anyone available to take a look at this?

Perhaps adding a note saying the test is disabled (and adding an ignore) is the better approach.

Would love to see this merged and #66 review with passing tests.

@phumpal
Copy link
Contributor Author

phumpal commented Jun 8, 2020

Gentle ping @rwky ^^ re: failing HSTS preload test (w/warnings)

@demonfoo
Copy link

demonfoo commented Jun 8, 2020

Also the check-ssl-anchor.rb tests fail locally for me, because OpenSSL 1.1.1's output changed from 1.0.1 and before, thus preventing it from working at all.

@majormoses
Copy link
Member

majormoses commented Jun 9, 2020

Hey so I think we should rather than remove the test update it to something we can trust. While I can't guarantee my cert wont ever expire since software by its nature we can use https://benabrams.it which is not currently expired and has the appropriate headers and preloading: https://hstspreload.org/?domain=benabrams.it.

I see we specifically need something with warnings...if someone can give me instructions on what I need to setup to make it "sorta wrong" I can try setting something up over the weekend.

I have some existing DNS entries we can also try testing if it helps: https://github.com/sensu-plugins/sensu-plugins-dns/blob/master/test/integration/helpers/serverspec/check-dns-shared_spec.rb

@phumpal
Copy link
Contributor Author

phumpal commented Jun 9, 2020

if someone can give me instructions on what I need to setup to make it "sorta wrong"

If I'm not mistaken here are some of the warning conditions

https://github.com/chromium/hstspreload/blob/master/header_test.go#L148-L201

@phumpal
Copy link
Contributor Author

phumpal commented Jun 16, 2020

I can try setting something up over the weekend

@majormoses any luck or should I dig in more to provide guidance?

@majormoses
Copy link
Member

I can try setting something up over the weekend

@majormoses any luck or should I dig in more to provide guidance?

Sorry I didn't get around to this. Hopefully I have the time and will take a look this weekend. I am also gonna see if there is any interest on the sensu inc side to host some testing infra for this.

@jspaleta
Copy link
Contributor

Okay so I have 2 local failing tests in master branch right now.

  1. failure for the anchor test is entirely due to a change in how the openssl client reports the certificate information. We just need to adjust the regexp match used for data[0] leading to a NOTOK string replacement to work for both newer and older openssl.
    I think I can prototype a less strict regexp that will work on both..but see my note below.

  2. failure of the preloading test warning condition. Since this check depends on how the hstspreload.org database works, i'd recommend just disabling the warning test. For this to work reliably the hstspreload.org would need to provide a guaranteed warning example.

Note:

I have to say... the fact that the anchor and cert checks requires a shell out to openssl commandline is a bit unnerving to me.
We should be able to accomplish the equivalent functionality with the ruby openssl module which makes use of the opensssl library directly, which may clean up changes in the text representation of the certificate chain.

@majormoses
Copy link
Member

@phumpal sorry been busy, I have not had time to look into setting this up. I have reached out to some folks at Sensu Inc to see if they are willing to help out here as I can see the value that it would provide the community to have some test infra available for ci and development.

@majormoses
Copy link
Member

I have to say... the fact that the anchor and cert checks requires a shell out to openssl commandline is a bit unnerving to me.
We should be able to accomplish the equivalent functionality with the ruby openssl module which makes use of the opensssl library directly, which may clean up changes in the text representation of the certificate chain.

No doubt, I am 👍 on it. It's one of the older checks while we have written many plugins to not rely on shell outs they do exist and in some cases are the only viable option as there is no native solution in the language. Take for example raid checks for a proprietary system where they provide a binary you can use but are not going to write client/language sdks.

@jspaleta
Copy link
Contributor

jspaleta commented Jun 24, 2020

One other thing... right now the cert used in the anchor test would need to also need to account for the openssel 1.0 or 1.1 variant being used.
We'd need to have rspec test the openssl client version by running openssl version capture the version number, parse it, and then use the correct anchor string variant appropriate for that version.

For openssl 1.1 -> 'i:O = Digital Signature Trust Co., CN = DST Root CA X3'
For openssl 1.0 -> 'i:/O=Digital Signature Trust Co./CN=DST Root CA X3'

There's probably some way to do that conditional switch out in the spec_helper.rb and then reference whatever variable the spec helper tests.

We could force the travis environment to be something newer that has openssl 1.1 and just live with it, or vice versa.

The important thing is to fix the regexp in the 'bin/check-ssl-anchor.rb' as that will cause the check to fail on the final host.

@jspaleta
Copy link
Contributor

One last thought, We could let the anchor cmdline option encode a regexp.. that way the test could use it.

Yeah I think I like that.. I'm gonna prototype that change as well.

@jspaleta
Copy link
Contributor

I'm gonna prep another pull request that will fix the anchor check and anchor check test by making use of the regexp.

I'm going to make it so its backwards compatible, by introducing a new cmdline option to treat the anchor as a regexp instead of as a normal string.

@jspaleta jspaleta mentioned this pull request Jun 24, 2020
2 tasks
@jspaleta
Copy link
Contributor

Okay I have a new branch up with a PR #70 that addresses the failing tests as I found them.

I was able to test the new anchor regexp logic on Fedora with openssl 1.1 and Centos7 with openssl 1.0 So that looks good.

The preload warning test I just disabled as it can't be made reliable.. until someone can identify a domain guaranteed to produce a warning from the online lookup.

@jspaleta
Copy link
Contributor

Take a look at this:
https://github.com/jarthod/ssl-test

this has everything we need to do in check-ssl-anchor.rb and more

@jspaleta
Copy link
Contributor

Okay New PR #71 is now up with a pure Ruby alternative implementation for the check-ssl-anchor.rb plugin.

@jspaleta
Copy link
Contributor

Hey @phumpal

Can you rebase this PR against master now that we've gotten my related PRs merged.

@phumpal phumpal force-pushed the remove-legacy-ruby branch from ac7196a to bf16f16 Compare June 30, 2020 19:42
@phumpal
Copy link
Contributor Author

phumpal commented Jun 30, 2020

@jspaleta rebased against master. PTAL

Copy link
Member

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -4,6 +4,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
This CHANGELOG follows the format listed [here](https://github.com/sensu-plugins/community/blob/master/HOW_WE_CHANGELOG.md).

## [Unreleased]
- Remove ruby-2.3.0. Upgrade bundler. Fix failing tests (@phumpal).
Copy link
Member

Choose a reason for hiding this comment

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

This should be under the Breaking Changes section but we will fix that up for you when we release (hoping to devote some time to sensu projects tomorrow).

@majormoses majormoses merged commit 47bf0ac into sensu-plugins:master Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants