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

OpenSSL-related fixes for staging-next (2024-10-02) #345998

Closed

Conversation

thillux
Copy link
Contributor

@thillux thillux commented Oct 2, 2024

This PR is used to track broken packages after bumping OpenSSL default version from 3.0 -> 3.3. Furthermore some more cleanup with the different OpenSSL 3.x versions is done.

[x] edk2/OVMF: upstream currently not able to build with OpenSSL 3.3, set default OpenSSL to 3.0 for this package. (see: tianocore/edk2#6184)
[x] haskellPackages.openssl-streams: repo contains old 1024 Bit RSA key, which OpenSSL 3.3 does not accept by default. Tests failed. Upstream seems dead for some years.
[x] WIP: Adapted openssl_3 to openssl_3_0, removed openssl_3_2 which only had a single user (switched cloudflare-warp to openssl_3_3)

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.

@adamcstephens
Copy link
Contributor

Is this only for edk2, or are you planning to add more to it? If just edk2, then the title should reflect this.

@thillux
Copy link
Contributor Author

thillux commented Oct 4, 2024

I'll planned to address multiple OpenSSL related issues here (if there are any). Otherwise I'll rename to EDK2 fixes.

vcunat

This comment was marked as resolved.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Oct 4, 2024
@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Oct 4, 2024
Signed-off-by: Markus Theil <theil.markus@gmail.com>
openssl_3 is no longer unambiguous with openssl_3_2 and openssl_3_3 also
present. Rename to openssl_3_0.

Signed-off-by: Markus Theil <theil.markus@gmail.com>
Signed-off-by: Markus Theil <theil.markus@gmail.com>
@thillux thillux force-pushed the mtheil/staging-next-fixes-2024-10-02 branch from 56d732c to f962382 Compare October 5, 2024 09:10
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 5, 2024
@thillux
Copy link
Contributor Author

thillux commented Oct 5, 2024

Why not a more specific version? Honestly I'm surprised that right now openssl_3 points to openssl_3_0 and openssl points to openssl_3_3.

EDIT: it's how confusingly we've setup the openssl names, I'm afraid.

I nevertheless adapted this and removed openssl_3. As it is not clear if openssl_3 should point to the oldest or newest version of 3.x. openssl_3_0 and openssl_3_3 are more explicit in this now.

@vcunat

@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package labels Oct 5, 2024
@vcunat
Copy link
Member

vcunat commented Oct 5, 2024

Please don't. Right now openssl_3_0 is defined as a deprecated alias, unfortunately :-/ Therefore some setups won't see that attribute! (allowAliases = false; nixpkgs config)

@vcunat
Copy link
Member

vcunat commented Oct 5, 2024

I'd prefer this state fixed, but this particular PR isn't a good place, I believe.

@K900
Copy link
Contributor

K900 commented Oct 5, 2024

Signed-off-by: Markus Theil <theil.markus@gmail.com>
@thillux
Copy link
Contributor Author

thillux commented Oct 5, 2024

I'd prefer this state fixed, but this particular PR isn't a good place, I believe.

Ok, I'll remove it again.

@K900
Copy link
Contributor

K900 commented Oct 5, 2024

I definitely prefer the result of this PR to the status quo, FWIW.

@vcunat
Copy link
Member

vcunat commented Oct 5, 2024

I'd separate that. And maybe it's better done in master instead of staging-next.

@emilazy
Copy link
Member

emilazy commented Oct 7, 2024

I am not sure if it makes sense to have package name churn because my hope is that we can drop 3.0 in 25.05, given that the only issues with 3.3 have been too‐small RSA keys in tests and weird build systems like EDK2 that are poking at internal headers because they expect vendored sources.

vcunat pushed a commit that referenced this pull request Oct 7, 2024
Signed-off-by: Markus Theil <theil.markus@gmail.com>
Picked from PR #345998
@maralorn
Copy link
Member

maralorn commented Oct 7, 2024

We generally prefer to not clutter nixpkgs with patches but use pkgs.fetchpatch in Haskell land.
(Genuinely uncertain whether that is unusual in nixpkgs.)

vcunat pushed a commit that referenced this pull request Oct 7, 2024
Signed-off-by: Markus Theil <theil.markus@gmail.com>
Picked from PR #345998

In particular, this fixes OVMF build (a channel blocker).
https://hydra.nixos.org/build/274346247/nixlog/1/tail
@vcunat
Copy link
Member

vcunat commented Oct 7, 2024

I think that's general, but I don't expect there's anywhere to fetch this from yet. BTW, I pushed that commit to staging-next already. (Also the edk2 commit, as it would be -small channel blocker and I heard no objection to that part.)

@maralorn
Copy link
Member

maralorn commented Oct 7, 2024

Yeah, I noticed that commit, that’s why I came here. Its not a big issue, just wanted to remark it.

The patch could be fetched from here:

https://github.com/snapframework/openssl-streams/commit/ae9f8db059b6093092f6bd5155d55557c0892a4e.patch

@emilazy
Copy link
Member

emilazy commented Oct 7, 2024

@maralorn I also used to have this preference, but Alyssa pointed out to me that if an unmerged pull request is force‐pushed (say, because of upstream review feedback), GitHub will sometimes garbage‐collect the old commit and break future Nixpkgs reproducibility (you can look at random old Nixpkgs PRs and see 404s on the previous commit hashes from force‐pushes). This is sort of a general problem with FODs, but it made me lean more towards vendoring patches that aren’t available in a more stable place (upstream commit, another distro’s repository, etc.).

@emilazy
Copy link
Member

emilazy commented Oct 7, 2024

About the PR, though, I think that removing 3.2 should be uncontroversial but it could probably use a compatibility alias since it’s already seen a release. openssl_3_0 is a better name than we have, but I would personally prefer to work on fixing EDK2 so we can just drop it; have we confirmed that those existing pins don’t build fine with an unpinned OpenSSL?

@maralorn
Copy link
Member

maralorn commented Oct 7, 2024

@maralorn I also used to have this preference, but Alyssa pointed out to me that if an unmerged pull request is force‐pushed (say, because of upstream review feedback), GitHub will sometimes garbage‐collect the old commit and break future Nixpkgs reproducibility.

I was aware of that problem. But I thought that can be fixed by modifying the URL to not mention the pull request. Do even the simple /commit/ urls go dead?

@emilazy
Copy link
Member

emilazy commented Oct 7, 2024

#123456f525eb3 is 404, sadly.

@vcunat
Copy link
Member

vcunat commented Oct 7, 2024

It's a property of git that unreachable commits are meant to be forgotten (eventually).

@emilazy
Copy link
Member

emilazy commented Oct 7, 2024

Yeah, though IMO the rebase‐oriented GitHub code review flow means that previous versions of a PR should not be considered unreachable. There’s links directly to them from the PR pages that are 404s. Garbage collection shouldn’t lead to dangling pointers :)

But it is what it is so it’s worth keeping in mind.

vcunat pushed a commit that referenced this pull request Oct 8, 2024
Signed-off-by: Markus Theil <theil.markus@gmail.com>

Picked from PR #345998
except that vcunat used `openssl` instead of `openssl_3_3`

I do think that we should be well covered with 3.0 and 3.3.
https://github.com/openssl/openssl/blob/openssl-3.3.0/NEWS.md
@thillux thillux closed this Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: haskell 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: clean-up 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants