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

Add the ability to configure and provide Oauth2 #485

Merged
merged 1 commit into from
Jun 29, 2021

Conversation

Firstyear
Copy link
Member

Fixes #329 relates #278. This is the core implementation of Oauth2 for Kanidm. There are still a number of outstanding todos such as:

  • Resource Server display names
  • Scope mapping
  • Authorisation/Filtering of who can access what resource servers
  • Token introspection
  • The Web UI elements for a clean workflow
  • Open ID Connect
  • Book chapters and documentation of how to configure resource servers
  • [ x ] cargo fmt has been run
  • [ x ] cargo clippy has been run
  • [ x ] cargo test has been run and passes
  • [ - ] book chapter included (if relevant)
  • [ x ] design document included (if relevant)

@Firstyear Firstyear requested review from QnnOkabayashi, victorcwai and yaleman and removed request for QnnOkabayashi and victorcwai June 17, 2021 04:57
Copy link
Collaborator

@QnnOkabayashi QnnOkabayashi 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 still a little confused where Kanidm comes into the Oauth2 process. We're both the client and the server? Are we the Auth server but not the resource server? What do we do differently from the oauth2 crate?

kanidm_client/src/asynchronous.rs Outdated Show resolved Hide resolved
kanidm_client/src/asynchronous.rs Outdated Show resolved Hide resolved
kanidm_client/src/asynchronous.rs Outdated Show resolved Hide resolved
kanidm_client/tests/oauth2_test.rs Outdated Show resolved Hide resolved
kanidm_proto/src/oauth2.rs Show resolved Hide resolved
Comment on lines +1797 to +1922
if self.qs_write.get_changed_ouath2() {
self.qs_write
.get_oauth2rs_set(au)
.and_then(|oauth2rs_set| self.oauth2rs.reload(oauth2rs_set))?;
}
// Commit everything.
self.oauth2rs.commit();
self.pw_badlist_cache.commit();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait can you give a quick rundown on what pieces of Oauth2 this file implements? I watched some videos on it but am still very confused.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are an identity provider.

Consider an application like "gitter". When you go there is says "do you want to authenticate with github?". In this case the "resource server" is gitter, and kanidm would be "github", where we are providing identities and authorisation to external applications.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So ideally in the future there would be options like "Sign in with Kanidm"?

Copy link
Member Author

Choose a reason for hiding this comment

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

For resource servers, yes, they could add that option. More generically it's "sign in with openidconnect" or "sign in with oauth", which has more relevance inside an organisation that the public internet.

kanidmd/src/lib/lib.rs Show resolved Hide resolved
kanidmd/src/lib/plugins/oauth2.rs Show resolved Hide resolved
kanidmd/src/lib/server.rs Show resolved Hide resolved
kanidmd/src/lib/value.rs Show resolved Hide resolved
@QnnOkabayashi QnnOkabayashi mentioned this pull request Jun 18, 2021
5 tasks
@Firstyear
Copy link
Member Author

Firstyear commented Jun 21, 2021

I'm still a little confused where Kanidm comes into the Oauth2 process. We're both the client and the server? Are we the Auth server but not the resource server? What do we do differently from the oauth2 crate?

Right, oauth2 has three parts:

  • Client, the browser where a user is sitting
  • Resource Server, the system or resource you want to access
  • Identity Provider, the holder of who you are and decided if you have access.

Oauth2 is a system that allows a client, when accessing a resource server, to be redirected to the IDP to make authorisation decisions on behalf of the resource server.

In this way, Kanidm will be the IDP.

We also need to implement some client and resource server behaviour for testing the behaviour of our IDP.

The oauth2 crate is extremely "opinionated" and only implements the components to be a resource server. It is not possible to use it to be a IDP or Client. Additionally, even for our use as an RS, because it's so opinionated it makes it more complex to try and use it as it implements so much magic behind the scenes, that it's a lot simpler to just bypass that crate all together.

edit: you may want to look at https://github.com/kanidm/kanidm/blob/master/designs/oauth.rst for an extended discussion

Does that help?

Copy link
Contributor

@victorcwai victorcwai left a comment

Choose a reason for hiding this comment

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

Though I am no expert in the domain of OAuth, the core functionalities in this PR seems to be implemented correctly to me.
But I think we should add more comments for the OAuth-related functions. Also we could write down the functions/APIs call flow during a successful authentication session. I think that will make it easier to understand.

kanidm_client/tests/proto_v1_test.rs Show resolved Hide resolved
kanidmd/src/lib/idm/oauth2.rs Show resolved Hide resolved
kanidm_proto/src/oauth2.rs Show resolved Hide resolved
kanidmd/src/lib/modify.rs Show resolved Hide resolved
Copy link
Collaborator

@QnnOkabayashi QnnOkabayashi left a comment

Choose a reason for hiding this comment

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

Is there a way I can see what new changes have been applied since last review? It only shows me which files have changes, not which specific lines.

Comment on lines +119 to +126
let form_req = AccessTokenRequest {
grant_type: "authorization_code".to_string(),
code: code.to_string(),
redirect_uri: Url::parse("https://demo.example.com/oauth2/flow")
.expect("Invalid URL"),
client_id: None,
code_verifier: pkce_code_verifier.secret().clone(),
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this how AccessTokenRequests are being created outside of tests too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty much yeah :)

.await
.expect("Unable to decode AccessTokenResponse");

// Step 4 - inspect the granted token.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still todo? Isn't this the most important part?

Copy link
Member Author

Choose a reason for hiding this comment

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

No? Token inspection is another rfc and I after I merge this I plan to open a handful of follow up issues such as token introsection :)

Copy link
Member

@yaleman yaleman left a comment

Choose a reason for hiding this comment

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

I can't find any issues with this, but... that's a low bar :)

@Firstyear Firstyear merged commit 1de1b2d into master Jun 29, 2021
@Firstyear Firstyear deleted the 329-278-oauth-pkce branch June 29, 2021 04:23
@Firstyear
Copy link
Member Author

Thank you all for your reviews! I'm going to open some follow up issues now

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

Successfully merging this pull request may close these issues.

Oauth2 pkce
4 participants