Bug32562 #1563
Closed
Bug32562 #1563
Conversation
- See hs_client_register_auth_credentials() for the entry point. - Also set the permanent flag for credentials we read from the filesystem. - Also add some missing documentation.
Remove Permanent flag from old tests, and make a new test that does all the permanent things.
Because the function that parses client auth credentials saved on disk (parse_auth_file_content()) is not future compatible, there is no way to add support for storing the nickname on the disk. Hence, nicknames cannot persist after Tor restart making them pretty much useless. In the future we can introduce nicknames by adding a new file format for client auth credentials, but this was not deemed worth doing at this stage.
- Remove key_dir which is useless. - Kill an indentation layer. We want to make it cleaner and slimmer so that we can reuse parts of it in the REMOVE command for removing the right client auth file.
Now we have a function that reads a file and returns a credential. We need that for the REMOVE control port command.
Pull Request Test Coverage Report for Build 7372
|
dgoulet-tor
reviewed
Nov 26, 2019
|
|
||
| tor_asprintf(&fname, "%s.auth_private", onion_address); | ||
| full_fname = hs_path_from_filename(dir, fname); | ||
| tor_free(fname); |
Maybe you meant to use full_name ?
I actually did mean full_fname, in the sense that it's the full path instead of just the actual filename. Let me know if you want me to change it to something else.
src/feature/hs/hs_client.c
Outdated
| @@ -1445,6 +1445,79 @@ client_dir_fetch_unexpected(dir_connection_t *dir_conn, const char *reason, | |||
| NULL); | |||
| } | |||
|
|
|||
| /** Get the full filename for storing the client auth credentials for the | |||
| * service in <b>onion_address</b>. The base directory is <b>dir</b>. */ | |||
Seems this can never fail that is never return NULL. I would add a comment about that.
Fixed in e7f8319.
| log_warn(LD_REND, "Failed to remove client auth file (%s).", | ||
| creds_file_path); | ||
| goto end; | ||
| } |
Hmmm, don't we have a TOCTOU issue race? Seems possibly harmless but you never know. I would probably just let tor_unlink() fail here instead of looking if the file is legit before?
Fixed in 2852f8d.
FWIW I was following the logic from expire_old_onion_keys() but this new logic is also fine.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
No description provided.
The text was updated successfully, but these errors were encountered: