-
-
Notifications
You must be signed in to change notification settings - Fork 37
Add support for SASL EXTERNAL #352
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
Conversation
I can successfully login with this code, however the GUI "login method" dropdown doesn't seem to be updated yet. Also, I cannot join channels, but this may be due to the non-standard setup of the specific internal server. |
Indeed, I forgot. Done.
Yeah, doesn't seem related |
Great work, thanks for digging into this! |
@@ -580,6 +621,7 @@ | |||
<widget name="login_password_entry"/> | |||
<widget name="remember_login_password_check_button"/> | |||
<widget name="login_cert_file_chooser_button"/> | |||
<widget name="client_cert_file_chooser_button"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client_cert name is coufusing, how about rename the login_cert to ecdsa_cert and this to another better name? such as sasl_external_cert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I wanted to keep the names here consistent with the name already in use in the config for ecdsa. Should I also change login.certificate
to something like login.ecdsa_certificate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can add a new confval like login.ecdsa_certificate
, but keep it compatible with the old login.certificate
confval.
} | ||
g_tls_connection_set_certificate(G_TLS_CONNECTION(tls_conn), cert); | ||
LOG_FR("Using client TLS certificate"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thinks this is why we have to configure cert in server section? Keep status quo is fine. We can expose a SIRC_EVNET in future.
Closes GH-246.
Tested on Ergo.Chat.
This might be a little confusing, as it requires a
method
configured in theuser
section of the config, and thecertificate
configured in theserver
section. (Though it works with only the latter on some servers, but this is called CertFP, not SASL EXTERNAL.)I was not able to do otherwise without refactoring all the configuration handling.
Additionally, the reason for adding this new
server->certificate
field is that this is a TLS cert, not an ECDSA cert like the existinguser->certificate
field. (btw, ECDSA authentication is deprecated by IRCv3 (including the original author))