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:
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
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.
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 :)
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:
pubfnset_new_password(new_password: &str)-> Result<(),PeachError>{letnew_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 :)
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
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.
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
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
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).
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
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
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...)
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
glyph
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:
Then in any route, you can simply add an argument
auth: Authenticated, like this: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:
My config.yml currently looks like this:
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.
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:
I agree that this is a perfectly sufficient solution for the time being.
This is a huge plus for me.
+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
/loginuntil 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
RequestGuardapproach makes it trivial to achieve this :)@@ -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 userDon't forget to update the doc comment here.
@@ -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 {I like the readability of the
matchhere. It's quite straightforward for a new Rustacean to read the code and grok what's happening.Another option is to transform the
Resulttype directly:I don't have a strong preference but thought it might be interesting to share another approach :)
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
@@ -69,1 +55,4 @@hasher.result_str()}/// Uses htpasswd to set a new temporary password for the admin userAnother small doc comment update needed here.
thanks!
stale comments are a source of great possible confusion
@@ -143,6 +119,7 @@ using this link: http://peach.local/reset_password",msg += &remote_link;// finally send the message to the adminslet peach_config = load_peach_config()?;info!("sending password reset: {}", msg);Lovely use of logging here 🖤
@@ -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.Tiny typo here on
iff."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.
Oh wow, I had no idea! Thanks for pointing that out.
@@ -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));I love how simple the logic is here. Really clean and simple.
agreed, its nice when not using oauth-challenge-craziness etc., login cookie logic is actually pretty easy to follow
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
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).
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
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.
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.
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...)
a4f459e1fctoe3640f0885