Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using existing key file on login #688

Merged
merged 3 commits into from Feb 10, 2023

Conversation

trygveaa
Copy link
Contributor

@trygveaa trygveaa commented Feb 5, 2023

If the given key on login in empty, keep the existing key file rather
than overwriting it with an empty file. This is useful if you log out
and want to log in again and still use the same key, or if you have
copied over the key file rather than providing it as input.

Before the first commit, an empty key file would be created if key
wasn't specified, and after that commit, the key file would not be
created if the key wasn't specified and stay empty if it was empty.

With the second commit the log command checks the key file if a key is
not specified and exits with an error message if either the key file
couldn't be opened or is empty. If a key is specified, the key file is
just created with it as before.

(Btw, great talk at fosdem!)

@conradludgate
Copy link
Collaborator

Seems ok, I'm a little skeptical on it's value, but I'm overall not against this

@conradludgate
Copy link
Collaborator

(you can ignore the clippy failure, being fixed in #686)

@trygveaa trygveaa force-pushed the allow-using-existing-key-file-on-login branch from 678163b to 54fd21b Compare February 6, 2023 15:15
@trygveaa
Copy link
Contributor Author

trygveaa commented Feb 6, 2023

Seems ok, I'm a little skeptical on it's value, but I'm overall not against this

I'd argue that the first commit is useful at least. After registering, I logged out and in again because I like to confirm that the login works and got the credentials I expect. On the key prompt, I assumed it would just use the existing key if I didn't specify anything, so I ended up with an empty key file.

The second commit could be better. I think it makes sense to verify that the key is valid on login, but it would be better to actually verify that it's a valid key. Just checking that it's not empty isn't that useful, especially since you shouldn't end up with an empty key after the first commit. (And I see now that the sync panics if the key isn't valid, so there shouldn't be any security concern with an empty/weak key here).

If we were to add a check that the key is valid, I think it would make sense to also do that check for key values specified at the prompt.

conradludgate
conradludgate previously approved these changes Feb 6, 2023
Copy link
Collaborator

@conradludgate conradludgate left a comment

Choose a reason for hiding this comment

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

I'll also ask for @ellie's judgement, but I am happy to accept this

If the given key on login in empty, keep the existing key file rather
than overwriting it with an empty file. This is useful if you log out
and want to log in again and still use the same key, or if you have
copied over the key file rather than providing it as input.
Before the previous commit, an empty key file would be created if key
wasn't specified, and after the previous commit, the key file would not
be created if the key wasn't specified and stay empty if it was empty.

Now the log command checks the key file if a key is not specified and
exits with an error message if either the key file couldn't be opened or
is empty. If a key is specified, the key file is just created with it as
before.
After reading the key either from an existing key file, or from the user
input, validate that the provided key is valid (rather than just
checking that it isn't empty). If no key file exists, create a new key
instead of erroring out.
@trygveaa
Copy link
Contributor Author

trygveaa commented Feb 9, 2023

The second commit could be better. I think it makes sense to verify that the key is valid on login, but it would be better to actually verify that it's a valid key. Just checking that it's not empty isn't that useful, especially since you shouldn't end up with an empty key after the first commit.

I changed it to validate that the key is valid, both when read from an existing file and from user input. I also changed it to create a new key if the key file is missing, rather than erroring out. And I rebased on main to resolve the conflicts.

Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

This looks good to me! Definitely an edge case, but probably a very frustrating one if you happen to be caught by it! Thanks for the contribution 😊

(Btw, great talk at fosdem!)

Thank you! 🙏

@ellie ellie enabled auto-merge (squash) February 10, 2023 19:26
@ellie ellie merged commit 2cec7ba into atuinsh:main Feb 10, 2023
@trygveaa trygveaa deleted the allow-using-existing-key-file-on-login branch February 10, 2023 23:02
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.

None yet

3 participants