-
-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
Conversation
Result of 9 packages built:
|
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. |
1d015f9
to
fbcd0a2
Compare
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
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 |
fbcd0a2
to
7c55b6c
Compare
@stigtsp Okay, I manually doctored the patch to ensure it isn't missing anything this time. |
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 |
I can do that. Since the modules used change, the Perl build files need patched as well to ensure the dependencies are correct, right? |
7c55b6c
to
885c135
Compare
@stigtsp I figured out how to use |
pkgs/top-level/perl-packages.nix
Outdated
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"; |
There was a problem hiding this comment.
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:
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"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
pkgs/top-level/perl-packages.nix
Outdated
--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" |
There was a problem hiding this comment.
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
--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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this 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
There was a problem hiding this 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 ];
00ef02a
to
a63d554
Compare
I've removed that one now. |
I've added a follow-up patch which corrects the dependencies specified in the project. |
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? |
... 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 |
I've got the tests working, just needed to add
That sounds like a very good idea. I'm working on it. Safe travels! |
a63d554
to
2074b1f
Compare
Okay, I've done a couple of things.
I've reworked the commits to reflect these changes. Let me know if I should rework or reorder anything. Thanks! |
2074b1f
to
cf0498e
Compare
pkgs/top-level/perl-packages.nix
Outdated
CryptDES | ||
CryptRijndael | ||
DigestHMAC | ||
DigestSHA |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've dropped DigestSHA
.
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.
cf0498e
to
b44ec15
Compare
|
There was a problem hiding this 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! 🎉
EDIT: This seems to be caused by #351312 Having some issues building this (on several machines), seemingly caused by fetchpatch2.
Unsure why exactly, but observing that rewriting to using 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
]; |
@ofborg build perlPackages.NetSNMP |
There was a problem hiding this 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
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
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.