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

perlPackages.NetSNMP: add patch to support modern SHA algorithms for authentication #349861

Merged
merged 3 commits into from
Oct 27, 2024

Conversation

jwillikers
Copy link
Contributor

@jwillikers jwillikers commented Oct 19, 2024

The aged perlPackages.NetSNMP library does not support modern SHA authentication protocols such as SHA-256 and SHA-512. It only supports the less secure SHA-1 algorithm. It's possible to enable support for these newer protocols with a patch from the OpenBSD ports tree found here.

References:

Patches used in Linux distributions:

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@zakame
Copy link
Member

zakame commented Oct 20, 2024

Result of nixpkgs-review pr 349861 run on x86_64-linux 1

9 packages built:
  • fusionInventory
  • munin
  • ocsinventory-agent
  • ocsinventory-agent.devdoc
  • perl538Packages.NetSNMP
  • perl538Packages.NetSNMP.devdoc
  • perl540Packages.NetSNMP
  • perl540Packages.NetSNMP.devdoc
  • smokeping

@zakame
Copy link
Member

zakame commented Oct 20, 2024

Thanks for the patch - checking upstream it appears maintenance is stalled and there are actually a few more suggestions of patches, cf https://rt.cpan.org/Public/Bug/Display.html?id=153907

@jwillikers
Copy link
Contributor Author

jwillikers commented Oct 20, 2024

Thanks for the patch - checking upstream it appears maintenance is stalled and there are actually a few more suggestions of patches, cf https://rt.cpan.org/Public/Bug/Display.html?id=153907

That's good to hear, I hope it gets updated upstream.

@jwillikers jwillikers force-pushed the NetSNMP-more-algos branch 2 times, most recently from 1d015f9 to fbcd0a2 Compare October 21, 2024 14:49
@jwillikers jwillikers changed the title perlPackages.NetSNMP: add patch to support SHA-256 and SHA-512 authentication perlPackages.NetSNMP: add patch to support modern SHA algorithms for authentication Oct 21, 2024
@stigtsp
Copy link
Member

stigtsp commented Oct 21, 2024

Hi there!

The patch seems fine to me, but there are some differences between the one vendored in here and the one from OpenBSD ports.

Nothing bad I'm sure, but there are } else { and our $VERSION = v4.0.1; removed in the OpenBSD one that is not removed in this patch.

