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

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

Open
hrbigelow opened this issue Aug 7, 2019 · 16 comments
Open

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

hrbigelow opened this issue Aug 7, 2019 · 16 comments
Labels
improvement Improve existing implementation

Comments

@hrbigelow
Copy link

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.

@hrbigelow hrbigelow added the improvement Improve existing implementation label Aug 7, 2019
@thespooler
Copy link
Contributor

thespooler commented Sep 24, 2019

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
Copy link
Contributor

thespooler commented Sep 28, 2019

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
Copy link
Owner

HeroicKatora commented Sep 28, 2019

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
Copy link
Owner

HeroicKatora commented Sep 28, 2019

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
Copy link
Contributor

thespooler commented Sep 29, 2019

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
Copy link
Contributor

thespooler commented Jan 14, 2020

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
Copy link
Owner

HeroicKatora commented Jan 14, 2020

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
Copy link
Owner

HeroicKatora commented Jan 14, 2020

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
Copy link
Contributor

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 added a commit to thespooler/oxide-actix-pcke that referenced this issue Jan 14, 2020
@thespooler
Copy link
Contributor

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
Copy link
Owner

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.

thespooler added a commit to thespooler/oxide-auth that referenced this issue Jan 28, 2020
As discussed in HeroicKatora#38, this fixes the incorrect implementation of Endpoint
for ErrorInto, missing a forward to the wrapped Endpoint, currently returning the
default implementation None.
thespooler added a commit to thespooler/oxide-auth that referenced this issue Apr 4, 2020
As discussed in HeroicKatora#38, this fixes the incorrect implementation of Endpoint
for ErrorInto, missing a forward to the wrapped Endpoint, currently returning the
default implementation None.
@VinceJuliano
Copy link

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
Copy link
Contributor

Geobert commented Jun 28, 2023

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
Copy link

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
Copy link
Contributor

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
Copy link

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/.

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

No branches or pull requests

6 participants