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

Update Gemspec Runtime Dependency to allow omniauth v2 Release from Jan 2021 #9

Closed
pboling opened this issue Apr 1, 2021 · 13 comments

Comments

@pboling
Copy link
Contributor

pboling commented Apr 1, 2021

@NickMeves @jcmuller @tfrey7 @dianaliu @mtoneil @bouffy @tdphillipsjr @acaloiaro @hugomarcotte @evan-duncan @Adam262 @kbruccoleri @jooshbzm @Ataraxic

This is not a good look for @grnhse (Greenhouse).
This needs to be addressed. It is a security issue.
Please also see #8

Please also note that this gem is dead as of now. It is incompatible with the current major version of omniauth, and is potentially a security risk. There are frequently CVEs filed against the omniauth family of gems (e.g. CVE-2015-9284, CVE-2020-26254, CVE-2020-15240), which in turn forces upgrades.

As this gem is incompatible with latest version it forces use of older, CVE-vulnerable, versions of the omniauth suite.

Bundler could not find compatible versions for gem "omniauth":
  In Gemfile:
    omniauth (~> 2.0.0)

    omniauth-greenhouse was resolved to 1.3.1, which depends on
      omniauth (>= 1.3.1, < 2)

Putting the community at risk

Sites with multiple authentication sources are now stuck between a rock and a hard place. They are not able to keep the "official" greenhouse integration, and also be secure, thanks to this line, which prevents the use of the latest, fixed, omniauth:

  spec.add_runtime_dependency 'omniauth', '>= 1.3.1', '< 2'

90% of these patches are to lock the version dependency, and that is a terrible precedent to set.

Screenshot 2020-04-13 13 37 11

What is next?

I tried to help, but it has been a year now, so I am removing our Greenhouse integration. I hope to see this situation improve.

FWIW, I am the primary maintainer of omniauth-identity, a sibling in this family. In that regard, one option may be to transfer the maintenance of this gem over to the Omniauth organization.

This current state is unacceptable. If you aren't going to maintain this gem, or transfer ownership, then please state as much in the readme, and archive the project.

@NickMeves
Copy link

Hi there,

Do you have any samples of CVEs on the omniauth gem specifically? I'm not seeing any on their GitHub repo, especially not any governing users to make the major version bump to v2 that released 3 months ago. Is this more of a compatibility issue & less security, so you don't have 1 gem holding you back from upgrading another one due to a dated dependency chain?

Regarding #8 - I think we need more context on what broke

