Skip to content

Conversation

kostasrim
Copy link
Contributor

  1. Properly print sha256 passwords by converting hex values to characters

@kostasrim kostasrim changed the base branch from main to acl_part_4_add_acl_set_user August 23, 2023 15:29
@kostasrim kostasrim self-assigned this Aug 23, 2023
@kostasrim kostasrim requested a review from dranikpg August 23, 2023 16:55
dranikpg
dranikpg previously approved these changes Aug 23, 2023
Copy link
Contributor

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

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

We had some utility like this somewhere 🤷

@kostasrim
Copy link
Contributor Author

kostasrim commented Aug 24, 2023

We had some utility like this somewhere

I will take a look before I merge, last time I checked I couldn't find anything

@kostasrim
Copy link
Contributor Author

I will take a look before I merge, last time I checked I couldn't find anything

Couldn't find anything, I imagine it would use isdigit (which I used to grep it) but nothing relevant came up).

I will move this at some point to a utility header, but it's a non priority for now

@romange
Copy link
Collaborator

romange commented Aug 24, 2023 via email

@dranikpg
Copy link
Contributor

dranikpg commented Aug 24, 2023

It's not escaping. The SHA is essentially a uint8_t[] (just stored in a string) and he wants to print it compactly, so his code is actually

string out;
for (char c: password)
  absl::StrAppend(&out, absl::Hex(c));

to get smth like this

d14a028c2a3a2bc9476102bb288234c415a2b01f828ea62ac5b3e42f

@kostasrim
Copy link
Contributor Author

@dranikpg

True, but not every character is a hex so you still need the conditional. Although it can be written in a more expressive fashion:

instead of
isdigit(c) ? result.push_back(c) : absl::StrAppend(&result, absl::Hex(c));

absl::StrAppend(&result, isdigit(c) : c : absl::Hex(c));

And I like the second more, so I will make this change :D

@kostasrim
Copy link
Contributor Author

Uh and you can't because Incompatible operand types :(

@royjacobson
Copy link
Contributor

There's absl::BytesToHexString in absl/strings/escaping.h

Comment on lines 56 to 60
for (size_t i = 0; i < 15; ++i) {
absl::StrAppend(&result, absl::Hex(pass[i]));
}

result.resize(15);
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. What about the leading digit? [1, 1] should be 0101

I thinks its better to use what Roy found 😝


// BytesToHexString()
//
// Converts binary data into an ASCII text string, returning a string of size
// `2*from.size()`.
std::string BytesToHexString(absl::string_view from);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100%

(*cntx)->StartArray(registry.size());

auto pretty_print_sha = [](std::string_view pass) {
return absl::BytesToHexString(pass.substr(15)).substr(15);
Copy link
Contributor

@dranikpg dranikpg Aug 24, 2023

Choose a reason for hiding this comment

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

I don't understand 😄 bytes to hex string returns a string of length 30 (twice the size), why do you need only half of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I want to print up to 15 characters :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay I'm just gonna trust you on this one 🤣

Base automatically changed from acl_part_4_add_acl_set_user to main August 24, 2023 10:24
@kostasrim kostasrim dismissed dranikpg’s stale review August 24, 2023 10:24

The base branch was changed.

@kostasrim kostasrim enabled auto-merge (squash) August 24, 2023 10:27
@kostasrim kostasrim requested a review from dranikpg August 24, 2023 10:28
@kostasrim kostasrim merged commit 631c8e9 into main Aug 24, 2023
@kostasrim kostasrim deleted the add_pretty_print_of_hashed_passwords branch August 24, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants