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

oauth2_proxy: 3.2.0 -> 5.1.0 #79840

Merged
merged 4 commits into from Apr 22, 2020
Merged

Conversation

knl
Copy link
Contributor

@knl knl commented Feb 11, 2020

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 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.

Copy link
Contributor

@yorickvP yorickvP left a comment

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?

nixos/modules/services/security/oauth2_proxy.nix Outdated Show resolved Hide resolved
nixos/modules/services/security/oauth2_proxy.nix Outdated Show resolved Hide resolved
pkgs/servers/oauth2_proxy/default.nix Outdated Show resolved Hide resolved
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
Update to match the current flags and apply fixes to all breaking changes.
No NixOS tests yet, but this is better than nothing.
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.

@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

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

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

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants