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

fix: improve openssl checker #2987

Merged
merged 1 commit into from
Jun 6, 2023
Merged

Conversation

ffontaine
Copy link
Contributor

@ffontaine ffontaine commented May 9, 2023

Improve openssl checker to avoid false positives with bind, exim, http_server, i2pd, janus, libssh, nginx, nmap, node, ntp, ntpsec, proftpd, sofia-sip, stunnel, tor and zabbix binaries which link dynamically with openssl library (and save the associated version number) or which has the following string:

OpenSSL 1.1.1 or newer

While at it, add an OpenWRT test package

@ffontaine ffontaine force-pushed the fix-openssl-checker branch 2 times, most recently from 1c30045 to 517aff0 Compare May 10, 2023 07:15
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2023

Codecov Report

Merging #2987 (f6b2717) into main (9bdba0c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f6b2717 differs from pull request most recent head 51f1077. Consider uploading reports for the commit 51f1077 to get more accurate results

@@           Coverage Diff           @@
##             main    #2987   +/-   ##
=======================================
  Coverage   82.67%   82.68%           
=======================================
  Files         674      676    +2     
  Lines       10626    10660   +34     
  Branches     1426     1429    +3     
=======================================
+ Hits         8785     8814   +29     
  Misses       1474     1474           
- Partials      367      372    +5     
Flag Coverage Δ
longtests 81.66% <100.00%> (+5.23%) ⬆️
win-longtests 80.10% <100.00%> (-0.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
test/test_data/apache_http_server.py 100.00% <ø> (ø)
test/test_data/bind.py 100.00% <ø> (ø)
test/test_data/exim.py 100.00% <ø> (ø)
test/test_data/i2pd.py 100.00% <ø> (ø)
test/test_data/janus.py 100.00% <ø> (ø)
test/test_data/libssh.py 100.00% <ø> (ø)
test/test_data/nginx.py 100.00% <ø> (ø)
test/test_data/nmap.py 100.00% <ø> (ø)
test/test_data/node.py 100.00% <ø> (ø)
test/test_data/ntp.py 100.00% <ø> (ø)
... and 8 more

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ffontaine ffontaine force-pushed the fix-openssl-checker branch 2 times, most recently from 639a081 to 698c17a Compare May 10, 2023 13:54
Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

I imagine this one will make a lot of folk happy. Thanks!

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

Whoops, hit approve too soon. Looks like the PR is missing a condensed download package for the new ipk.

@ffontaine ffontaine force-pushed the fix-openssl-checker branch 5 times, most recently from f6b2717 to 51f1077 Compare May 11, 2023 07:57
Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

Noticed the removal of pyca cryptography test and I think we should keep that one. Did it break when you updated the patterns?

Comment on lines 23 to 29
{
"url": "https://files.pythonhosted.org/packages/ba/91/84a29d6a27fd6dfc21f475704c4d2053d58ed7a4033c2b0ce1b4ca4d03d9/",
"package_name": "cryptography-3.0-cp35-abi3-manylinux2010_x86_64.whl",
"product": "openssl",
"version": "1.1.1g",
"other_products": ["gcc"],
},
Copy link
Contributor

Choose a reason for hiding this comment

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

The cryptography package really does include a copy of openssl. Or it did at the time this test was added, anyhow. I don't know offhand if later versions found another way, but it was policy to include the correct, latest openssl as part of the wheel to reduce dependency friction with various OSes.

So I think we should keep this test. It's surprising but likely useful to make sure we don't stop detecting that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the pattern to continue to detect openssl in cryptography

Improve openssl checker to avoid false positives with bind, exim,
http_server, i2pd, janus, libssh, nginx, nmap, node, ntp, ntpsec,
proftpd, sofia-sip, stunnel, tor and zabbix binaries which link
dynamically with openssl library (and save the associated version
number) or which has the following string:

OpenSSL 1.1.1 or newer

While at it, add an OpenWRT test package

Signed-off-by: Fabrice Fontaine <fabrice.fontaine@orange.com>
Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

Thank you! Time to get this merged.

@terriko terriko merged commit 0f7a834 into intel:main Jun 6, 2023
20 of 22 checks passed
@ffontaine ffontaine deleted the fix-openssl-checker branch June 7, 2023 06:29
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

3 participants