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

irc: allow specifying SSL CA per server #1262

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Oct 6, 2018

Closes #438.

This PR is an improvement of PR #613 (credit to @ManiacTwister), which started in the right direction but has some still unresolved issues:

  1. It accesses core (only) functions from irc plugin by direct include, not via plugin API: https://github.com/weechat/weechat/pull/613/files#r50862583
  2. It modifies the trust list during certificate verification instead of setting up the correct trust list before that. It seems wrong because it's not obvious why allocating a new trust list mid-verification is allowed (if it even works) and if GnuTLS doesn't break this in the future.
  3. The solution is not well extensible: other users of hook_connect still have no way to manipulate the trust list for their connection only.

This PR tries to solve these problems and hopefully finally bring per server SSL CAs to weechat. Every hook_connect user can choose to manipulate the trust list however they see fit: clear it (or not), add CAs from file (or elsewhere).

Turns out GnuTLS >=3.3.0 is required to manipulate the trust list like this. This is a significantly higher requirement than otherwise is needed but it only affects the added ssl_ca_file option use and should be fine on most cases (even the old Ubuntu 16.04 has GnuTLS 3.4).

Happy Hacktoberfest!

hook_connect->callback_data,
*HOOK_CONNECT(hook_connect, gnutls_sess),
NULL, 0, NULL, 0, NULL,
WEECHAT_HOOK_CONNECT_GNUTLS_CB_INIT_XCRED);
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this isn't the best to call the GnuTLS callbacks function when simply initializing the trust list because strictly it isn't a callback for a GnuTLS function. INIT_XCRED could also be added as a case under the usual hook_connect callback if that's more appropriate, although to me it seems more related to using GnuTLS specifically and most connections will not need to touch that part.

@flashcode flashcode added the feature New feature request label Oct 7, 2018
@codecov-io
Copy link

Codecov Report

Merging #1262 into master will decrease coverage by 0.02%.
The diff coverage is 4.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1262      +/-   ##
==========================================
- Coverage   27.69%   27.66%   -0.03%     
==========================================
  Files         201      201              
  Lines       81939    81976      +37     
==========================================
- Hits        22693    22681      -12     
- Misses      59246    59295      +49
Impacted Files Coverage Δ
src/core/wee-config.c 46.77% <ø> (+0.09%) ⬆️
src/plugins/irc/irc-command.c 3.33% <0%> (-0.01%) ⬇️
src/plugins/irc/irc-server.c 8.58% <0%> (-0.08%) ⬇️
src/core/hook/wee-hook-connect.c 0% <0%> (ø) ⬆️
src/core/wee-network.c 1.99% <0%> (-2.04%) ⬇️
src/plugins/irc/irc-config.c 58.76% <100%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dda5ff...8d031ce. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to specify ca certificates per network
3 participants