Create authentication request guard #17

notplants · 2021-11-08T15:49:30Z

notplants commented

2021-11-08 15:49:30 +00:00

In this PR, I added an authentication request guard, and added it to all routes, based largely off of this tutorial https://medium.com/@james_32022/authentication-in-rocket-feb4f7223254.

I learned that cookies.get_private and cookies.add_private, are secure methods which rocket has implemented, such that these values cannot be inspected, tampered with, or manufactured by clients.

This means that we can simply store a user_id, or any value in a private cookie like this, and if a user request has this cookie, we know that we have authenticated them.

The implementation of the RequestGuard, in authentication.rs, looks like this:

#[rocket::async_trait]
impl<'r> FromRequest<'r> for Authenticated {
    type Error = LoginError;

    async fn from_request(req: &'r Request<'_>) -> request::Outcome<Self, Self::Error> {
        let authenticated = req
            .cookies()
            .get_private(AUTH_COOKIE_KEY)
            .and_then(|cookie| cookie.value().parse().ok())
            .map(|_value: String| { Authenticated { } });
        match authenticated {
            Some(auth) => {
                request::Outcome::Success(auth)
            },
            None => {
                request::Outcome::Failure((Status::Forbidden, LoginError::UserNotLoggedIn))
            }
        }
    }
}

Then in any route, you can simply add an argument auth: Authenticated, like this:

#[get("/api/v1/ping")]
pub fn ping_pong(auth: Authenticated) -> Value {

and then that route is now only accessible to logged in users.

If the user is not logged in, it will redirect them to the /login page.

The way I've currently implemented things, I actually just used all the code from the http-basic-auth before, but just substituted nginx auth with this cookie auth.

In the current form, there is still no sql or database integration. I store a hash of the password in config.yml, along with the other config variables.

Of course this can be up for debate, but I initially did this because:

its not clear that PeachCloud needs multiple users with independent logins
its kind of a nice lean feature that all the state of the app is in that one config.yml file
its just a hash of a password, so it should be just as secure as if the password were stored somewhere else
it was not much work to implement
we are still so far achieving the needed functionality without needing to manage a database with migrations

My config.yml currently looks like this:

external_domain: ""
dyn_domain: ""
dyn_dns_server_address: ""
dyn_tsig_key_path: ""
dyn_enabled: false
ssb_admin_ids: []
admin_password_hash: 3c255775632b05b1194107f9ac8b40f9d498720c70536a3f90be2686b31d1b67
temporary_password_hash: 75f40ca05158fb1b990ab2c7bcd29081ee70fd4c25010a952c6b08cac6fb5754

On the downside, these hashes could be confusing without an explanation. But maybe comments in the yml could help with that.

Since I was able to keep the same code as before,
this also still includes the same logout, change password, and password reset features (via ssb dm), but now with the portability of not needing to alter nginx configurations (and so could run in an environment like yunohost, where nginx configs are handled separately from the app itself), and a prettier login screen (no weird http basic auth, instead its the nice login template you already made).

Another nice thing, is if we still decide we want to add traditional sql-based multi-user auth in the future, we could still use this request guard, and just change its implementation.

My initial thought would be to keep this for now, and then if at somepoint we want to go further with sql and multi-user auth, do that later in a separate PR.

In this PR, I added an authentication request guard, and added it to all routes, based largely off of this tutorial https://medium.com/@james_32022/authentication-in-rocket-feb4f7223254. I learned that cookies.get_private and cookies.add_private, are secure methods which rocket has implemented, such that these values cannot be inspected, tampered with, or manufactured by clients. This means that we can simply store a user_id, or any value in a private cookie like this, and if a user request has this cookie, we know that we have authenticated them. The implementation of the RequestGuard, in authentication.rs, looks like this: ``` #[rocket::async_trait] impl<'r> FromRequest<'r> for Authenticated { type Error = LoginError; async fn from_request(req: &'r Request<'_>) -> request::Outcome<Self, Self::Error> { let authenticated = req .cookies() .get_private(AUTH_COOKIE_KEY) .and_then(|cookie| cookie.value().parse().ok()) .map(|_value: String| { Authenticated { } }); match authenticated { Some(auth) => { request::Outcome::Success(auth) }, None => { request::Outcome::Failure((Status::Forbidden, LoginError::UserNotLoggedIn)) } } } } ``` Then in any route, you can simply add an argument `auth: Authenticated`, like this: ``` #[get("/api/v1/ping")] pub fn ping_pong(auth: Authenticated) -> Value { ``` and then that route is now only accessible to logged in users. If the user is not logged in, it will redirect them to the /login page. The way I've currently implemented things, I actually just used all the code from the http-basic-auth before, but just substituted nginx auth with this cookie auth. In the current form, there is still no sql or database integration. I store a hash of the password in config.yml, along with the other config variables. Of course this can be up for debate, but I initially did this because: - its not clear that PeachCloud needs multiple users with independent logins - its kind of a nice lean feature that all the state of the app is in that one config.yml file - its just a hash of a password, so it should be just as secure as if the password were stored somewhere else - it was not much work to implement - we are still so far achieving the needed functionality without needing to manage a database with migrations My config.yml currently looks like this: ```--- external_domain: "" dyn_domain: "" dyn_dns_server_address: "" dyn_tsig_key_path: "" dyn_enabled: false ssb_admin_ids: [] admin_password_hash: 3c255775632b05b1194107f9ac8b40f9d498720c70536a3f90be2686b31d1b67 temporary_password_hash: 75f40ca05158fb1b990ab2c7bcd29081ee70fd4c25010a952c6b08cac6fb5754 ``` On the downside, these hashes could be confusing without an explanation. But maybe comments in the yml could help with that. Since I was able to keep the same code as before, this also still includes the same logout, change password, and password reset features (via ssb dm), but now with the portability of not needing to alter nginx configurations (and so could run in an environment like yunohost, where nginx configs are handled separately from the app itself), and a prettier login screen (no weird http basic auth, instead its the nice login template you already made). Another nice thing, is if we still decide we want to add traditional sql-based multi-user auth in the future, we could still use this request guard, and just change its implementation. My initial thought would be to keep this for now, and then if at somepoint we want to go further with sql and multi-user auth, do that later in a separate PR.

notplants added 2 commits 2021-11-08 15:49:30 +00:00

212c2e1545 Add request guard for password authentication

04011e4f1a Add rocket authentication to all routes

notplants added 4 commits 2021-11-08 16:03:52 +00:00

18fbb7fa5f Fix typos

6605af7bed Fix clippy errors

0d23307f86 Use empty struct for Authenticated

309ec94cdb Allow large enum variant to avoid clippy warning

notplants requested review from glyph 2021-11-08 16:04:12 +00:00

glyph approved these changes 2021-11-09 10:53:44 +00:00

glyph left a comment

Thanks for the detailed PR. This is terrific work! I'm a big fan of the simplicity of this solution.

I've made a number of code comments in the review. You're welcome to merge once you've had a chance to read them and edit where required.

A few responses to questions and comments you raised:

In the current form, there is still no sql or database integration. I store a hash of the password in config.yml, along with the other config variables.

I agree that this is a perfectly sufficient solution for the time being.

we are still so far achieving the needed functionality without needing to manage a database with migrations

This is a huge plus for me.

On the downside, these hashes could be confusing without an explanation. But maybe comments in the yml could help with that.

+1 to explanatory comments and a "Do not manually edit these values" warning.

Some questions and comments of my own:

Do you know the lifespan of the cookies which are currently being served? I wonder what an appropriate duration would be?

Thinking of security: it might be wise to implement some basic bruteforcing defense mechanism. Otherwise, an adversary could just hammer the /login until they crack it. We could implement this at the application layer or via a dedicated service such as Fail2ban. Keen to hear your thoughts on this.

In the future it will probably be useful to expose some routes for public viewership (ie. without requiring auth). Examples might be an invite-request page (leave your SSB public key and a note with your request) or a device status page which allows pub followers to see if the Peach is operational. I like that the current RequestGuard approach makes it trivial to achieve this :)

Thanks for the detailed PR. This is terrific work! I'm a big fan of the simplicity of this solution. I've made a number of code comments in the review. You're welcome to merge once you've had a chance to read them and edit where required. **A few responses to questions and comments you raised:** > In the current form, there is still no sql or database integration. I store a hash of the password in config.yml, along with the other config variables. I agree that this is a perfectly sufficient solution for the time being. > we are still so far achieving the needed functionality without needing to manage a database with migrations This is a huge plus for me. > On the downside, these hashes could be confusing without an explanation. But maybe comments in the yml could help with that. +1 to explanatory comments and a "Do not manually edit these values" warning. **Some questions and comments of my own:** Do you know the lifespan of the cookies which are currently being served? I wonder what an appropriate duration would be? Thinking of security: it might be wise to implement some basic bruteforcing defense mechanism. Otherwise, an adversary could just hammer the `/login` until they crack it. We could implement this at the application layer or via a dedicated service such as Fail2ban. Keen to hear your thoughts on this. In the future it will probably be useful to expose some routes for public viewership (ie. without requiring auth). Examples might be an invite-request page (leave your SSB public key and a note with your request) or a device status page which allows pub followers to see if the Peach is operational. I like that the current `RequestGuard` approach makes it trivial to achieve this :)

peach-lib/src/password_utils.rs Outdated

						
				@ -49,57 +36,46 @@ pub fn validate_new_passwords(new_password1: &str, new_password2: &str) -> Resul

				/// Uses htpasswd to set a new password for the admin user

glyph commented

2021-11-09 07:52:36 +00:00

Don't forget to update the doc comment here.

👍 1

notplants marked this conversation as resolved

peach-lib/src/password_utils.rs

						
				@ -65,1 +39,3 @@

				        Err(PeachError::FailedToSetNewPassword { msg: err_output })

				    let new_password_hash = hash_password(&new_password.to_string());

				    let result = set_admin_password_hash(&new_password_hash);

				    match result {

glyph commented

2021-11-09 07:58:27 +00:00

I like the readability of the match here. It's quite straightforward for a new Rustacean to read the code and grok what's happening.

Another option is to transform the Result type directly:

pub fn set_new_password(new_password: &str) -> Result<(), PeachError> {
    let new_password_hash = hash_password(&new_password.to_string());
    set_admin_password_hash(&new_password_hash)
        // map `Result<PeachConfig, PeachError>` to `Result<(), PeachError>`
        .map(|_| ())
        // map `PeachError` to the `FailedToSetNewPassword` variant
        .map_err(|_| PeachError::FailedToSetNewPassword {
            msg: "failed to save password hash".to_string(),
        })
}

I don't have a strong preference but thought it might be interesting to share another approach :)

I like the readability of the `match` here. It's quite straightforward for a new Rustacean to read the code and grok what's happening. Another option is to transform the `Result` type directly: ```rust pub fn set_new_password(new_password: &str) -> Result<(), PeachError> { let new_password_hash = hash_password(&new_password.to_string()); set_admin_password_hash(&new_password_hash) // map `Result<PeachConfig, PeachError>` to `Result<(), PeachError>` .map(|_| ()) // map `PeachError` to the `FailedToSetNewPassword` variant .map_err(|_| PeachError::FailedToSetNewPassword { msg: "failed to save password hash".to_string(), }) } ``` I don't have a strong preference but thought it might be interesting to share another approach :)

notplants commented

2021-11-09 11:32:36 +00:00

wow I am super happy to know about this as a possible alternate syntax for handling success/error, thanks for sharing

this seems quite ergonomic

I also dont have a strong preference

wow I am super happy to know about this as a possible alternate syntax for handling success/error, thanks for sharing this seems quite ergonomic I also dont have a strong preference

👍 1

notplants marked this conversation as resolved

peach-lib/src/password_utils.rs Outdated

						
				@ -69,1 +55,4 @@

				    hasher.result_str()

				}

				/// Uses htpasswd to set a new temporary password for the admin user

glyph commented

2021-11-09 07:59:37 +00:00

Another small doc comment update needed here.

notplants commented

2021-11-09 11:27:56 +00:00

thanks!

stale comments are a source of great possible confusion

thanks! stale comments are a source of great possible confusion

notplants marked this conversation as resolved

peach-lib/src/password_utils.rs Outdated

						
				@ -143,6 +119,7 @@ using this link: http://peach.local/reset_password",

				    msg += &remote_link;

				    // finally send the message to the admins

				    let peach_config = load_peach_config()?;

				    info!("sending password reset: {}", msg);

glyph commented

2021-11-09 08:00:57 +00:00

Lovely use of logging here 🖤

Lovely use of logging here :black_heart:

notplants marked this conversation as resolved

peach-web/src/routes/authentication.rs Outdated

						
				@ -15,0 +36,4 @@

				}

				/// Request guard which returns an Authenticated struct with is_authenticated=true

				/// iff the user has a cookie which proves they are authenticated with peach-web.

glyph commented

2021-11-09 10:34:23 +00:00

Tiny typo here on iff.

Tiny typo here on `iff`.

notplants commented

2021-11-09 11:24:46 +00:00

