feat: crypt hasher #2788

notanatol · 2022-01-31T20:07:00Z

Checklist

I have read the coding guide
My change requires a documentation update and I have done it

Description

We now return 403 Forbidden in only one case - when the provided token is valid but does not have sufficient privileges to access the requested resource.
All the other cases will result in a 401 Unauthorized return code.
Added a new command bcrypt that can generate a bcrypt hash for a given plain text password. The returned value can be used for admin-password configuration parameter. The tool can also validate that the provided hash matches a plain text password (using the --check option). Note: when checking the hash please make sure to quote it, the shell does not take kindly to the dollar signs in the hash text.

Motivation and context

The bcrypt command is a handy offline alternative to https://bcrypt-generator.com/

Usage

> bee bcrypt super$ecret
$2a$10$eZP5YuhJq2k8DFmj9UJGWOIjDtXu6NcAQMrz7Zj1bgIVBcHA3bU5u
> bee bcrypt --check super$ecret '$2a$10$eZP5YuhJq2k8DFmj9UJGWOIjDtXu6NcAQMrz7Zj1bgIVBcHA3bU5u'
OK: password hash matches provided plain text

This change is

cmd/hasher/main.go

AuHau · 2022-02-03T06:23:02Z

LGTM, but actually I would like to bring up why we actually even need to hash the password? What is the goal?

I guess protection of the password, but I am not sure if it is not overkill? This just seems to me like a bad UX...

notanatol · 2022-02-03T13:25:27Z

LGTM, but actually I would like to bring up why we actually even need to hash the password? What is the goal?

I guess protection of the password, but I am not sure if it is not overkill? This just seems to me like a bad UX...

I don't remember the exact reason (except not having plain text passwords) but the reasoning was given by @janos in one of the comments here: #2400

janos · 2022-02-03T16:07:51Z

LGTM, but actually I would like to bring up why we actually even need to hash the password? What is the goal?

I guess protection of the password, but I am not sure if it is not overkill? This just seems to me like a bad UX...

Yes, as @notanatol said more details are in the previous pr comments, but basic the reason is the same as passwords are not stored in plain text in databases in common web applications. Or, why web servers like apache2 and nginx are doing the same for basic auth configurations. Apache2 and nginx are very popular web servers.

acud

@notanatol since we are using viper-cobra wouldn't it make more sense to use the existing infrastructure to ship the new command?

Also, since you're de-facto creating a new binary here, it will not be built nor shipped by default, which means that we're gonna have to have separate artifacts on releases for this. This involves a lot of changes to the release process and I'm not sure this is entirely necessary or desirable (we were already burnt by maintaining these sort of extra util in the past).

So I wonder whether it would not make sense to pack this as a subcommand in bee. Ideally we would not have this sort of stuff in bee, not sure why this cannot be a command as part of swarm-cli. @agazso input please?

agazso · 2022-02-15T17:21:07Z

So I wonder whether it would not make sense to pack this as a subcommand in bee. Ideally we would not have this sort of stuff in bee, not sure why this cannot be a command as part of swarm-cli. @agazso input please?

I agree that creating a new binary can make things cumbersome. Alternatively it makes sense to add this functionality to swarm-cli.

notanatol · 2022-02-15T17:39:55Z

So I wonder whether it would not make sense to pack this as a subcommand in bee. Ideally we would not have this sort of stuff in bee, not sure why this cannot be a command as part of swarm-cli. @agazso input please?

I agree that creating a new binary can make things cumbersome. Alternatively it makes sense to add this functionality to swarm-cli.

OK so in this case I'm going to split this PR in two - one with the fixes (HTTP codes) requested by @AuHau and another one with the embedded hasher.

Then we'll have all the time we need to make a decision if we want this hasher in the binary or a separate js tool.

mrekucci · 2022-02-15T18:34:29Z

Maybe it will be worth considering just using something existing as htpasswd: https://unix.stackexchange.com/questions/307994/compute-bcrypt-hash-from-command-line

