HeroicKatora / oxide-auth

A OAuth2 server library, for use in combination with actix or other frontends, featuring a set of configurable and pluggable backends.
689 stars 92 forks source link

Use PKCE extension in examples/actix.rs (or other examples) #38

Open hrbigelow opened 5 years ago

hrbigelow commented 5 years ago

Project Improvement

Would it be possible to include the PKCE extension technique in the examples/actix.rs? I've been looking through the code and haven't figured out yet how to use PKCE with the code grant flow.

thespooler commented 5 years ago

I'm also trying to accomplish this. I've been tinkering with using an Extended<Generic<...>> endpoint, but I keep stumbling with the with_solicitor block, which looks like this, self being the state and self.endpoint being Extended<Generic<>>:

    pub fn with_solicitor<'a, S>(
        &'a mut self,
        solicitor: S,
    ) -> impl Endpoint<OAuthRequest, Error = WebError> + 'a
    where
        S: OwnerSolicitor<OAuthRequest> + 'static,
    {
        ErrorInto::new(Generic { 
            registrar: self.endpoint.registrar().unwrap(),
            authorizer: self.endpoint.authorizer_mut().unwrap(),
            issuer: self.endpoint.issuer_mut().unwrap(),
            solicitor: solicitor,
            scopes: Vacant,
            response: Vacant,
        })
    }

But rust complains that type annotations needed and that it cannot infer type for Request on self.endpoint.registrar() (on registrar). This stems from impl<Request, Inner, Ext> Endpoint for Extended<Inner, Ext>, but I can't quite figure out how to explicit the typing other than the return type that already explicitly defines it.

thespooler commented 5 years ago

I made a small demo that tries to show a pkce flow. It does compile, but oxide trips on the handlers access_extensions. I seem to be misunderstanding how to integrate PKCE extension on my endpoint. If anyone cares to help, heres my sample repo: https://github.com/thespooler/oxide-actix-pcke

On a side note, I altered the consent page so as to pass the state per issue #42

HeroicKatora commented 5 years ago

This is being actively tested (without any specific web frontend), maybe the structure of these can help you:

https://github.com/HeroicKatora/oxide-auth/blob/next/oxide-auth/src/endpoint/tests/pkce.rs

From a quick look it seems your repository sets state parameters on the post urls but does not include the extra parameters of the PKCE extension, code_challenge and code_challenge_method.

You may want to build the redirect target dynamically from the url of the GET to ensure that all parameters are copied accordingly.

HeroicKatora commented 5 years ago

Sidenote: Getting an error also means that the extension is being called, Pkce::required() rejects requests that do not have contain PKCE information. The default behaviour, were the extension not added, is to silently ignore any additional parameters as per spec.

thespooler commented 5 years ago

I did shamelessly copy the test case, up to the PkceSetup name and I did guess that it wasn't the PKCE implementation that was incorrect but my endpoint wrapper on top of it. I also did try to include the code_challenge and code_challenge_method params for the POST, without any success. I even tried with the challenge values from RFC 7636 Apendix B.

thespooler commented 4 years ago

This is being actively tested (without any specific web frontend), maybe the structure of these can help you:

https://github.com/HeroicKatora/oxide-auth/blob/next/oxide-auth/src/endpoint/tests/pkce.rs

From a quick look it seems your repository sets state parameters on the post urls but does not include the extra parameters of the PKCE extension, code_challenge and code_challenge_method.

You may want to build the redirect target dynamically from the url of the GET to ensure that all parameters are copied accordingly.

I should be passing all the required pkce POST values now.

I'm creating a new AddonList with Pkce:required when creating my endpoint within with_solicitor, is it possible that this isn't correct? It's noted in Extended (endpoint) that the wrapped endpoint extensions will be ignored: could it be that previous Pkce instances (or other extensions?) must be reused somehow? If so, how should I do that? From what I see, this might be hard with the actix actor owning the endpoint and AddonList not being Copy-able.

HeroicKatora commented 4 years ago

That's slightly embarrasing. Quite simply, ErrorInto forgets to forward the extensions of the underlying endpoint so that the whole configuration takes no effect. The good news is, you can write that wrapper yourself and it isn't particularly complicated. It's only utility wrappers. Basically, create a local copy of:

I'll make sure to fix this in an upcoming release.

HeroicKatora commented 4 years ago

Also a win for reproducibility, I would have taken long to guess any connection to ErrorInto had you not provided the full example code. So many other things outside to consider first..

thespooler commented 4 years ago

Darn, I should've catched that on my end too. I'll see if I cannot get that thing working now. Cheers 🥂 and thanks for the help

thespooler commented 4 years ago

Oh the joy! It works! See https://github.com/thespooler/oxide-actix-pcke for a sample of how to use PKCE with Actix along with a simple HTML page showing the interaction from the browser side.

On a side note, am I right in understanding that when an incorrect code_verifier is povided, the correct verifier cannot be used anymore? I tried doing a test (from JS, client-side) where an incorrect verifier is sent before sending the correct one, but the correct one never worked.

HeroicKatora commented 4 years ago

That is how I understood the RFC but it is not explicit about the error handling here. The PKCE reference says that an error must be returned to the first request while the security considerations for oauth state that an authorization code must be single-use. Furthermore, failing the PKCE challenge most likely means that a man-in-the-middle entity (e.g. another application on a shared communication device) tried to steal your token. Although the error information returned to the geniune client is opaque, failing could be part of detectection and analysis of such an attack.

VinceJuliano commented 3 years ago

Hello, I also seem to have gotten a basic PKCE example working with Rocket. My example is not as complete as the above actix one because it is only the backend, my frontend repo is seperate but I used essentially the javascript from https://github.com/thespooler/oxide-actix-pcke. I figured it may be helpful for someone looking to get Rocket working, can post more if anyone needs, thank you guys for the above info -

https://github.com/VinceJuliano/oxide-rocket-pkce/blob/main/src/main.rs

Geobert commented 1 year ago

I was wondering if I should hijack this thread or create a new one, and stick with the former.

I’m hitting a build error which doesn’t make sense to me and I can’t find what I’m missing.

let mut ext = AddonList::new();
ext.push_code(pkce_ext);
let ep = Extended::extend_with(self, ext); // self being an EndpointImpl which implements Endpoint
match AuthorizationFlow::prepare(ep) {
    Ok(flow) => flow,
    Err(_) => unreachable!(),
}

the error:

 --> src/oauth/endpoint.rs:121:42
    |
121 |         match AuthorizationFlow::prepare(ep) {
    |               -------------------------- ^^ the trait `oxide_auth_async::endpoint::Endpoint<_>` is not implemented for `oxide_auth::frontends::simple::extensions::Extended<oauth::endpoint::EndpointImpl<'_>, oxide_auth::frontends::simple::extensions::AddonList>`
    |               |
    |               required by a bound introduced by this call
    |
    = help: the trait `oxide_auth_async::endpoint::Endpoint<oxide_auth::frontends::simple::request::Request>` is implemented for `oauth::endpoint::EndpointImpl<'a>`
note: required by a bound in `oxide_auth_async::endpoint::authorization::AuthorizationFlow::<E, R>::prepare`
   --> /home/geobert/.cargo/registry/src/my-registry/oxide-auth-async-0.1.0/src/endpoint/authorization.rs:107:8

but I have

impl<'a> Endpoint<Request> for EndpointImpl<'a>

What am I missing here?

EDIT: I’m using oxide-auth-async and not oxide-auth Endpoint so that’s why… how to fix that is the next question EDIT2: I’ll open a new issue to discuss that

SorteKanin commented 2 months ago

Hi, just ran into this problem myself. I'd like to use the PKCE extension since it seems to be highly recommended, but I have no idea how to do so with oxide-auth. In general I find the documentation lacking (not blaming, I know it is hard, just stating the fact).

Would it be possible to add to the documentation how to do this? It seems like there is some progress above. I'm curious how the author intends for this to be used. I'm personally using axum with a custom Endpoint implementation.

thespooler commented 2 months ago

Did you take a look at my sample above? Basically, the only thing you need to change server-side is adding a PKCE extensions to the endpoint addons. See https://github.com/thespooler/oxide-actix-pcke/blob/8ec03551f884e6fa35048d8043c52fb766555446/src/pkcetest.rs#L68.

On the client side, there's more to do, but you can rip it off my sample: https://github.com/thespooler/oxide-actix-pcke/blob/8ec03551f884e6fa35048d8043c52fb766555446/static/index.html#L25. It's all about generating a code_challenge to add to your authorize request.

SorteKanin commented 2 months ago

I'm not using Generic, but a custom implementation of Endpoint, mostly because Generic is not Clone. Extended is also not Clone so I can't use that either. But in general it would be very helpful to see actual usage examples on https://docs.rs/oxide-auth/latest/oxide_auth/.