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

Allow public clients to authenticate without client_secret #1031

Merged
merged 1 commit into from Mar 14, 2018

Conversation

@f3ndot
Copy link
Contributor

f3ndot commented Feb 10, 2018

Summary

This PR aims to solve #999 and comply with Section 8.5 of draft-ietf-oauth-native-apps-12:

   Secrets that are statically included as part of an app distributed to
   multiple users should not be treated as confidential secrets, as one
   user may inspect their copy and learn the shared secret.  For this
   reason, and those stated in Section 5.3.1 of [RFC6819], it is NOT
   RECOMMENDED for authorization servers to require client
   authentication of public native apps clients using a shared secret,
   as this serves little value beyond client identification which is
   already provided by the "client_id" request parameter.

Public vs Private Clients

The original OAuth 2 RFC makes a definition between public & private clients (Section 2.1). To this end, along with modeling industry best practice, Doorkeeper::Application now has a #confidential? attribute. The developer must decide when creating an application, whether it is to be a confidential or non-confidential client.

This allows Doorkeeper to behave differently when receiving an authentication request, depending on the identified client and its confidentiality.

This decision maintains security by forcing the presence of a client_secret when the developer intends their app only to be used in secure environments.

For backwards compatibility, an UPGRADING.md document should be written asking developers to generate a migration that adds the confidential column. To be safe by default, the default value/backfill should be a confidential app.

What's Next

I omitted some important work to keep this PR simple:

  1. There's no tracking in the Doorkeeper::AccessToken for whether or not it was created under authenticated means (confidential app) or just identified (public app). A future PR should do this so it is clear to the developer that that Access Token's application relationship is guaranteed or just claimed.
  2. We don't forbid certain grant types depending on the confidentiality of an app. For example, a public app cannot use Authorization Code (unless PKCE). Research on the RFC specs is required, to determine if/how we may prevent a public client from authenticating using a grant type intended for private apps and vice versa.
token = Doorkeeper::AccessToken.first
context "when client_secret absent" do
it "should not issue new token" do
expect do

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Parenthesize the param change { Doorkeeper::AccessToken.count } to make sure that the block will be associated with the change method call.


context "when client_secret incorrect" do
it "should not issue new token" do
expect do

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Parenthesize the param change { Doorkeeper::AccessToken.count } to make sure that the block will be associated with the change method call.

expect do
post password_token_endpoint_url(client: @client, resource_owner: @resource_owner)
end.to change { Doorkeeper::AccessToken.count }.by(1)
context "with non-confidential/public client" do

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Block has too many lines. [29/25]

app = FactoryBot.create :application
authenticated = Application.by_uid_and_secret(app.uid, app.secret)
expect(authenticated).to eq(app)
describe :by_uid_and_secret do

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Block has too many lines. [30/25]

@@ -0,0 +1,11 @@
class AddConfidentialToApplication < ActiveRecord::Migration[5.1]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Missing magic comment # frozen_string_literal: true.

@@ -1,4 +1,4 @@
class AddOwnerToApplication < ActiveRecord::Migration
class AddOwnerToApplication < ActiveRecord::Migration[4.2]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Missing magic comment # frozen_string_literal: true.

@@ -1,4 +1,4 @@
class CreateDoorkeeperTables < ActiveRecord::Migration
class CreateDoorkeeperTables < ActiveRecord::Migration[4.2]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Missing magic comment # frozen_string_literal: true.

@@ -1,4 +1,4 @@
class AddPasswordToUsers < ActiveRecord::Migration
class AddPasswordToUsers < ActiveRecord::Migration[4.2]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Missing magic comment # frozen_string_literal: true.

@@ -1,4 +1,4 @@
class CreateUsers < ActiveRecord::Migration
class CreateUsers < ActiveRecord::Migration[4.2]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Missing magic comment # frozen_string_literal: true.

@@ -48,7 +48,7 @@ def set_application
end

def application_params
params.require(:doorkeeper_application).permit(:name, :redirect_uri, :scopes)
params.require(:doorkeeper_application).permit(:name, :redirect_uri, :scopes, :confidential)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Line is too long. [98/80]

@f3ndot

This comment has been minimized.

Copy link
Contributor Author

f3ndot commented Feb 10, 2018

Screenshots of the views:

screen shot 2018-02-10 at 2 05 00 pm

screen shot 2018-02-10 at 2 02 20 pm

screen shot 2018-02-10 at 2 02 05 pm