AuHau · 2022-02-15T20:18:48Z

Oh, I haven't noticed that it is a standalone command. I originally envisioned it as bee's subcommand, so I agree that makes more sense.

Although I would strongly suggest incorporating this functionality directly into bee. The @mrekucci mentioned htpasswd command requires apache-utils package which is a dependency completely unrelated to Bee's stack and requires the user to actually understand what command and what parameters it should use to produce hash compatible with Bee's requirements.

Also, I am not in favor of outsourcing this to swarm-cli as it is directly related to the Bee's configuration and not "Swarm functionality" (which is the domain of swarm-cli). For example, I am doing this hashing as part of Bee Factory setup for the restricted API support and this would require also to install swarm-cli to make it work...

notanatol · 2022-02-17T14:47:29Z

Oh, I haven't noticed that it is a standalone command. I originally envisioned it as bee's subcommand, so I agree that makes more sense.

@AuHau I moved it to be a bee sub-command, feel free to give it a try.

acud

the usual naming nitpicking :)

cmd/bee/cmd/hasher.go

cmd/hasher/main.go

acud · 2022-02-18T01:24:10Z

Also, I am not in favor of outsourcing this to swarm-cli as it is directly related to the Bee's configuration and not "Swarm functionality" (which is the domain of swarm-cli). For example, I am doing this hashing as part of Bee Factory setup for the restricted API support and this would require also to install swarm-cli to make it work...

I guess here we are not 100% agreeing (which is fine). IMO it is not in bee's responsibility to hash passwords just as much as it does not have an explicit command that generates you with a V3 ethereum key (you use external libraries for that). It also also raises the question on what is exactly the responsibility (and long-term aspirations) of swarm-cli. For me swarm-cli can easily be a sort of apache-utils suite. Just some thoughts.

AuHau · 2022-02-21T13:38:09Z

Ups sorry 😅 Mis-click 🙈
Just wanted to write that I have tried it and LGTM! 😅

AuHau · 2022-02-21T13:40:07Z

And ad @acud:
I guess I agree with you, except that until now "configuration of Bee" is not really domain of swarm-cli and that was why I have though of Bee's CLI as better place to put it. If swarm-cli will start to be in the configuration business of Bee then I agree there it would make more sense...

acud · 2022-02-22T19:52:06Z

cmd/bee/cmd/bcrypt.go

+				return errors.New("failed to generate password hash")
+			}
+
+			fmt.Println(string(hashed))


Be aware that you're print not just the actual string here, but also a line feed. It would be good to check that it doesn't make some cli checks fail as a result (e.g. if the hashed password is stored in a file then simply bee bcrypt --check playtextpass | diff hash_pass - to see that they match)

I think it works:

$> go run $(pwd)/cmd/bee bcrypt super$ecret > plaintext $> go run $(pwd)/cmd/bee bcrypt --check super$ecret `cat plaintext` OK: password hash matches provided plain text

but I will remove the new line just in case

they will always pass in this case, because the command generated both outputs (hence both have exactly the same characters, line feed included) :)

notanatol · 2022-02-22T20:50:30Z

$> go run $(pwd)/cmd/bee bcrypt --check super$ecret `go run $(pwd)/cmd/bee bcrypt super$ecret`
OK: password hash matches provided plain text

notanatol added 2 commits January 31, 2022 13:39

feat: bcrypt hasher utility

88a4fe8

fix: wrong http codes

3164c4a

notanatol requested review from AuHau and acud January 31, 2022 20:07

bee-runner bot added the pull-request label Jan 31, 2022

notanatol changed the title ~~Crypt hasher~~ feat: crypt hasher Jan 31, 2022

notanatol added the ready for review The PR is ready to be reviewed label Jan 31, 2022

mrekucci reviewed Feb 1, 2022

View reviewed changes

cmd/hasher/main.go Outdated Show resolved Hide resolved

fix: user friendly error

1a139d1

