Skip to content

oauth2_proxy: 3.2.0 -> 5.1.0 #79840

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 22, 2020
Merged

oauth2_proxy: 3.2.0 -> 5.1.0 #79840

merged 4 commits into from
Apr 22, 2020
+525 −69

Conversation

knl
Copy link
Contributor

@knl knl commented Feb 11, 2020 •
edited
Loading

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.

Sorry, something went wrong.

@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 ehmry and yorickvP February 11, 2020 16:50
@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
Copy link
Member

@GrahamcOfBorg build oauth2_proxy

@knl
Copy link
Contributor Author

knl commented Mar 27, 2020

@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
Copy link
Member

@GrahamcOfBorg build oauth2_proxy

@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
Copy link
Member

@GrahamcOfBorg build oauth2_proxy

@knl
Copy link
Contributor Author

knl commented Apr 6, 2020

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
Copy link
Contributor

yorickvP commented Apr 9, 2020

Looks good to me, please add yourself as maintainer.

yorickvP
yorickvP requested changes Apr 9, 2020
View reviewed changes
Copy link
Contributor

@yorickvP yorickvP left a comment •
edited
Loading

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?

@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
Copy link
Contributor Author

knl commented Apr 20, 2020

@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 the maintainer of the package it changes label Apr 20, 2020
@basvandijk
Copy link
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
Copy link
Member

arianvp commented Apr 23, 2020 •
edited
Loading

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
Copy link
Contributor Author

knl commented Apr 23, 2020

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

@arianvp
Copy link
Member

arianvp commented Apr 23, 2020 •
edited
Loading

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
Copy link
Contributor Author

knl commented Apr 23, 2020

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
Copy link
Member

arianvp commented Apr 23, 2020

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

@flokli
Copy link
Contributor

flokli commented Apr 23, 2020 •
edited
Loading

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
pkgs/servers/oauth2_proxy/default.nix
version = "3.2.0";

version = "5.1.0";

goPackagePath = "github.com/pusher/${pname}";

src = fetchFromGitHub {
repo = pname;
owner = "pusher";
Copy link
Contributor

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.

Copy link
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
Copy link
Member

arianvp commented Apr 23, 2020

There is no CVE fix for 3.x

@yorickvP
Copy link
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

@yorickvP yorickvP

@ehmry ehmry

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 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@knl @basvandijk @yorickvP @arianvp @flokli