@@ -48,7 +48,8 @@ def set_application
end

def application_params
params.require(:doorkeeper_application).permit(:name, :redirect_uri, :scopes)
params.require(:doorkeeper_application)
.permit(:name, :redirect_uri, :scopes, :confidential)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Place the . on the previous line, together with the method call receiver.

@@ -48,7 +48,8 @@ def set_application
end

def application_params
params.require(:doorkeeper_application).permit(:name, :redirect_uri, :scopes)
params.require(:doorkeeper_application)
.permit(:name, :redirect_uri, :scopes, :confidential)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2018

Place the . on the previous line, together with the method call receiver.

@f3ndot f3ndot force-pushed the f3ndot:support-app-confidentiality branch 2 times, most recently from 9cfaee6 to 6cadbc8 Feb 10, 2018
@f3ndot

This comment has been minimized.

Copy link
Contributor Author

f3ndot commented Feb 10, 2018

Ignoring the coverage decrease because the existing lines that became uncovered are guarded for Rails 5 only and the test envs that never reach it are 4.x

@f3ndot

This comment has been minimized.

Copy link
Contributor Author

f3ndot commented Feb 16, 2018

@nbulaj any comments on this work?

@nbulaj

This comment has been minimized.

Copy link
Member

nbulaj commented Feb 16, 2018

@f3ndot I will take a look at it a little bit later - currently I do not have much time

@f3ndot

This comment has been minimized.

Copy link
Contributor Author

f3ndot commented Feb 16, 2018

@nbulaj Take all the time you need, no rush.

@nbulaj nbulaj force-pushed the doorkeeper-gem:master branch 3 times, most recently from 14903a4 to 54e8345 Mar 6, 2018
@nbulaj

This comment has been minimized.

Copy link
Member

nbulaj commented Mar 7, 2018

Hi @f3ndot . Could you please sync this branch with the master and squash the commits to a single one? I will take a look at this this week.

@f3ndot

This comment has been minimized.

Copy link
Contributor Author

f3ndot commented Mar 7, 2018

👋 @nbulaj I'll take care of this Friday or on the weekend

@baxang

This comment has been minimized.

Copy link
Contributor

baxang commented Mar 8, 2018

Thanks for your work @f3ndot. I'm looking forward to this becoming available soon. I tried testing this on my application but had an issue with database migration (the new column was not recognised for some reason). I will give it another try.

Add Application#confidential

Add dummy migration for Application#confidential

Because Dummy app is now Rails 5.1, the old migrations' ancestor class needed to be explicitly the 4.2 variety.

Expose app confidentiality in views & controllers

Allow public applications to be found if secret is blank

Don't use #client_via_uid fallback on Password strategy

Since credentials will only contain UID when a public application is calling, the fallback method of finding by UID alone is dead code. Private apps are not allowed to be identified by UID alone.
@f3ndot f3ndot force-pushed the f3ndot:support-app-confidentiality branch from 6cadbc8 to de4618c Mar 11, 2018
@f3ndot

This comment has been minimized.

Copy link
Contributor Author

f3ndot commented Mar 11, 2018

@nbulaj all set!

if credentials
server.client
elsif parameters[:client_id]
server.client_via_uid

This comment has been minimized.

Copy link
@nbulaj

nbulaj Mar 11, 2018

Member

Is it OK that we removed client_via_uid and introduced only client?

This comment has been minimized.

Copy link
@f3ndot

f3ndot Mar 12, 2018

Author Contributor

I think so, because this method was originally returning a client if:

  1. Valid credentials aka good client_id and client_secret
  2. Only good client_id set

Because credentials now returns true if good client_id and client_secret or only good client_id set this private method is redundant. We can trust the server.client to be set when either (1) or (2) is satisfied.

@nbulaj nbulaj self-assigned this Mar 12, 2018
@nbulaj nbulaj merged commit 2825db5 into doorkeeper-gem:master Mar 14, 2018
2 of 3 checks passed
2 of 3 checks passed
hound Hound is busy reviewing changes...
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 98.172%
Details
@nbulaj

This comment has been minimized.

Copy link
Member

nbulaj commented Mar 14, 2018

Hi @f3ndot . I've added upgrade notes to the Migration from old versions doc and introduced generator for generating migration for projects with old Doorkeeper version.

f3ndot added a commit to f3ndot/doorkeeper that referenced this pull request Mar 14, 2018
nbulaj added a commit that referenced this pull request Mar 14, 2018
Add #1031 to news
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.