notanatol force-pushed the crypt-hasher branch from 2aeaedc to 1a139d1 Compare February 2, 2022 02:49

mrekucci approved these changes Feb 2, 2022

View reviewed changes

feat: password hash checking functionality

3f4b5a3

notanatol force-pushed the crypt-hasher branch from 7ad7a0a to 3f4b5a3 Compare February 14, 2022 02:30

notanatol requested a review from janos February 15, 2022 15:20

acud reviewed Feb 15, 2022

View reviewed changes

notanatol added in progress ongoing development , hold out with review and removed ready for review The PR is ready to be reviewed labels Feb 16, 2022

notanatol added 4 commits February 17, 2022 07:42

fix: hasher as cobra command

1278408

fix: help

b535e8e

fix: arg length

a74fe16

fix: lint

ba27ba1

notanatol added ready for review The PR is ready to be reviewed and removed in progress ongoing development , hold out with review labels Feb 17, 2022

notanatol requested a review from acud February 17, 2022 16:32

acud reviewed Feb 18, 2022

View reviewed changes

cmd/bee/cmd/hasher.go Outdated Show resolved Hide resolved

cmd/bee/cmd/hasher.go Outdated Show resolved Hide resolved

cmd/hasher/main.go Outdated Show resolved Hide resolved

fix: copyright and command name

c2c622d

AuHau closed this Feb 21, 2022

AuHau reopened this Feb 21, 2022

acud reviewed Feb 22, 2022

View reviewed changes

acud approved these changes Feb 22, 2022

View reviewed changes

fix: no newline

e75b66b

notanatol merged commit 7849e33 into master Feb 23, 2022

notanatol deleted the crypt-hasher branch February 23, 2022 00:52

aloknerurkar added this to the 1.5.0 milestone Mar 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: crypt hasher #2788

feat: crypt hasher #2788

notanatol commented Jan 31, 2022 •

edited

Loading

AuHau commented Feb 3, 2022

notanatol commented Feb 3, 2022 •

edited

Loading

janos commented Feb 3, 2022

acud left a comment •

edited

Loading

agazso commented Feb 15, 2022

notanatol commented Feb 15, 2022

mrekucci commented Feb 15, 2022

AuHau commented Feb 15, 2022

notanatol commented Feb 17, 2022 •

edited

Loading

acud left a comment

acud commented Feb 18, 2022 •

edited

Loading

AuHau commented Feb 21, 2022

AuHau commented Feb 21, 2022

acud Feb 22, 2022

notanatol Feb 22, 2022

acud Feb 22, 2022

notanatol commented Feb 22, 2022 •

edited

Loading

feat: crypt hasher #2788

feat: crypt hasher #2788

Conversation

notanatol commented Jan 31, 2022 • edited Loading

Checklist

Description

Motivation and context

Usage

AuHau commented Feb 3, 2022

notanatol commented Feb 3, 2022 • edited Loading

janos commented Feb 3, 2022

acud left a comment • edited Loading

Choose a reason for hiding this comment

agazso commented Feb 15, 2022

notanatol commented Feb 15, 2022

mrekucci commented Feb 15, 2022

AuHau commented Feb 15, 2022

notanatol commented Feb 17, 2022 • edited Loading

acud left a comment

Choose a reason for hiding this comment

acud commented Feb 18, 2022 • edited Loading

AuHau commented Feb 21, 2022

AuHau commented Feb 21, 2022

acud Feb 22, 2022

Choose a reason for hiding this comment

notanatol Feb 22, 2022

Choose a reason for hiding this comment

acud Feb 22, 2022

Choose a reason for hiding this comment

notanatol commented Feb 22, 2022 • edited Loading

notanatol commented Jan 31, 2022 •

edited

Loading

notanatol commented Feb 3, 2022 •

edited

Loading

acud left a comment •

edited

Loading

notanatol commented Feb 17, 2022 •

edited

Loading

acud commented Feb 18, 2022 •

edited

Loading

notanatol commented Feb 22, 2022 •

edited

Loading