"iff" is a shorthand that means "if and only if" (https://en.wikipedia.org/wiki/If_and_only_if),

for now I changed the comment to say "if and only if",
but "iff" is a pretty useful acronym, gets used a fair amount in technical docs, maybe you will see it around more now.

"iff" is a shorthand that means "if and only if" (https://en.wikipedia.org/wiki/If_and_only_if), for now I changed the comment to say "if and only if", but "iff" is a pretty useful acronym, gets used a fair amount in technical docs, maybe you will see it around more now.

glyph commented

2021-11-09 11:49:33 +00:00

Oh wow, I had no idea! Thanks for pointing that out.

notplants marked this conversation as resolved

peach-web/src/routes/authentication.rs

						
				@ -51,0 +123,4 @@

				            // NOTE: since we currently have just one user, the value of the cookie

				            // is just admin (this is arbitrary).

				            // If we had multiple users, we could put the user_id here.

				            cookies.add_private(Cookie::new(AUTH_COOKIE_KEY, ADMIN_USERNAME));

glyph commented

2021-11-09 10:35:23 +00:00

I love how simple the logic is here. Really clean and simple.

notplants commented

2021-11-09 11:17:14 +00:00

agreed, its nice when not using oauth-challenge-craziness etc., login cookie logic is actually pretty easy to follow

👍 1

notplants marked this conversation as resolved

notplants commented

2021-11-09 11:18:56 +00:00

accidentally clicked "resolve conversation" on your comment "lovely use of logging" - I'm just deleting that log now actually, that log statement prints the temporary password in plain text to the log, which isn't the best practice

glyph commented

2021-11-09 11:23:46 +00:00

Hahahaha oh god I definitely did not see the password included in the log >_<

Glad you removed that! 😅

FWIW I always "resolve conversation" on all reviewer comments before I merge a PR (whether they're asking for changes or just making a comment).

Hahahaha oh god I definitely _did not_ see the password included in the log >_< Glad you removed that! 😅 FWIW I always "resolve conversation" on all reviewer comments before I merge a PR (whether they're asking for changes or just making a comment).

notplants commented

2021-11-09 11:36:23 +00:00

aw interesting, I now see when you click "resolve" they dont entirely disappear and be re-shown. thats a nice feature for using the comments like a todo list. assuming my replies to comments dont get lost, like if you didnt see the replies because it was kind of hidden, would be a shame

👍 1

notplants commented

2021-11-09 11:47:52 +00:00

Do you know the lifespan of the cookies which are currently being served? I wonder what an appropriate duration would be?

The default expiration for a private cooke is one week (https://api.rocket.rs/master/rocket/http/struct.CookieJar.html#method.add_private), but we can change this with a supplied argument if we choose to. One week seems ok to me though.

Thinking of security: it might be wise to implement some basic bruteforcing defense mechanism. Otherwise, an adversary could just hammer the /login until they crack it. We could implement this at the application layer or via a dedicated service such as Fail2ban. Keen to hear your thoughts on this.

Agreed this would be a nice feature. Yunohost uses fail2ban (so in that environment they would handle that separately from the app itself). In the PeachCloud environment we could add our own fail2ban at some point. Intuitively, I don't feel like this is a priority, but something that would be good to add at some point.

I like that the current RequestGuard approach makes it trivial to achieve this :)

Agreed. I already have exposed some public routes quite easily as part of the authentication flow (/login, /reset_password and /forgot_password)

@glyph (mentioning as not sure if it makes a notification if I just respond or it probably does actually and I'm just trauma-conditioned from patchwork not doing that lol...)

> Do you know the lifespan of the cookies which are currently being served? I wonder what an appropriate duration would be? The default expiration for a private cooke is one week (https://api.rocket.rs/master/rocket/http/struct.CookieJar.html#method.add_private), but we can change this with a supplied argument if we choose to. One week seems ok to me though. > Thinking of security: it might be wise to implement some basic bruteforcing defense mechanism. Otherwise, an adversary could just hammer the /login until they crack it. We could implement this at the application layer or via a dedicated service such as Fail2ban. Keen to hear your thoughts on this. Agreed this would be a nice feature. Yunohost uses fail2ban (so in that environment they would handle that separately from the app itself). In the PeachCloud environment we could add our own fail2ban at some point. Intuitively, I don't feel like this is a priority, but something that would be good to add at some point. > I like that the current RequestGuard approach makes it trivial to achieve this :) Agreed. I already have exposed some public routes quite easily as part of the authentication flow (/login, /reset_password and /forgot_password) @glyph (mentioning as not sure if it makes a notification if I just respond or it probably does actually and I'm just trauma-conditioned from patchwork not doing that lol...)

❤️ 1

glyph referenced this pull request

2021-11-09 11:56:21 +00:00

Implement bruteforce defense #18

notplants added 2 commits 2021-11-10 11:33:43 +00:00

e3640f0885 Clean up comments

a4f459e1fc Change /send_password_reset to /forgot_password

notplants force-pushed auth from a4f459e1fc to e3640f0885

2021-11-10 11:35:19 +00:00

Compare

notplants merged commit a4e0f7d7fe into main

2021-11-10 11:35:47 +00:00

notplants referenced this issue from a commit

2021-11-10 11:35:47 +00:00

Merge pull request 'Create authentication request guard' (#17) from auth into main

notplants deleted branch auth

2021-11-10 11:35:47 +00:00

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: PeachCloud/peach-workspace#17

				`@ -49,57 +36,46 @@ pub fn validate_new_passwords(new_password1: &str, new_password2: &str) -> Resul`

				`/// Uses htpasswd to set a new password for the admin user`