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

python2Packages.certifi: init at 2019.11.28 #127453

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

dotlambda
Copy link
Member

Motivation for this change

fixes #127423

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 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.

cc @tollb @NixOS/nixops-committers

@dotlambda
Copy link
Member Author

To be honest, I don't like providing an outdated version of certifi for Python 2 because we shouldn't ship outdated security-related stuff. Maybe it's better not to fix this and mark NixOps 1.7 as broken.
Also, I will unsubscribe from this pull request. Any necessary modifications can be force-pushed to my branch by @NixOS/nixops-committers who should also merge this pull request if they think it's the best solution or create a new one otherwise.
This has been the very last time I put any work into fixing some Python 2 package required by NixOps!

@dotlambda dotlambda added the 9.needs: maintainer Package or module needs active maintainers label Jun 19, 2021
@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 127453 at 304839a9 run on x86_64-linux 1

11 packages marked as broken and skipped:
  • gnuradio3_7
  • gnuradio3_7Packages.ais
  • gnuradio3_7Packages.gnuradio
  • gnuradio3_7Packages.gsm
  • gnuradio3_7Packages.limesdr
  • gnuradio3_7Packages.nacl
  • gnuradio3_7Packages.osmosdr
  • gnuradio3_7Packages.rds
  • natron
  • ocropus
  • rainbowstream
3 packages failed to build:
12 packages built successfully:
  • bitbucket-cli
  • buttersink
  • cloudmonkey
  • datadog-agent
  • euca2ools
  • fmbt
  • git-fast-export
  • mercurial_4
  • mx
  • nixops
  • tsung
  • zabbix-cli

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

@mweinelt mweinelt added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Jun 19, 2021
@mweinelt
Copy link
Member

This poses a certain security risk, so I decided to set knownVulnerabilities.

license = licenses.isc;
maintainers = with maintainers; [ ]; # NixOps team
knownVulnerabilities = [
"Python 2 supported for certifi ended in 2019 and shipping an outdated certificate store carries a non-negligble risk"
Copy link
Member

Choose a reason for hiding this comment

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

Can anyone describe an actual threat model based on “shipping an outdated certificate store”? I can't think of any that would actually be a problem for normal users, and non-normal users already have much worse to worry about.

IMO adding knownVulnerabilities when there are no actually known vulnerabilities only makes people more used to working around it, especially for a package as advertised as nixops, and so is a net negative for the security of NixOS users.

Now, if there were some specific known vulnerability (eg. “this CA that was there in 2019 was removed after having issued malicious certificates that can still be used”), I would totally understand this item; but IMO as currently described this is lacking motivation and it'd be better to have no knownVulnerabilities than this one.

Copy link
Contributor

@zeri42 zeri42 Jun 22, 2021

Choose a reason for hiding this comment

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

easy. Some CA get's their private key stolen and now the thief can issue new certificates. Alternatively (way more common) the CA is seen to be not trustworthy due to weird behavior or bogus certificates issued by it. CAs that are not trustworthy or compromised might issue certificates that can be used to stage successful man-in-the-middle attacks. This impacts all tls secured communication that uses a certificate store that includes this CA (passwords/downloads/...). Unless you have support for OCSP, revocation is done via two ways:

  1. certificate revocation lists (that are sparely used). In this case the issuing CA revokes the compromised CA and publishes a file that includes this information.
  2. removing the CA from certificate stores like this one. The only way to handle compromised self-signed CA.
    Hence keeping the certificate store up to date is important.

For reference: https://sslmate.com/certspotter/failures

Copy link
Member

Choose a reason for hiding this comment

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

Hm, what about re-using the bundle from the cacert package instead? This would not only solve this issue but also would consolidate the CA bundle to a single location. For example when writing integration tests involving Python software, one usually needs to not only override cacert but also python*Packages.certifi to inject testing certificates.

Copy link
Contributor

Choose a reason for hiding this comment

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

The approach suggested by @aszlig may also be applicable to the boto 2.49.0 package, as I believe it has an embedded cacert file that is even further out-of-date: https://raw.githubusercontent.com/boto/boto/2.49.0/boto/cacerts/cacerts.txt. I'm not certain when this embedded cacerts.txt is used by boto versus the one provided by python2Packages.certifi.

If someone can confirm that boto's embedded cacerts.txt is actually used (especially through nixops 1.7), we might want to add a knownVulnerabilities to the boto package as well -- for consistency with python2Packages.certifi. Or, provide updated certificates in both packages. I took a quick look at arch and debian to see if they had updated the certs in their boto 2.49.0 packages, and it doesn't seem that they have. So, maybe I am mistaken.

Copy link
Member

Choose a reason for hiding this comment

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

@zeri42 Thank you, your link confirms that as of today there is no known instance of a CA having an issue that would warrant the knownVulnerabilities toggle since the version of certifi that is being considered here, and so that reverting updates should not be a security vulnerability :) (though it is only one source, of course)

(And also, even if they were in most cases I'd keep my position: an attacker stealing the private key from a CA is not a reasonable threat model for a normal NixOS user, which is the reason why I wrote my first message)

This being said I'm going to step out of this discussion: I'm only focussed on “knownVulnerabilities here would be bad”; there are for sure much better things that could be done by improving stuff in various ways :)

@nlewo
Copy link
Member

nlewo commented Jun 21, 2021

@dotlambda Note this would also fixes the python2Packages.requests library which depends on certifi.

@dpausp
Copy link
Contributor

dpausp commented Jun 24, 2021

We could also use a recent version of certifi and disable the test for the contents() function. The function is broken but it didn't even exist on 2019.11.28. This wouldn't break existing Python 2.7 code where the new function has never worked. Ugly, but having newer certificates is better, I think.

@tomfitzhenry
Copy link
Contributor

This will also fix buttersink failing to build: #128151.

@prusnak
Copy link
Member

prusnak commented Jun 29, 2021

This will also fix gnuradio 3.7 shipped with nixos-21.05: #128620

devhell added a commit to openlab-aux/vuizvui that referenced this pull request Jun 30, 2021
It's not clear when the `certifi` PR[1] will be merged, and I don't want
to have to wait for a new hydra build because of it. `nixopsUnstable` is
unusable in its current from due to almost lacking documentation, and so
I have little choice but to remove `nixops` right now until `certifi` is
merged.

[1]:NixOS/nixpkgs#127453
@adisbladis
Copy link
Member

@dotlambda I took the liberty of reworking this PR a bit.
We now use 2019.11.28 for building the package but inherit the cert bundle from the maintained python3-only version.

I think this is a decent solution with minimal maintenance.

@prusnak
Copy link
Member

prusnak commented Jul 2, 2021

@adisbladis I wonder whether the package version of this hybrid should be 2019.11.28 (source code version as reported by __init__.py) or 2021.05.30 (the version of cacert.pem)

@dotlambda
Copy link
Member Author

The version should be 2019.11.28 because the API has since been changed.

@adisbladis
Copy link
Member

Alright, version set as 2019.11.28.

We use the sources from 2019.11.28 to build, but inherit the cert bundle
from the maintained Python3-only version of certifi.

Co-authored-by: adisbladis <[email protected]>
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jul 3, 2021
@clefru
Copy link
Contributor

clefru commented Jul 3, 2021

clefru@57e65ad has a commit that patches Python 2 support back into a recent certifi. Works for me so far (with nixops)

@adisbladis
Copy link
Member

@clefru I thought about adopting a similar approach but opted against it because of the added maintenance burden.

I think for now I'll just merge this and if you think your approach is sufficiently better could you please create a separate PR?

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2021

Successfully created backport PR #129374 for release-21.05.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 6.topic: python 8.has: package (new) This PR adds a new package 9.needs: maintainer Package or module needs active maintainers 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nixops-1.7: no longer builds on master and 21.05