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 tests to use GH actions to work with more recent openssl versions #1

Merged
merged 4 commits into from
Feb 19, 2021

Conversation

dwelch-r7
Copy link
Contributor

@dwelch-r7 dwelch-r7 commented Feb 9, 2021

This PR replaces travis with Github actions

Also updates a handful of tests that fail on more recent versions of openssl (where by default SSLv2 isn't available) where they were updated they should now work with a greater range of openssl versions

test run here: https://github.com/dwelch-r7/rex-sslscan/actions/runs/551962679

Investigated as part of the ruby 3 upgrade here rapid7/metasploit-framework#14666

Comment on lines 147 to 151
rescue ArgumentError => e
expect(e.message).to eq "unknown SSL method `SSLv2'."
rescue OpenSSL::OpenSSLError => e
expect(e.message).to eq "SSL_CTX_set_min_proto_version"
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is unexpected for me, I wouldn't expect the tests (or API) to look like this. Generally we'd expect only one error to be returned. This looks like a breaking change to me at the very least

i.e. Are there any assumptions on the client that we'll raise ArgumentError? Now that we're not raising that, it might be not gracefully handled correctly by a calling client.

As a side note, I wonder why this wasn't written like the other code

expect { ... }.to raise ...

Copy link
Contributor Author

@dwelch-r7 dwelch-r7 Feb 9, 2021

Choose a reason for hiding this comment

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

This change is unexpected for me, I wouldn't expect the tests (or API) to look like this

I 100% agree, the tests are very strange (even before I butchered them) either expecting a success or a specific failure...depending on how openssl is configured, and I think since these tests were written whether or not your version of openssl supports SSLV2 throws a different exception

as for why it wasn't written like the other code is because it won't always raise an exception, only if SSLv2 isn't supported 😅

@adfoster-r7 adfoster-r7 merged commit e9d83fc into rapid7:master Feb 19, 2021
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.

None yet

2 participants