Skip to content

oauth2_proxy: 3.2.0 -> 5.1.0 - #79840

Merged
basvandijk merged 4 commits into
NixOS:masterfrom
knl:update-oauth2_proxy-to-5.0.0
Apr 22, 2020
Merged

oauth2_proxy: 3.2.0 -> 5.1.0#79840
basvandijk merged 4 commits into
NixOS:masterfrom
knl:update-oauth2_proxy-to-5.0.0

Conversation

@knl

@knl knl commented Feb 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor
Motivation for this change

oauth2_proxy is now at version 5.0.0, and it brings support for many new providers, as well as security fixes. This PR updates both the package as well as the NixOS module.

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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@knl
knl force-pushed the update-oauth2_proxy-to-5.0.0 branch 2 times, most recently from 133f408 to 6e48a34 Compare February 11, 2020 15:37
@ofborg ofborg 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 Feb 11, 2020
@ofborg
ofborg Bot requested review from a user and yorickvP February 11, 2020 16:50
@ofborg 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 Feb 11, 2020
@knl
knl force-pushed the update-oauth2_proxy-to-5.0.0 branch from 6e48a34 to ed35189 Compare March 12, 2020 12:17
@knl
knl marked this pull request as ready for review March 12, 2020 12:29
@knl knl changed the title Update oauth2_proxy to 5.0.0 oauth2_proxy: 3.2.0 -> 5.0.0 Mar 12, 2020
@basvandijk

basvandijk commented Mar 12, 2020

Copy link
Copy Markdown
Member

@GrahamcOfBorg build oauth2_proxy

@knl

knl commented Mar 27, 2020

Copy link
Copy Markdown
Contributor Author

@ehmry @yorickvP could you please take a look

@knl
knl force-pushed the update-oauth2_proxy-to-5.0.0 branch from ed35189 to d268d4f Compare April 6, 2020 07:54
@basvandijk

basvandijk commented Apr 6, 2020

Copy link
Copy Markdown
Member

@GrahamcOfBorg build oauth2_proxy

@ofborg ofborg Bot added the 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. label Apr 6, 2020
@knl
knl force-pushed the update-oauth2_proxy-to-5.0.0 branch from d268d4f to c965ff4 Compare April 6, 2020 10:59
@knl knl changed the title oauth2_proxy: 3.2.0 -> 5.0.0 oauth2_proxy: 3.2.0 -> 5.1.0 Apr 6, 2020
@knl
knl force-pushed the update-oauth2_proxy-to-5.0.0 branch from c965ff4 to cc57c95 Compare April 6, 2020 11:00
@basvandijk

basvandijk commented Apr 6, 2020

Copy link
Copy Markdown
Member

@GrahamcOfBorg build oauth2_proxy

@knl

knl commented Apr 6, 2020

Copy link
Copy Markdown
Contributor Author

Waiting for #83689 to land on master, as it contains a fix for go 1.14 on older kernels (which are the ones used by grahamcofbog builders).

@knl
knl force-pushed the update-oauth2_proxy-to-5.0.0 branch from cc57c95 to 6b19b4c Compare April 9, 2020 12:18
@yorickvP

yorickvP commented Apr 9, 2020

Copy link
Copy Markdown
Contributor

Looks good to me, please add yourself as maintainer.

yorickvP
yorickvP requested changes Apr 9, 2020
View reviewed changes

@yorickvP yorickvP left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm actually not sure about azure-tenant-string, could you confirm that?

Comment thread nixos/modules/services/security/oauth2_proxy.nix Outdated
Comment thread nixos/modules/services/security/oauth2_proxy.nix Outdated
Comment thread pkgs/servers/oauth2_proxy/default.nix Outdated
@knl
oauth2_proxy: 3.2.0 -> 5.1.0
6256d88 
v3.2.0 is 11 months old.

v5.1.0 brings new providers and fixes security vulnerabilities. In addition, the
project switched to go 1.14 and uses go modules, which is now reflected in the
build process.

NOTE: There are many breaking changes, which are then reflected in the NixOS
services configuration.
@knl
knl force-pushed the update-oauth2_proxy-to-5.0.0 branch from d0b6f04 to 7321a24 Compare April 20, 2020 07:40
knl added 3 commits April 20, 2020 10:11
@knl
oauth2_proxy: Update NixOS module
3c55184 
Update to match the current flags and apply fixes to all breaking changes.
@knl
oauth2_proxy: Start running tests for oauth2_proxy
3552e61 
No NixOS tests yet, but this is better than nothing.
@knl
oauth2_proxy: Add knl as a maintainer
d31bb1e 
Per request from the current maintainer.
@knl
knl force-pushed the update-oauth2_proxy-to-5.0.0 branch from 7321a24 to d31bb1e Compare April 20, 2020 08:11
@knl

knl commented Apr 20, 2020

Copy link
Copy Markdown
Contributor Author

@yorickvP Please take another look.

@ofborg
ofborg Bot requested a review from yorickvP April 20, 2020 13:56
@ofborg ofborg Bot added the 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. label Apr 20, 2020
@basvandijk

basvandijk commented Apr 21, 2020

Copy link
Copy Markdown
Member

@GrahamcOfBorg build oauth2_proxy

@basvandijk
basvandijk merged commit 784aa29 into NixOS:master Apr 22, 2020
@knl
knl deleted the update-oauth2_proxy-to-5.0.0 branch April 22, 2020 10:15
@arianvp

arianvp commented Apr 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

I think we should backport removing OAuth2-proxy completely from NixOS 20.03. Given that the version with the security fix is backwards-incompatible with the version we ship (3.2.0)

i'm kind of uncomfortable that we're shipping a NixOS module that sets people up for open redirect attacks GHSA-qqxw-m5fj-f7gv

@knl

knl commented Apr 23, 2020

Copy link
Copy Markdown
Contributor Author

@arianvp should we then remove any software from NixOS that has a CVE?

@arianvp

arianvp commented Apr 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

One that defeats the purpose of the package in the first place? yes. Open redirect bypasses the security measures that the package was supposed to fix in the first place. an authentication modules that allows attackers to steal your credentials is not very useful to ship.

Im also ok with rewritng the NixOS module to support 5.0 but seems that is a big breaking change.

(So is removing but it's less work). I don't know what is wisdom here

@knl

knl commented Apr 23, 2020

Copy link
Copy Markdown
Contributor Author

I understand your sentiment. But all software has bugs, and we can either just accept there will be a breaking change, or drop it. The former can be handled nicely by documenting the change and the reason for introducing it in the first place (I suspect no one would be happy with running quite an old version of anything). The latter just opens a can of worms -- any CVE in majority of the tools is defeating the purpose of the tool.

@arianvp

arianvp commented Apr 23, 2020

Copy link
Copy Markdown
Member

Ok so in that case should we create a backport PR for this?

@flokli

flokli commented Apr 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

We should check if there's a CVE fix for 3.x versions, and backport this.

If there's no such fix, we should just mark it as insecure in 20.03 and 19.09. This will wake people up using it, so they can work on a fix ;-)

Edit: It might be just a matter of backporting oauth2-proxy/oauth2-proxy@a316f8a to our 3.2.0.

flokli
flokli reviewed Apr 23, 2020
View reviewed changes
Comment thread pkgs/servers/oauth2_proxy/default.nix

src = fetchFromGitHub {
repo = pname;
owner = "pusher";

@flokli flokli Apr 23, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This has been redirected to oauth2-proxy/oauth2-proxy.

@knl knl Apr 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it has been, but not official yet. I'd wait for them to make a release and then do the change. That is, Go code still uses pusher/oauth2-proxy, while only the repo is renamed.

@arianvp

arianvp commented Apr 23, 2020

Copy link
Copy Markdown
Member

There is no CVE fix for 3.x

@yorickvP

yorickvP commented Apr 26, 2020

Copy link
Copy Markdown
Contributor

We can backport the fix, it's a single-line patch.

@knl knl mentioned this pull request Apr 27, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flokli flokli flokli left review comments
@yorickvP yorickvP yorickvP approved these changes

Assignees

No one assigned

Labels

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/` 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@knl @basvandijk @yorickvP @arianvp @flokli