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

[hrbigelow]

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]

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]

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]

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]

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]

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]

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]

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:

• The definition https://github.com/HeroicKatora/oxide-auth/blob/next/oxide-auth/src/frontends/simple/endpoint.rs#L119
• The implementation https://github.com/HeroicKatora/oxide-auth/blob/next/oxide-auth/src/frontends/simple/endpoint.rs#L474-L513
• Finally, forward an implementation of fn extension(&mut self) -> Option<&mut dyn Extension>

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

[HeroicKatora]

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]

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]

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]

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]

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]

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