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

add support for a password callback & PKCS#12 files #195

Merged
merged 6 commits into from
Mar 23, 2022

Conversation

sjaeckel
Copy link
Member

@sjaeckel sjaeckel commented Feb 3, 2022

In order to be able to load password-protected key files a password
callback was added.

This also adds support for PKCS#12 containers instead of certificate+key.

@sjaeckel sjaeckel requested a review from pasis February 3, 2022 12:48
@sjaeckel sjaeckel force-pushed the password-callback branch 2 times, most recently from b5c33a9 to e9cccfd Compare February 3, 2022 14:12
@jubalh
Copy link
Member

jubalh commented Feb 3, 2022

Rest looks good to me, but I don't have much expertise in this area :)

@sjaeckel sjaeckel force-pushed the some-maintenance branch 5 times, most recently from 5e77506 to a120643 Compare February 6, 2022 11:37
@pasis
Copy link
Member

pasis commented Feb 6, 2022

Please add new files to EXTRA_DIST in Makefile.am. Otherwise, tarballs will miss the files.

@@ -80,6 +81,52 @@ static int certfail_handler(const xmpp_tlscert_t *cert,
return read_char[0] == 'y' || read_char[0] == 'Y';
}

static int
password_callback(char *pw, size_t pw_max, const char *fname, void *userdata)
Copy link
Member

Choose a reason for hiding this comment

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

Quick question before reading the function: is it expected to implement this number of steps by users every time? From the comment I understand, that this code caches password, but as far as I understand, for a single connection this callback will always be called for the same key file, right? If so, can we move caching to libstrophe? I know, this is not secure, but we already cache account's password. Besides, we can clear the cached password after connection establishment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I also thought about the same, but then the problem becomes: when the user enters a wrong password, how do we handle this? Simply bail out? Retry after wiping the cache? If we bail out we'd have to expose the TLS-stack specific error to the user ... or mangle it into an strophe-specific error ...

If we add caching, can it be disabled at run-time? compile-time?

strophe.h Show resolved Hide resolved
strophe.h Outdated Show resolved Hide resolved
@sjaeckel sjaeckel changed the base branch from some-maintenance to master February 6, 2022 22:22
@sjaeckel sjaeckel force-pushed the password-callback branch 9 times, most recently from caa90e3 to a1bc90b Compare February 8, 2022 15:36
@sjaeckel
Copy link
Member Author

sjaeckel commented Feb 8, 2022

Please add new files to EXTRA_DIST in Makefile.am. Otherwise, tarballs will miss the files.

added two commits on top, please have a look

I'd simply pick them to master if you're fine with it

Moved them to #196

@sjaeckel sjaeckel force-pushed the password-callback branch 2 times, most recently from adba12f to b0d4b8c Compare February 10, 2022 10:31
@sjaeckel sjaeckel mentioned this pull request Feb 10, 2022
@sjaeckel sjaeckel force-pushed the fixups-and-improvements branch from a674463 to e32d39d Compare February 10, 2022 10:39
@sjaeckel sjaeckel force-pushed the password-callback branch 2 times, most recently from d91982a to 8a08baf Compare February 22, 2022 14:09
@sjaeckel sjaeckel changed the base branch from fixups-and-improvements to master February 22, 2022 14:15
@sjaeckel sjaeckel force-pushed the password-callback branch 2 times, most recently from 8a08baf to 7f1d138 Compare February 22, 2022 14:25
@sjaeckel sjaeckel requested a review from pasis February 22, 2022 14:38
@sjaeckel sjaeckel force-pushed the password-callback branch from 7f1d138 to a34e2ac Compare March 9, 2022 15:53
@sjaeckel sjaeckel changed the title add support for a password callback add support for a password callback & PKCS#12 files Mar 16, 2022
@sjaeckel sjaeckel force-pushed the password-callback branch from a34e2ac to e5ff112 Compare March 17, 2022 11:25
In order to be able to load password-protected key files a password
callback was added.

This also adds support for PKCS#12 containers instead of certificate+key.

Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>
@sjaeckel sjaeckel force-pushed the password-callback branch 3 times, most recently from 15c0818 to 668df48 Compare March 23, 2022 00:08
The cache is stored per connection object and is cleared on
* entry of wrong password
* release of connection object
* successful connection

It can be configured that libstrophe retries the password entry in case
the user entered a wrong password.

Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>
`ERR_func_error_string()` has been deprecated.

Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>
Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>
@sjaeckel sjaeckel force-pushed the password-callback branch 2 times, most recently from 56a0ef3 to 4f95174 Compare March 23, 2022 17:58
Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>
Signed-off-by: Steffen Jaeckel <jaeckel-floss@eyet-services.de>
@sjaeckel sjaeckel force-pushed the password-callback branch from 4f95174 to 7111ee2 Compare March 23, 2022 18:06
@sjaeckel sjaeckel merged commit 7111ee2 into master Mar 23, 2022
@sjaeckel sjaeckel deleted the password-callback branch March 23, 2022 18:14
@sjaeckel
Copy link
Member Author

ping profanity-im/profanity#1422

@sjaeckel sjaeckel restored the password-callback branch March 23, 2022 19:00
@sjaeckel sjaeckel deleted the password-callback branch March 23, 2022 19:01
@sjaeckel sjaeckel added this to the 0.12.0 milestone Jul 11, 2022
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.

3 participants