working auth filter rejection with authentication header and realm name #751

mike-kfed · 2020-11-18T10:23:25Z

This is for issue #62 based on pr #716 but mine actually works. However I understand it is a crazy hack how I pass the realm name along to create the additional header. If some maintainer could guide me how to make that clean please? I'll fix it as needed.

For cleaner implementation: I'd assume we need a new rejection type. Because a failed authentication (i.e. user/pass wrong) will require the same headers returned as this authentication header missing filter does, only now needed by another filter: the one checking the credentials.

FreeMasen · 2021-02-25T20:08:31Z

src/filters/auth.rs

+pub fn basic(
+    realm: &'static str,
+) -> impl Filter<Extract = (Authorization<Basic>,), Error = Rejection> + Copy {
+    header::header2().or_else(move |_| future::err(crate::reject::unauthorized(realm)))


I feel like it would be nicer to have the callback argument be Basic instead of Authorization

I don't follow what you mean here, can you please elaborate?

Something like the following

pub fn basic( realm: &'static str, ) -> impl Filter<Extract = (Basic,), Error = Rejection> + Copy { header::header2().map(|auth: Authorization<Basic>| auth.0).or_else(move |_| future::err(crate::reject::unauthorized(realm)))

thanks, okay I'll look into that!

I see how that helps now, much cleaner code afterwards :) should be okay now too

FreeMasen · 2021-02-25T20:09:28Z

examples/basic_auth.rs

+#[tokio::main]
+async fn main() {
+    let routes = warp::auth::basic("Realm name").map(|auth_header: Authorization<Basic>| {
+        // TODO: some password lookups done in here


It might be nice for the example to actually include a method for password lookups

well anything serious needs to pass in some form of database, file or at least a hashmap dragging in dependencies. easiest form is a silly if name == password check, but not sure that makes sense.

If the goal of an example is to illustrate how to use the filter, it would be good to provide something closer to a real world example instead of a todo comment. Using the dir example as a template, the project provides an actual path that is populated with files. When that example is run, it actually serves content, which helps to illustrate how to use that filter.

I'll add a mutex guarded Hashmap then, everything else needs 3rd party libs. Okay like that?

You may want to make it a Arc<tokio::sync::Mutex<HashMap<String, String>>> since almost any implementation is going to need to do some kind of async work. The tokio dependency will already be there from using tokio::main

I realised a lock-guard is not needed, since this is read-only. example should be fine now

My suggestion of using the tokio mutex was mostly to have some async work to perform in the handler, the new example is quite a bit more illustrative though

Sure I understand, but anything that needs async work (e.g. update of password db while server is running) is a way too big example. And showing use of a Mutex when lock-free is actually good enough is not a good example either ;)
however my example is broken, as it does not return a 401 on auth-error. but this is what my comment in the parent thread discusses, how to do this best 🤔

FreeMasen · 2021-02-25T20:13:24Z

src/reject.rs

+                let realm = e.downcast_ref::<String>().expect("realm must be string");
+                res.headers_mut().insert(
+                    "www-authenticate",
+                    HeaderValue::from_str(format!("Basic realm=\"{}\"", realm).as_str()).unwrap(),


The unauthorized response should be able to handle both Basic and Bearer replies at least. The full list of valid values here is 10 entries long, so some flexibility on the scheme would be nice.

Also according to MDN the realm could be omitted, though I am not sure it is wise to allow that

Given that what I've coded so far feels kinda hacky, supporting multiple schemes calls for an even cleaner implementation. Meaning for example coming up with a new rejection type as mentioned in my description of the PR? I'll try to add Bearer but I am not sure this is really readable code after without fundamental changes.

Updating the error type to not be a unit_error should simplify the realm and scheme extraction from the user's configuration.

On issue #62 there are a few files still available from the previous PR around an auth filter. I've updated their approach to the current master here which might be worth looking at.

thx for the feedback, I'll hack away :)

FreeMasen · 2021-02-25T20:15:28Z

src/reject.rs

@@ -509,6 +543,11 @@ unit_error! {
    pub UnsupportedMediaType: "The request's content-type is not supported"
 }

+unit_error! {


This is probably not a unit_error if the scheme is going to be provided

what other type of error is it then? I just tried make it return Unauthorized request, I am not a full pro of warps internals ;)

MissingHeader would be an error you could model this off of.

thx, will look into it

mike-kfed · 2021-02-26T09:07:40Z

While getting rid of unit-error and looking at your attempt with the Authorizer trait, I realized my example is broken. Here's a quick attempt to work around the limitation:

use headers::authorization::Basic;
use std::collections::HashMap;
use std::sync::Arc;
use warp::Filter;

async fn is_authenticated(
    auth_header: Basic,
    pwdb: Arc<HashMap<&str, &str>>,
) -> Result<String, warp::Rejection> {
    let user = auth_header.username();
    if pwdb.get(user) == Some(&auth_header.password()) {
        Ok(user.to_string())
    } else {
        Err(warp::reject::unauthorized("Basic", "Realm")) // stupid hack making unauthorized public
    }
}

#[tokio::main]
async fn main() {
    let pwdb: HashMap<_, _> = vec![
        ("alice", "wonderland"),
        ("bob", "cat"),
        ("carl", "IePai4ph"),
    ]
    .into_iter()
    .collect();
    let pwdb = Arc::new(pwdb); // no lock-guard needed, it's read-only
    let pwdb = warp::any().map(move || pwdb.clone());
    let routes = warp::auth::basic("Realm name")
        .and(pwdb.clone())
        .and_then(is_authenticated)
        .map(|user: String| format!("Hello, {} you're authenticated", user));

    warp::serve(routes).run(([127, 0, 0, 1], 3030)).await;
}

Another important thing, the CONTENT_TYPE header is hardcoded to "text/plain; charset=utf-8" in everything we do. this is another stumbling block for securing REST-APIs (especially when Bearer scheme is used this is a no-go, people will use this for json).

In my application I hacked around all those limitations making a custom rejection handler and more boilerplate. Would be nice to also allow for custom-content-type too while we are at it.

I like your Authorizer trait idea, seems more flexible than my simple and_then filter approach.

FreeMasen · 2021-02-26T15:29:18Z

Another important thing, the CONTENT_TYPE header is hardcoded to "text/plain; charset=utf-8" in everything we do. this is another stumbling block for securing REST-APIs (especially when Bearer scheme is used this is a no-go, people will use this for json).

I am not sure I follow this, the 401 reply is sent back with an empty body so the Content-Type of that reply is going to be plain/text. Any subsequent handler would be able to reply with whatever Content-Type they would like. Are you saying that you'd like configure the reply's body when the header is not found? If so, then adding a new Rejection would probably be a requirement that a user could .recover from.

I like your Authorizer trait idea, seems more flexible than my simple and_then filter approach.

I would be curious where you end up with this, I ultimately decided that this wasn't the right abstraction but it could have been my implementation.

mike-kfed · 2021-02-26T15:44:23Z

the problem arises when the authentication fails, then the warp reject code we are building here is active, correct? If that is the case you'll have a content-type of plain/text instead of potentially wanted json. Plus no control of output on auth-error, you might want to send something like {"error": "blah", "msg": "password expired"}. Sure if auth succeeds then whatever the user serves is correctly handled, since our filter here was passed.

Basically we need to be able to handle

no request-header came in
header came in but password/token wrong
header came in but invalid
header came in auth ok.

cases 1, 3 and 4 are easily doable with crate only code, number 2 is where the user likely wants to customise.

FreeMasen · 2021-02-26T16:19:17Z

header came in but password/token wrong

This would have to follow down the rejection path. It is a bit more boilerplate but I think allowing for rejections to be handled outside of the primary handler is a nice thing about this pattern. Here is a gist with the basic pattern:

https://gist.github.com/FreeMasen/c0ec97e03bfe93d3d703ee31a605bcbf

mike-kfed · 2021-02-26T22:31:27Z

thanks for the help, that is basically what I built for my app - I think it is a lot of boilerplate necessary to "just" have authentication. If we can find a way to hide that for the users of warp it'd be cooler. I'll return trying to find a nice solution on monday, no computer accessible this weekend :)

mike-kfed · 2021-03-01T10:17:09Z

I made my code a bit more readable and played with your example code.
Problem using the rejection handler is now

not providing user/pass returns Error 500 because it does not handle UnauthorizedChallenge rejections
if I were to add UnauthorizedChallenge handling, that struct would have to be public and not private.
- Let's suppose that it was public: we'd write redundant boiler plate returning the www-authenticate header with realm/scheme which is already in src/reject.rs.
- for browsers we need to return www-authenticate header and 401 if the password was wrong to make them popup the user/pass input, just 403 forbidden makes it impossible for the user to fix his error since the browser will keep sending the Authorization header. If this is done in the rejection handler, again redundant code that is already in src/reject.rs
using rej.into_response() in the reject handler doesn't work either because warp::reject::sealed::IsReject trait is private
problem I remember from my private project, if you add a websocket server to this you need to add e.g. MissingConnectionUpgrade handling, even more boilerplate just because authentication is of interest lots of other stuff needs to be handled too.

I think we need to figure out a better way for .recover() in general. I'd propose we polish up the filters::auth and reject modules, write a working example with all boilerplate needed, try to get that merged into master. And then open a new issue that discusses getting rid of the boilderplate? what do you think?

mike-kfed · 2021-03-01T10:45:14Z

quickhack for into_response() being private:

diff --git a/src/reject.rs b/src/reject.rs
index 05e4a86..e2cbb4c 100644
--- a/src/reject.rs
+++ b/src/reject.rs
@@ -337,6 +337,11 @@ impl Rejection {
             false
         }
     }
+
+    /// workaround for recover() handler
+    pub fn make_response(&self) -> hyper::Response<Body> {
+        self.into_response()
+    }
 }

 impl<T: Reject> From<T> for Rejection {

then your examples else branch is simply

let res = if let Some(inner) = rej.find::<AuthRejections>() {
// ...
} else {
    rej.make_response()
};

but this change goes way beyond this issues title I feel.

FreeMasen · 2021-03-01T20:00:03Z

* not providing user/pass returns Error 500 because it does not handle `UnauthorizedChallenge` rejections

My expectations is that the UnauthorizedChallenge wouldn't be a rejection but a reply that is already taken care of by the framework. If I instrument an endpoint with warp::auth::basic("MyRealm", |basic| { //... }) I want warp to send the 401 for me automatically. If that isn't something provided by the filter it really doesn't seem very useful since you can get the same via warp::header("Authorization")

mike-kfed · 2021-03-01T20:17:43Z

This is what I mean, as soon as you recover you have to deal with any rejection, the examples/rejections.rs even shows that you need to deal with 404 and everything. So yes right now you could build this yourself with warp::header("Authorization") no advantage using warp's library filters, which is the point I tried to make - a fundamental change of how this works is needed. (Or I am misunderstanding how . recover() is supposed to be used, my experience so far is it hands over any rejection in the route giving you responsibility to handle it yourself.)

FreeMasen · 2021-03-01T20:30:25Z

(Or I am misunderstanding how . recover() is supposed to be used

I think you are understanding how recover is supposed to be used. In the case that a route is defined using basic I would expect that an reply, not a rejection, would be sent by that filter to orchestrate the challenge. CORS preflight requests would be a similar example, an incoming OPTIONS request is not handled by the instrumented route, instead is replied to by warp under the hood.

Another important thing, the CONTENT_TYPE header is hardcoded to "text/plain; charset=utf-8" in everything we do. this is another stumbling block for securing REST-APIs

I believe this is still the point you are trying to make that I may be misunderstanding. Are you saying that you want to reply to an unauthorized request with some kind of custom body during the Unauthorized Challenge? If so, can you provide a little more detail about the use case you see as being a stumbling block? I would expect the body of the Unauthorized Challenge response to be empty making the content type not really important

mike-kfed · 2021-03-01T21:22:56Z

the issue is for browsers you need to send the 401 + www-authenticate header in 2 cases

the user has not provided any auth info
the user provided the wrong auth info, i.e. after a custom is_authenticated call

If the basic() filter returns a reply for case 1. that could be okay, but you need to then redundantly construct the reply for case 2. in your code since the basic() will not pass along the scheme+realm into the rejection handler (it's a reply now). Also, given that 404 and more is a rejection, as soon I want to custom handle the auth using rejections, you also have to custom handle 404, connectionupgrade and whatnot. When all you wanted to do is create and handle a certain type of rejections. We need a rejections "filter" to go before .recover() maybe, that auto-handles rejections into replies except the ones matching a certain pattern?

for the content-type issue, if you want to write a pure json only api, you should be able to serve only json without sometimes changing to text/plain. This can be done by copying what is being coded here and handling every rejection, so one could argue it's fine to write lots of boilerplate for this special usecase (even though it'd be a verbatim copy changing only the content-type). Something like nginx config types { } default_type "application/json"; to override would be cool to have (overriding content-types of whatever warp does internally).

For standard authentication without custom requirements beyond authentication/authorization I feel it is too much extra code to write and confusing. I wouldn't expect I have to handle 404 myself when I only want to handle auth myself and vice versa.

mike-kfed · 2021-03-01T21:32:55Z

I would expect the body of the Unauthorized Challenge response to be empty making the content type not really important

most webservers send a human readable string informing them auth is necessary, otherwise the browser window stays empty when the user clicks "cancel" in his auth-popup. But yeah, when the content is empty, you can also argue it is not necessary to send a content-type at all ;)

FreeMasen · 2021-03-01T21:39:17Z

the user provided the wrong auth info, i.e. after a custom is_authenticated call

I would expect this should lead to a 403 and not another 401 as defined here, also for reference

if you want to write a pure json only api,

I think this is a separate issue entirely. I could possibly see the inclusion of another Server level configuration or some kind of with(warp::default_content_type("json")) but in this case I would say worrying about the content-type is out of scope. Alternatively it may already be covered by the with_header fn

mike-kfed · 2021-03-01T21:51:58Z

I would expect this should lead to a 403 and not another 401 as defined here, also for reference

correct, I patched it to send empty body and no content-type now.

I think this is a separate issue entirely.

I agree.

Leaves us with how to best treat recover for rejections lowering the coding-load of auth implementations. My hack from above is basically the rejections-filter doing the default warp action if not handled myself. what do you think?

mike-kfed · 2021-03-02T09:33:39Z

CORS preflight requests would be a similar example, an incoming OPTIONS request is not handled by the instrumented route, instead is replied to by warp under the hood.

I just played with this, the cors filter also is implemented with rejections, difference is it's use of wrapsealed, however if your example code was extended with CORS this way, the handle_rejections also gets the CORS rejections to handle:

    let routes = warp::auth::basic("Realm name")
        .and(pwdb.clone())
        .and_then(is_authenticated)
        .map(|user: String| format!("Hello, {} you're authenticated", user))
        .with(cors)
        .recover(handle_rejections);

normally nobody notices that because they probably chain the .with() at the end of their route avoiding it by chance. So I guess the auth filter is implemented correctly, it's just a question of how to resolve issue #451

…hanks to FreeMasen example is not yet handling missing auth-headers correctly

FreeMasen · 2021-03-03T17:18:01Z

normally nobody notices that because they probably chain the .with() at the end of their route avoiding it by chance. So I guess the auth filter is implemented correctly, it's just a question of how to resolve issue #451

I feel like auth should probably be a with style filter as apposed to the normal route style filter (these should have better names...). As it stands I haven't been able to figure out how to do async work from a with style filter, either using the wrap_fn filter or trying to setup my own auth filters.

mike-kfed · 2021-03-04T09:46:40Z

I agree a with style filter would help, and it might be worth the effort to get one going. But in the end it is probably easier to make a public interface to convert a rejection into a impl Reply which is needed anyway for other use-cases too.

mike-kfed · 2021-12-25T14:33:41Z

Hello @FreeMasen I started a new branch for a with-filter called http_basic_auth_cb, based on PR #819 but allowing for a callback function to authenticate, filter code here basic.rs how to use it is shown here tests/auth_basic.rs

It'd be great if we can work together on making that work with an async callback? or discuss a different API for building an auth filter? Anyways please let me know what you think

working auth filter rejection with authentication header and realm name

7b357e4

FreeMasen mentioned this pull request Feb 17, 2021

Add Authorization header filters #62

Open

FreeMasen reviewed Feb 25, 2021

View reviewed changes

mike-kfed added 2 commits February 26, 2021 07:54

minimal example of password check using hashmap

8438c37

make callback argument cleaner

3082706

mike-kfed added 4 commits February 26, 2021 14:10

interim version where Unauthorized is not a unit_error

12320ef

prepared forbidden rejection, pseudo-use scheme

8e4f544

renamed challenge info to UnauthorizedChallenge

8ba686e

refactoring, moved challenge struct to auth

aacb845

mike-kfed added 2 commits March 1, 2021 10:37

use enum for authentication schemes

cef3c05

added bearer auth filter, commented alternative authentication schemes

caed929

no content or content-type for 401

b8f9f94

mike-kfed mentioned this pull request Mar 2, 2021

Unreasonably hard to recover internal errors from Rejection. #451

Open

better example for an async userdb check with locking requirements, t…

aad5a2f

…hanks to FreeMasen example is not yet handling missing auth-headers correctly

586837r mentioned this pull request Mar 16, 2021

Add warp::auth::basic() filter #819

Open

jtroo mentioned this pull request Mar 29, 2021

Improved rejections #712

Closed

working auth filter rejection with authentication header and realm name #751

Are you sure you want to change the base?

working auth filter rejection with authentication header and realm name #751

Conversation

mike-kfed commented Nov 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-kfed commented Feb 26, 2021

FreeMasen commented Feb 26, 2021

mike-kfed commented Feb 26, 2021

FreeMasen commented Feb 26, 2021

mike-kfed commented Feb 26, 2021

mike-kfed commented Mar 1, 2021 • edited Loading

mike-kfed commented Mar 1, 2021

FreeMasen commented Mar 1, 2021

mike-kfed commented Mar 1, 2021

FreeMasen commented Mar 1, 2021

mike-kfed commented Mar 1, 2021

mike-kfed commented Mar 1, 2021

FreeMasen commented Mar 1, 2021 • edited Loading

mike-kfed commented Mar 1, 2021 • edited Loading

mike-kfed commented Mar 2, 2021

FreeMasen commented Mar 3, 2021

mike-kfed commented Mar 4, 2021

mike-kfed commented Dec 25, 2021

mike-kfed commented Mar 1, 2021 •

edited

Loading

FreeMasen commented Mar 1, 2021 •

edited

Loading

mike-kfed commented Mar 1, 2021 •

edited

Loading