Index: lib/Net/SNMP/Security/USM.pm			      |	diff --git a/lib/Net/SNMP/Security/USM.pm b/lib/Net/SNMP/Secu
--- lib/Net/SNMP/Security/USM.pm.orig			      |	index 4fb5742..da82212 100644
+++ lib/Net/SNMP/Security/USM.pm			      |	--- a/lib/Net/SNMP/Security/USM.pm
@@ -25,9 +25,10 @@ use Net::SNMP::Message qw(		      |	+++ b/lib/Net/SNMP/Security/USM.pm
							      >	@@ -25,8 +25,9 @@ use Net::SNMP::Message qw(
 							      |	+
+							      |	
 our $VERSION = v4.0.1;					      <
 							      |	+
-   } else {						      <
 							      |	+
+							      |	
+   } else {						      |	    } else {
							      >	-

Generated with:

curl -so /tmp/net-smtp-openbsd.patch https://raw.githubusercontent.com/openbsd/ports/ab6365da3ac861e1c56520acdf388d421b357c17/net/p5-Net-SNMP/patches/patch-lib_Net_SNMP_Security_USM_pm
curl -so /tmp/net-smtp-nixpkgs-pr349861.patch https://raw.githubusercontent.com/NixOS/nixpkgs/fbcd0a2761e960a01eb6dde36a9a26259059c0f5/pkgs/development/perl-modules/net-snmp-enable-newer-sha-algorithms.patch
diff --suppress-common-lines --color=always -y /tmp/net-smtp-openbsd.patch /tmp/net-smtp-nixpkgs-pr349861.patch

@jwillikers
Copy link
Contributor Author

Hi there!

The patch seems fine to me, but there are some differences between the one vendored in here and the one from OpenBSD ports.

Nothing bad I'm sure, but there are } else { and our $VERSION = v4.0.1; removed in the OpenBSD one that is not removed in this patch.

Index: lib/Net/SNMP/Security/USM.pm			      |	diff --git a/lib/Net/SNMP/Security/USM.pm b/lib/Net/SNMP/Secu
--- lib/Net/SNMP/Security/USM.pm.orig			      |	index 4fb5742..da82212 100644
+++ lib/Net/SNMP/Security/USM.pm			      |	--- a/lib/Net/SNMP/Security/USM.pm
@@ -25,9 +25,10 @@ use Net::SNMP::Message qw(		      |	+++ b/lib/Net/SNMP/Security/USM.pm
							      >	@@ -25,8 +25,9 @@ use Net::SNMP::Message qw(
 							      |	+
+							      |	
 our $VERSION = v4.0.1;					      <
 							      |	+
-   } else {						      <
 							      |	+
+							      |	
+   } else {						      |	    } else {
							      >	-

Generated with:

curl -so /tmp/net-smtp-openbsd.patch https://raw.githubusercontent.com/openbsd/ports/ab6365da3ac861e1c56520acdf388d421b357c17/net/p5-Net-SNMP/patches/patch-lib_Net_SNMP_Security_USM_pm
curl -so /tmp/net-smtp-nixpkgs-pr349861.patch https://raw.githubusercontent.com/NixOS/nixpkgs/fbcd0a2761e960a01eb6dde36a9a26259059c0f5/pkgs/development/perl-modules/net-snmp-enable-newer-sha-algorithms.patch
diff --suppress-common-lines --color=always -y /tmp/net-smtp-openbsd.patch /tmp/net-smtp-nixpkgs-pr349861.patch

@stigtsp Thanks for finding that discrepancy. I must have screwed up the patch somehow when I imported it into Git or exported it from Git. I wish I could get fetchpatch to work for the patch, but it was giving me some trouble.

@jwillikers
Copy link
Contributor Author

@stigtsp Okay, I manually doctored the patch to ensure it isn't missing anything this time.

@stigtsp
Copy link
Member

stigtsp commented Oct 21, 2024

Looks good to me, maybe add a comment above the patch why it's vendored, including the source URL of the patch?

I can also reproduce problems applying this patch using fetchpatch but vendoring works.

@jwillikers
Copy link
Contributor Author

I can do that. Since the modules used change, the Perl build files need patched as well to ensure the dependencies are correct, right?

@jwillikers
Copy link
Contributor Author

Looks good to me, maybe add a comment above the patch why it's vendored, including the source URL of the patch?

I can also reproduce problems applying this patch using fetchpatch but vendoring works.

@stigtsp I figured out how to use fetchpatch for the patch.

patches = [
(fetchpatch {
name = "enable-newer-sha-algorithms.patch";
url = "https://raw.githubusercontent.com/openbsd/ports/refs/heads/master/net/p5-Net-SNMP/patches/patch-lib_Net_SNMP_Security_USM_pm";
Copy link
Member

Choose a reason for hiding this comment

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

The URL here should be pinned to a commit, maybe:

Suggested change
url = "https://raw.githubusercontent.com/openbsd/ports/refs/heads/master/net/p5-Net-SNMP/patches/patch-lib_Net_SNMP_Security_USM_pm";
url = "https://raw.githubusercontent.com/openbsd/ports/ab6365da3ac861e1c56520acdf388d421b357c17/net/p5-Net-SNMP/patches/patch-lib_Net_SNMP_Security_USM_pm";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes of course, my bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 19223 to 19224
--replace "--- lib/Net/SNMP/Security/USM.pm.orig" "--- a/lib/Net/SNMP/Security/USM.pm" \
--replace "+++ lib/Net/SNMP/Security/USM.pm" "+++ b/lib/Net/SNMP/Security/USM.pm"
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a reasonable solution, consider using --replace-fail so the replace will fail in case it doesn't actually replace anything

Suggested change
--replace "--- lib/Net/SNMP/Security/USM.pm.orig" "--- a/lib/Net/SNMP/Security/USM.pm" \
--replace "+++ lib/Net/SNMP/Security/USM.pm" "+++ b/lib/Net/SNMP/Security/USM.pm"
--replace-fail "--- lib/Net/SNMP/Security/USM.pm.orig" "--- a/lib/Net/SNMP/Security/USM.pm" \
--replace-fail "+++ lib/Net/SNMP/Security/USM.pm" "+++ b/lib/Net/SNMP/Security/USM.pm"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

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

Since the patch is now fetched from github, the vendored one can be removed

Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

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

The dependencies for Net::SNMP should be updated, DigestHMAC is needed for Digest::HMAC_MD5.

The other additions seem to be required by the failing test suite (although I didn't look at the failing tests in detail).

propagatedBuildInputs = [ CryptDES CryptRijndael DigestHMAC  DigestSHA1 Socket6 ];

@jwillikers jwillikers force-pushed the NetSNMP-more-algos branch 2 times, most recently from 00ef02a to a63d554 Compare October 23, 2024 19:17
@jwillikers
Copy link
Contributor Author

Since the patch is now fetched from github, the vendored one can be removed

I've removed that one now.

@jwillikers
Copy link
Contributor Author

The dependencies for Net::SNMP should be updated, DigestHMAC is needed for Digest::HMAC_MD5.

The other additions seem to be required by the failing test suite (although I didn't look at the failing tests in detail).

propagatedBuildInputs = [ CryptDES CryptRijndael DigestHMAC  DigestSHA1 Socket6 ];

I've added a follow-up patch which corrects the dependencies specified in the project.

@jwillikers jwillikers requested a review from stigtsp October 23, 2024 21:49
@stigtsp
Copy link
Member

stigtsp commented Oct 24, 2024

Thanks for the great work! I'm going traveling tomorrow, but will have a look as soon as I can after that.

I'm thinking we might consider looking at getting the tests to pass as well, there was a fork referenced in the RT issue here that could have some interesting patches for that:

https://github.com/michal-josef-spacek/Net-SNMP

What do you think @zakame?

@stigtsp
Copy link
Member

stigtsp commented Oct 24, 2024

... also if there are some test for the new algos somewhere, that would also be great so we can make sure this keeps working going forward

@jwillikers
Copy link
Contributor Author

jwillikers commented Oct 24, 2024

Thanks for the great work! I'm going traveling tomorrow, but will have a look as soon as I can after that.

I'm thinking we might consider looking at getting the tests to pass as well, there was a fork referenced in the RT issue here that could have some interesting patches for that:

https://github.com/michal-josef-spacek/Net-SNMP

What do you think @zakame?

I've got the tests working, just needed to add /etc/protocols since it uses getprotobyname.

... also if there are some test for the new algos somewhere, that would also be great so we can make sure this keeps working going forward

That sounds like a very good idea. I'm working on it.

Safe travels!

@jwillikers
Copy link
Contributor Author

jwillikers commented Oct 24, 2024

Okay, I've done a couple of things.

  1. I fixed the tests.
  2. I applied the patches from Fedora's package. These are probably the same as https://github.com/michal-josef-spacek/Net-SNMP since Michal Josef Špaček submitted those patches to the Fedora package. There's just a couple less patches in Fedora then there are commits in the repo. These patches correct the dependencies defined in the project so that we don't need to add our own patches to reflect this. These patches also have the benefit of adding more tests.
  3. I adapted the patch https://github.com/openbsd/ports/blob/master/net/p5-Net-SNMP/patches/patch-lib_Net_SNMP_Security_USM_pm to work on top of the Fedora patches.
  4. I included additional tests for each of the new SHA algorithms in the adapted patch.

I've reworked the commits to reflect these changes. Let me know if I should rework or reorder anything. Thanks!

CryptDES
CryptRijndael
DigestHMAC
DigestSHA
Copy link
Member

Choose a reason for hiding this comment

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

DigestSHA is in perl core, so you don't need to list it here

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've dropped DigestSHA.

@stigtsp
Copy link
Member

stigtsp commented Oct 25, 2024

This is starting to look pretty good I think, thanks again for the great work on it.

I'd like to review the tests and patches a bit more before merging, will try to get this done tomorrow.

The aged perlPackages.NetSNMP library does not support modern SHA algorithms for authentication.
It's possible to enable support for these newer protocols with a patch.
The patch is taken from the OpenBSD ports tree:
https://github.com/openbsd/ports/blob/master/net/p5-Net-SNMP/patches/patch-lib_Net_SNMP_Security_USM_pm
It enables support for SHA 224, 256, 384, and 512.
Tests are also include in the patch for these additional algorithms.
@zakame
Copy link
Member

zakame commented Oct 25, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 349861


x86_64-linux

✅ 11 packages built:
  • fusionInventory
  • munin
  • nagiosPlugins.check_nwc_health
  • nagiosPlugins.check_ups_health
  • ocsinventory-agent
  • ocsinventory-agent.devdoc
  • perl538Packages.NetSNMP
  • perl538Packages.NetSNMP.devdoc
  • perl540Packages.NetSNMP
  • perl540Packages.NetSNMP.devdoc
  • smokeping

Copy link
Member

@zakame zakame left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR! 🎉

@stigtsp
Copy link
Member

stigtsp commented Oct 26, 2024

EDIT: This seems to be caused by #351312


Having some issues building this (on several machines), seemingly caused by fetchpatch2.

error: builder for '/nix/store/7sq13pjbsqc10jifq9fzsbjjpkq6c6sz-Net-SNMP-v6.0.1-Rewrite_from_Digest-SHA1-to-Digest-SHA.patch.drv' failed with exit code 1;
       last 9 log lines:
       > error checking the existence of https://tarballs.nixos.org//sha256-dznhj1Fcy0iBBl92p825InjkNZixR2MURVQ/b9bVjtc=:
       > curl: (77) error setting certificate file: /nix/store/h8qhspg7pml84z8s4ghrhmx6m77rs161-nss-cacert-3.104/etc/ssl/certs/ca-bundle.crt
       >
       > trying https://src.fedoraproject.org/rpms/perl-Net-SNMP/raw/6e1d3e8ff2b9bd38dab48301a9d8b5d81ef3b7fe/f/Net-SNMP-v6.0.1-Rewrite_from_Digest-SHA1-to-Digest-SHA.patch
       >   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
       >                                  Dload  Upload   Total   Spent    Left  Speed
       >   0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
       > curl: (77) error setting certificate file: /nix/store/h8qhspg7pml84z8s4ghrhmx6m77rs161-nss-cacert-3.104/etc/ssl/certs/ca-bundle.crt
       > error: cannot download Net-SNMP-v6.0.1-Rewrite_from_Digest-SHA1-to-Digest-SHA.patch from any mirror
       For full logs, run 'nix log /nix/store/7sq13pjbsqc10jifq9fzsbjjpkq6c6sz-Net-SNMP-v6.0.1-Rewrite_from_Digest-SHA1-to-Digest-SHA.patch.drv'.`

Unsure why exactly, but observing that rewriting to using fetchpatch (and updating hashes due to different patch normalization presumably) works:

diff --git a/pkgs/top-level/perl-packages.nix b/pkgs/top-level/perl-packages.nix
index 3b533a10e96b..f7d05cc16c00 100644
--- a/pkgs/top-level/perl-packages.nix
+++ b/pkgs/top-level/perl-packages.nix
@@ -7,7 +7,7 @@

 { config
 , stdenv, lib, buildPackages, pkgs, darwin
-, fetchurl, fetchpatch, fetchpatch2, fetchFromGitHub, fetchFromGitLab
+, fetchurl, fetchpatch, fetchFromGitHub, fetchFromGitLab
 , perl, shortenPerlShebang
 , nixosTests
 }:
@@ -19214,25 +19214,25 @@ with self; {
       hash = "sha256-FMN7wcuz883H1sE+DyeoWfFM3P1epUoEZ6iLwlmwt0E=";
     };
     patches = [
-      (fetchpatch2 {
+      (fetchpatch {
         url = "https://src.fedoraproject.org/rpms/perl-Net-SNMP/raw/6e1d3e8ff2b9bd38dab48301a9d8b5d81ef3b7fe/f/Net-SNMP-v6.0.1-Switch_from_Socket6_to_Socket.patch";
-        hash = "sha256-IpVhqI+dXqzauTkLF0Doulg5U33FxHUhqFTp0jeMtMY=";
+        hash = "sha256-AlzNUkAmNSRIIZXs8v5gdp8n7fMfKyqqgM4VDpO47Bs=";
       })
-      (fetchpatch2 {
+      (fetchpatch {
         url = "https://src.fedoraproject.org/rpms/perl-Net-SNMP/raw/6e1d3e8ff2b9bd38dab48301a9d8b5d81ef3b7fe/f/Net-SNMP-v6.0.1-Simple_rewrite_to_Digest-HMAC-helpers.patch";
-        hash = "sha256-ZXo9w2YLtPmM1SJLvIiLWefw7SwrTFyTo4eX6DG1yfA=";
+        hash = "sha256-UFL1rz8HGsU1V+GypybBLvflzpAvh2DYNWjZbfjguvw=";
       })
-      (fetchpatch2 {
+      (fetchpatch {
         url = "https://src.fedoraproject.org/rpms/perl-Net-SNMP/raw/6e1d3e8ff2b9bd38dab48301a9d8b5d81ef3b7fe/f/Net-SNMP-v6.0.1-Split_usm.t_to_two_parts.patch";
-        hash = "sha256-A2gsD6DIX1aFSVLbSL/1zKSM1xiM6hWBadJJH7f5E8o=";
+        hash = "sha256-/tuPmpehUOnCsHgfSAXieQI3QZqEWr67mwohd7NVD2o=";
       })
-      (fetchpatch2 {
+      (fetchpatch {
         url = "https://src.fedoraproject.org/rpms/perl-Net-SNMP/raw/6e1d3e8ff2b9bd38dab48301a9d8b5d81ef3b7fe/f/Net-SNMP-v6.0.1-Add_tests_for_another_usm_scenarios.patch";
-        hash = "sha256-U7nNuL35l/zdSzx1jgjp1PmLQn3xzzDw9DGnyeydi2E=";
+        hash = "sha256-HDRRJTZD+u/ZHgTIx1fVr4U/0DVH92ELfOMAgY++RQE=";
       })
-      (fetchpatch2 {
+      (fetchpatch {
         url = "https://src.fedoraproject.org/rpms/perl-Net-SNMP/raw/6e1d3e8ff2b9bd38dab48301a9d8b5d81ef3b7fe/f/Net-SNMP-v6.0.1-Rewrite_from_Digest-SHA1-to-Digest-SHA.patch";
-        hash = "sha256-dznhj1Fcy0iBBl92p825InjkNZixR2MURVQ/b9bVjtc=";
+        hash = "sha256-jFz5eehV8MHFdd3Wq5S9wWCDhUr5qk4wAJ1mDRJffzw=";
       })
       ../development/perl-modules/net-snmp-add-sha-algorithms.patch
     ];

@stigtsp
Copy link
Member

stigtsp commented Oct 26, 2024

@ofborg build perlPackages.NetSNMP

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 26, 2024
Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

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

Looks good to me

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 349861


x86_64-linux

✅ 11 packages built:
  • fusionInventory
  • munin
  • nagiosPlugins.check_nwc_health
  • nagiosPlugins.check_ups_health
  • ocsinventory-agent
  • ocsinventory-agent.devdoc
  • perl538Packages.NetSNMP
  • perl538Packages.NetSNMP.devdoc
  • perl540Packages.NetSNMP
  • perl540Packages.NetSNMP.devdoc
  • smokeping

@stigtsp stigtsp merged commit 0c98c85 into NixOS:master Oct 27, 2024
28 of 29 checks passed
@jwillikers jwillikers deleted the NetSNMP-more-algos branch October 27, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants