Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

Add taker http basic auth #976

Closed
wants to merge 4 commits into from
Closed

Add taker http basic auth #976

wants to merge 4 commits into from

Conversation

bonomat
Copy link
Collaborator

@bonomat bonomat commented Dec 27, 2021

Resolves #609

This is the first step to have authentication for the taker.
For our current use of running the taker on umbrel this is sufficient. Eventually we might want to allow the user to change the password from inside of the application.

Since maker and taker are two different binaries there is no need to have a maker_username and taker_username
This will allow us to have arbitrary length passwords and easier configure it through cli parameters
@bonomat bonomat changed the title Add taker basic auth Add taker http basic auth Dec 27, 2021
The password can be provided as a argument or has a default value
@bonomat
Copy link
Collaborator Author

bonomat commented Dec 27, 2021

Note: we should backport this to 0.3.x because it is mostly needed for getumbrel/umbrel#1149

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a fan of the default password IN the binary.

@@ -16,6 +16,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
The previous payout transactions are invalidated.
The new payout transactions spend from the same lock transaction, so the rollover happens off-chain.
In case a maker rejects a rollover request from a taker the old oracle price event and payout transactions stay in place.
- Changed username for HTTP authentication to `itchysats`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go under a different section, we are in ### Added here :)

@@ -14,7 +14,8 @@ use std::str::FromStr;
/// A request guard that can be included in handler definitions to enforce authentication.
pub struct Authenticated {}

pub const MAKER_USERNAME: &str = "maker";
pub const USERNAME: &str = "itchysats";
pub const TAKER_DEFAULT_PASSWORD: &str = "onbrinkofsecondbailout";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not have a default password in the binary. If we want a default password for umbrel, we can always hardcode that in the docker-compose file. If --password is not specified, we can always derive a password from the seed.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also: if we would like a default password for the CI, we can always hardcode one in cargo dev-taker etc.

Comment on lines +307 to +310
/// let password = "Now I'm feelin' so fly like a G6".to_string();
/// let password_vec = password.as_bytes();
/// let password_hex = hex::encode(vec);
/// let basic_auth = base64::encode(&format!("{}:{}", "itchysats", password_hex));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That diff seems unnecessary if we are just changing the username from maker to itchysats.

Comment on lines +280 to +286
/// A rocket responder that prompts the user to sign in to access the API.
#[derive(rocket::Responder)]
#[response(status = 401)]
pub struct PromptAuthentication {
inner: (),
www_authenticate: Header<'static>,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be re-used across both binaries.

Comment on lines +271 to +278
/// A "catcher" for all 401 responses, triggers the browser's basic auth implementation.
#[rocket::catch(401)]
pub fn unauthorized() -> PromptAuthentication {
PromptAuthentication {
inner: (),
www_authenticate: Header::new("WWW-Authenticate", r#"Basic charset="UTF-8"#),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one probably too!

@thomaseizinger
Copy link
Contributor

Closed in favor of #982.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Taker UI authentication
3 participants