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.

Sign up for GitHub

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

Jump to bottom

Added support for SASL EXTERNAL #363

Merged
merged 12 commits into from
Dec 31, 2022
Merged

Added support for SASL EXTERNAL #363

merged 12 commits into from
Dec 31, 2022

Conversation

trevarj
Copy link
Contributor

@trevarj trevarj commented Nov 14, 2021

A user can now generate a x509 certificate, register it with a server,
and provide the PEM file to tiny for use over TLS.

Closes #196

Overview:

  1. Bumped rustls crates
  2. Added configuration for SASL EXTERNAL, where a user specifies a path to a PEM file with a certificate inside (generated with instructions from server).
  3. Use user provided certificate in TLS connector config for client authorization (crates/libtiny_client/src/stream.rs)

@trevarj
Copy link
Contributor Author

trevarj commented Nov 14, 2021 •
edited
Loading

Native TLS using openssl is waiting on sfackler/rust-native-tls#209, which will make it much more convenient to add the user's certificate.

trevarj
trevarj commented Nov 14, 2021
View reviewed changes
crates/tiny/src/main.rs Outdated Show resolved Hide resolved
osa1
osa1 reviewed Nov 15, 2021
View reviewed changes
Copy link
Owner

@osa1 osa1 left a comment

Choose a reason for hiding this comment

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

Thanks @trevarj! Reviewed the front-end parts for now. I'll review the client parts tomorrow.

crates/tiny/src/main.rs Outdated Show resolved Hide resolved
crates/tiny/src/main.rs Outdated Show resolved Hide resolved
crates/tiny/src/main.rs Outdated Show resolved Hide resolved
crates/tiny/src/main.rs Outdated Show resolved Hide resolved
crates/tiny/src/main.rs Outdated Show resolved Hide resolved
crates/tiny/src/main.rs Outdated Show resolved Hide resolved
crates/tiny/config.yml Outdated Show resolved Hide resolved
osa1
osa1 approved these changes Nov 15, 2021
View reviewed changes
Copy link
Owner

@osa1 osa1 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 great, thanks again @trevarj.

Are you currently using this? I didn't test it myself.

crates/libtiny_client/src/lib.rs Outdated Show resolved Hide resolved
crates/libtiny_client/src/state.rs Show resolved Hide resolved
crates/tiny/config.yml Outdated Show resolved Hide resolved
crates/tiny/src/main.rs Outdated Show resolved Hide resolved
@trevarj
Copy link
Contributor Author

trevarj commented Nov 15, 2021

Are you currently using this? I didn't test it myself.

Yes, I think I will continue once merged too.

@trevarj
Added support for SASL EXTERNAL
38a994c 
A user can now generate a x509 certificate, register it with a server,
and provide the PEM file to tiny for use over TLS.

Closes osa1#196
@trevarj
Small cleanup and comments
0bc3a87
@trevarj
Fix comment error
8f6f82c
@trevarj
PR feedback
7ce42d4 
- Showing SASL errors in the respective server tab
- Error message changes
@trevarj
Config backwards compatibility
c68ba07 
Made the SASL config backwards compatible.
@trevarj
More PR Feedback
1555836
@trevarj
SASL External support for native TLS
db39a09
@trevarj
Fixed clippies
3b10e68
@trevarj
Fixed merge issues with pass cmd feature
934cc02
@trevarj
Copy link
Contributor Author

trevarj commented Dec 18, 2022

Fixed conflicts after pass cmd feature.

Needs testing still.

@trevarj trevarj mentioned this pull request Dec 18, 2022
Open
@trevarj
Copy link
Contributor Author

trevarj commented Dec 19, 2022

@osa1 Tested using new password commands and SASL EXTERNAL. Both working as expected!
If you still want to merge this, it should be a fairly quick code review.

osa1
osa1 reviewed Dec 19, 2022
View reviewed changes
Copy link
Owner

@osa1 osa1 left a comment

Choose a reason for hiding this comment

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

Thanks @trevarj. Code looks good. I will have to read a bit about SASL EXTERNAL before merging this. Hopefully this week..

crates/tiny/src/main.rs Outdated Show resolved Hide resolved
crates/tiny/src/main.rs Outdated Show resolved Hide resolved
crates/tiny/config.yml Outdated
# Providing a path to a PEM file will configure SASL EXTERNAL
# For SASL EXTERNAL certificate and fingerprint generation, see server documentation.
# You will need to register the cert's fingerprint with NickServ
# ex. https://www.oftc.net/NickServ/CertFP/
Copy link
Owner

Choose a reason for hiding this comment

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

Remove this paragraph please. The config is getting quite large. We have an example below on how to use SASL EXTERNAL. We should add this info to the README if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add to the Wiki and link to it from the README? (Since the README is getting large too...)

Copy link
Owner

Choose a reason for hiding this comment

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

👍 moving this to wiki makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Add to wiki once merged

@trevarj
Copy link
Contributor Author

trevarj commented Dec 19, 2022

I will have to read a bit about SASL EXTERNAL

Links that I also put in code comments:
https://ircv3.net/docs/sasl-mechs
https://www.alphachat.net/sasl.xhtml

osa1
osa1 approved these changes Dec 31, 2022
View reviewed changes
Copy link
Owner

@osa1 osa1 left a comment

Choose a reason for hiding this comment

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

Thanks @trevarj. Before merging this, could you update CHANGELOG adding your name to "Thanks to ..." part and also a line to describe these changes? I would do it myself but I don't know what this is about and I don't have time to study it now. If it works I'm happy to merge.

@trevarj
Copy link
Contributor Author

trevarj commented Dec 31, 2022

@osa1 Updated CHANGELOG. I created a stub page on the wiki to add configuration details later.

@osa1 osa1 merged commit 2544793 into osa1:master Dec 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@osa1 osa1 osa1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SASL External
2 participants
@trevarj @osa1