If I understand correctly, this PR from 2015 (https://github.com/omniauth/omniauth-oauth2/pull/70/files) changed how the callback_url is constructed, and that caused an incompatibility with Greenhouse's OAuth2 implementation?

I'm not a omniauth user myself, so correct me if I'm wrong, but I'm assuming callback_url is what it is using as the redirect_uri param in the authorization endpoint call to begin the OAuth2 flow that needs to be allowlisted?

If any of this discussion touches on live security issues, let's move it to security@greenhouse.io and out of this public forum.

@pboling
Copy link
Contributor Author

pboling commented Apr 2, 2021

Here is all the context you should need, copied from the description of #8

Fix for breaking change introduced in omniauth-oauth2 v1.4.0

I hope you can parse the links.

@pboling
Copy link
Contributor Author

pboling commented Apr 2, 2021

If I understand correctly, this PR from 2015 (https://github.com/omniauth/omniauth-oauth2/pull/70/files) changed how the callback_url is constructed, and that caused an incompatibility with Greenhouse's OAuth2 implementation?

That is correct. PR #8 moves the definition of the removed method into this gem. Very simple fix, and the same exact fix that many other gems in the family used.

@NickMeves
Copy link

NickMeves commented Apr 2, 2021

What is the redirect_url that the callback_url method is generating that isn't compatible with what you have allowlisted on the OAuth2 side (feel free to anonymize parts of the URL if needed)?

That as the aspect that is key to help understand the implications of the fix across the customer base.

Thanks!

@pboling
Copy link
Contributor Author

pboling commented Apr 2, 2021

Is this more of a compatibility issue & less security, so you don't have 1 gem holding you back from upgrading another one due to a dated dependency chain?

Correct. It is blocking my project from upgrading the following (some of which have CVEs):

Package Update Change
omniauth major ~> 1.9.1 -> ~> 2.0.0
omniauth-facebook major ~> 6.0.0 -> ~> 8.0.0
omniauth-github major ~> 1.4.0 -> ~> 2.0.0
omniauth-google-oauth2 major ~> 0.8.0 -> ~> 1.0.0

Suggestion: You should have a Ruby spec app that integrates with your API via this gem, and validates it.

@NickMeves NickMeves changed the title 🚨🚨This gem is a security risk!🚨🚨 Update Gemspec Runtime Dependency to allow omniauth v2 Release from Jan 2021 Apr 2, 2021
@NickMeves
Copy link

Dropping context for watchers:

Omniauth v2.0.0 Release: https://github.com/omniauth/omniauth/releases/tag/v2.0.0

Release content contains guidance around mitigations for https://nvd.nist.gov/vuln/detail/CVE-2015-9284 if you don't jump to v2

@pboling
Copy link
Contributor Author

pboling commented Apr 2, 2021

It's worth noting that both this gem and omniauth-oauth2 are in violation of the OAuth 2.0 spec, but it can be fixed (I think) by my PR #8 .

It's also worth noting that I am the primary maintainer of the oauth2 Ruby gem, but I am actually still learning about OAuth 2.0, and trying to muddle through the spec.

This is the comment from my PR #8, which probably should be revised.

    # Override callback URL for compatibility with omniauth-oauth2 >= 1.4,
     #   which by default passes the entire URL of the callback, including
     #   query parameters. Greenhouse fails validation because that doesn't match the
     #   registered callback as required in the Oauth 2.0 spec.
     # Refs:
     # https://tools.ietf.org/html/rfc6749#section-4.1.3
     # https://github.com/omniauth/omniauth-oauth2/commit/26152673224aca5c3e918bcc83075dbb0659717f
     # https://github.com/omniauth/omniauth-oauth2/pull/70

I recommend searching the spec for the term redirect_url. I based my inclusion of it on that and the changes I saw being made to many other omniauth family repos as a result of the breaking change in omniauth v1.4.0.

@NickMeves
Copy link

NickMeves commented Apr 2, 2021

I recommend searching the spec for the term redirect_uri. I based my inclusion of it on that and the changes I saw being made to many other omniauth family repos as a result of the breaking change in omniauth v1.4.0.

I'm familiar with the specifics of the OAuth2 spec, I just need some help understanding how omniauth and omniauth-oauth2 implement callback_url (I haven't dug into their codebases).

Seeing the differences in callback URLs that both of those generate in actual samples (master vs your PR) would be greatly helpful to understanding why redirect_url isn't working.

@pboling
Copy link
Contributor Author

pboling commented Apr 2, 2021

Oops, I meant to write redirect_url, not redirect_uri.

Here is the definition from the PR:

def callback_url
  options[:redirect_uri] || (full_host + script_name + callback_path)
end

Here is the previous definition that was removed by omniauth-oauth2

def callback_url
  full_host + script_name + callback_path
end

All the change does is return options[:redirect_uri] if it was provided by the authorization request. This is required by the OAuth 2.0 spec. The difference is very simple, and I am unable to give you any examples, as I have removed Greenhouse from our codebase.

Unfortunately, I don't have any more time to devote to this.

@tdphillipsjr
Copy link
Member

This PR has been released.

@ducthien1490
Copy link

Hi , I'm upgrading project and this gem is still a blocker when all other gems requires omniauth >= 2 except this one.
Did new release really fix the problem?

@pboling
Copy link
Contributor Author

pboling commented Aug 2, 2022

@ducthien1490 See https://github.com/grnhse/omniauth-greenhouse#important-note

@ducthien1490
Copy link

@pboling : Thanks, we decided to remove this gem from source code

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

No branches or pull requests

4 participants