python3Packages.nassl: improve overridden openssl versions by risicle · Pull Request #144939 · NixOS/nixpkgs

risicle · 2021-11-06T22:28:48Z

Motivation for this change

Firstly override the correct respective base openssl version - this gives us an approximately appropriate set of patches and knownVulnerabilities to start with.

Filter patches that don't apply to the overridden source, fixing the build on darwin.

Add knownVulnerabilities for openssl 1.1.1h.

Would appreciate an aarch64-darwin tester as I think I might have fixed it there too.

Things done

firstly override the correct respective base openssl version - this gives us an approximately appropriate set of patches and knownVulnerabilities to start with. filter patches that don't apply to the overridden source, fixing the build on darwin.

thiagokokada

~~Is this a change related to fix macOS 🤔 ? Because I remember this package was working for x86_64-linux after this PR #144638.~~

~~If yes, wouldn't it be better to limit the openssl changes to macOS?~~

Edit: forget it, now I understand the changes.

thiagokokada

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

5 packages marked as broken and skipped:

python38Packages.nassl
python38Packages.sslyze
python39Packages.nassl
python39Packages.sslyze
sslyze

nix-build -A python3Packages.nassl
error: Package ‘openssl-1.0.2e’ in /home/thiagoko/Projects/nixpkgs/pkgs/development/libraries/openssl/default.nix:171 is marked as insecure, refusing to evaluate.


       Known issues:
        - Support for OpenSSL 1.0.2 ended with 2019.

       You can install it anyway by allowing this package, using the
       following methods:

       a) To temporarily allow all insecure packages, you can use an environment
          variable for a single invocation of the nix tools:

            $ export NIXPKGS_ALLOW_INSECURE=1

       b) for `nixos-rebuild` you can add ‘openssl-1.0.2e’ to
          `nixpkgs.config.permittedInsecurePackages` in the configuration.nix,
          like so:

            {
              nixpkgs.config.permittedInsecurePackages = [
                "openssl-1.0.2e"
              ];
            }

       c) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
          ‘openssl-1.0.2e’ to `permittedInsecurePackages` in
          ~/.config/nixpkgs/config.nix, like so:

            {
              permittedInsecurePackages = [
                "openssl-1.0.2e"
              ];
            }
(use '--show-trace' to show detailed location information)

risicle · 2021-11-06T23:04:33Z

Yeah :(

nixpkgs-review does obey NIXPKGS_ALLOW_INSECURE=1 to allow you to still review things though (in my experience).

thiagokokada · 2021-11-06T23:08:40Z

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

4 packages built:

python38Packages.nassl
python38Packages.sslyze
python39Packages.nassl
sslyze (python39Packages.sslyze)

thiagokokada

Result of nixpkgs-review pr 144939 run on x86_64-darwin 1

2 packages failed to build:

python39Packages.nassl
sslyze (python39Packages.sslyze)

2 packages built:

python38Packages.nassl
python38Packages.sslyze

Error log: http://ix.io/3Ef9

thiagokokada

Going to soft approve here since I think somebody else should also review the changes.

In my eyes the changes are good. At least the user will have to take a conscious decision of using a software that depends on insecure versions of OpenSSL.

risicle · 2021-11-07T00:22:57Z

@thiagokokada What macos version is this? Does it build from master on this machine?

thiagokokada · 2021-11-07T00:33:20Z

@thiagokokada What macos version is this? Does it build from master on this machine?

10.15.

master fails while trying to apply a patch (I assume on openssl). Error log: http://ix.io/3Efp.

risicle · 2021-11-07T00:47:24Z

Oh of course that's the bug I'm trying to fix.

Could it have been the "trying to build the python 3.8 and 3.9 versions at the same time and colliding on ports" darwin problem?

thiagokokada · 2021-11-07T00:47:25Z

Result of nixpkgs-review pr 144939 run on x86_64-darwin 1

4 packages built:

python38Packages.nassl
python38Packages.sslyze
python39Packages.nassl
sslyze (python39Packages.sslyze)

Seems like a flaky test maybe?

Oh of course that's the bug I'm trying to fix.

Could it have been the "trying to build the python 3.8 and 3.9 versions at the same time and colliding on ports" darwin problem?

Also possible.

SuperSandro2000 · 2021-11-07T01:13:23Z

pkgs/development/python-modules/nassl/default.nix

-  opensslStatic = (openssl.override nasslOpensslArgs).overrideAttrs (
+  opensslStatic = (openssl_1_1.override nasslOpensslArgs).overrideAttrs (
    oldAttrs: rec {
      name = "openssl-${version}";


Suggested change

name = "openssl-${version}";

pname = "openssl";

Nope, it needs name to fill out the download url.

The openssl package in nixpkgs already uses pname + version in the download url. Maybe we should update it or not. I am not sure. Marking as unresolved so that it is clear where I wrote a comment.

overrideAttrs needs to override name for the changes to be reflected. pname+version will be concatentated under name when invoked with stdenv.mkDerivation.

This is done at multiple places wrong. I think this should change internally because it is very unintuitive.

risicle · 2021-11-07T17:37:53Z

Unfortunately marking it with knownVulnerabilities stops ofborg from building it and that's my only way of testing it on aarch64.

thiagokokada · 2021-11-07T17:42:25Z

Unfortunately marking it with knownVulnerabilities stops ofborg from building it and that's my only way of testing it on aarch64.

You can use this trick to build to other architectures: https://nixos.wiki/wiki/NixOS_on_ARM#Compiling_through_QEMU

It is slow, but in this case I don't think it will take that much time.

risicle · 2021-11-09T20:41:05Z

Ok, I think this is a better state than previous, so am merging.

risicle · 2021-11-09T20:41:59Z

(I was going to try simply omitting the legacy openssl on aarch64 thinking it might be optional, but I don't think it is)

veehaitch · 2021-11-10T17:58:07Z

nassl is a Python library which was primarily created for sslyze which is an SSL/TLS security scanner. It relies on an outdated OpenSSL to perform insecure negotiations, algorithms, etc. I agree that we should make sure other packages do not depend on it without an explicit opt-in. Still, it would be nice to allow for using sslyze without the warning that this package is insecure—this seems like a false conclusion, at least for the case of sslyze.

risicle · 2021-11-10T18:14:07Z

The thing is, making any of those conclusions involves us making judgements ourselves what constitutes a vulnerability that is "ok". You mention that nassl inherently uses insecure connections to do its' job - sure - but some of the CVEs involved have potential consequences beyond "your crypto could be broken". What we would not want is people using a vulnerable nassl and scanning a malicious site only to end up with remote (er.. local in this case) code execution without having been warned. I don't think we have the resources to be able to rule out this possibility (and reassess every time a new openssl vulnerability is revealed).

I understand your reservations though and it did hurt a bit slapping this knownVulnerabilties on.

veehaitch · 2021-11-10T18:47:36Z

Fair enough, thanks for the quick reply!

risicle requested review from FRidh and jonringer as code owners November 6, 2021 22:28

github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Nov 6, 2021

ofborg bot requested a review from veehaitch November 6, 2021 22:40

ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Nov 6, 2021

thiagokokada reviewed Nov 6, 2021

View reviewed changes

thiagokokada approved these changes Nov 6, 2021

View reviewed changes

SuperSandro2000 reviewed Nov 7, 2021

View reviewed changes

risicle merged commit f614f94 into NixOS:master Nov 9, 2021

veehaitch mentioned this pull request Dec 14, 2021

python3Packages.sslyze: 4.1.0 -> 5.0.0 #150567

Merged

13 tasks

Uh oh!

Conversation

risicle commented Nov 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Things done

Uh oh!

thiagokokada left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiagokokada left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

risicle commented Nov 6, 2021

Uh oh!

thiagokokada commented Nov 6, 2021

Uh oh!

thiagokokada left a comment

Choose a reason for hiding this comment

Uh oh!

thiagokokada left a comment

Choose a reason for hiding this comment

Uh oh!

risicle commented Nov 7, 2021

Uh oh!

thiagokokada commented Nov 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

risicle commented Nov 7, 2021

Uh oh!

thiagokokada commented Nov 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SuperSandro2000 Nov 7, 2021

Choose a reason for hiding this comment

Uh oh!

risicle Nov 7, 2021

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 Nov 7, 2021

Choose a reason for hiding this comment

Uh oh!

jonringer Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

risicle commented Nov 7, 2021

Uh oh!

thiagokokada commented Nov 7, 2021

Uh oh!

risicle commented Nov 9, 2021

Uh oh!

risicle commented Nov 9, 2021

Uh oh!

veehaitch commented Nov 10, 2021

Uh oh!

risicle commented Nov 10, 2021

Uh oh!

veehaitch commented Nov 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

risicle commented Nov 6, 2021 •

edited

Loading

thiagokokada left a comment •

edited

Loading

thiagokokada left a comment •

edited

Loading

thiagokokada commented Nov 7, 2021 •

edited

Loading

thiagokokada commented Nov 7, 2021 •

edited

Loading