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 SASL mechanisms to the software table. #48

Merged
merged 2 commits into from
Dec 17, 2015
Merged

Add SASL mechanisms to the software table. #48

merged 2 commits into from
Dec 17, 2015

Conversation

SadieCat
Copy link
Contributor

I'm abusing the required attribute to show the ones that people "should" be implementing. If anyone has a better idea for this then I can change it.

@grawity
Copy link
Contributor

grawity commented Dec 17, 2015

maybe there should be a more generic field "status: required/optional/deprecated/dear gods no"

@DanielOaks
Copy link
Member

This looks nice, and thanks for that partial fix.

I'm against showing DH-AES and DH-BLOWFISH on the site in the same way we're showing everything else. For that matter, given #11 I'm very iffy on including ECDSA-NIST256P-CHALLENGE on there as well.

If we display them at all, my initial thought is to add a 'deprecated' class to every cell in that column and make them very desaturated, darker grey versions of the green, and just a darker grey instead of red for not supported. And for the title attribute of those cells to be set to DEPRECATED to make it a bit more clear on hovering over them. Hell, maybe even instead of No, we just write Deprecated or something similar to make it very clear that we don't like those mechanisms and that not implementing them isn't a fault.

That sort of thing could definitely be done with some creative extension of the irc_versions.yml file. And I could always hack away at it 'til I get something that mostly works all else fails. But I think it would probably just be easier and cleaner to see whether that column is marked deprecated in irc_versions.yml, and if so then just hide it.

Regarding SCRAM-SHA-256, I don't know enough about crypto and SASL to take a proper look at the whole 'required' thing. It looks alright to me, but I'd want some other people to weigh in on whether or not it should be there.

I definitely really like the idea. I'm just a little iffy about including it right here, along with those deprecated/non-standard/etc concerns. If we do have this, maybe a services page would be in order sometime after this is merged into master?

edit: Just for reference with the "iffy about including it right here" comment. I feel that we should basically only include things we want people to support on these pages. If it's on the specific SASL mechanisms page, I'm more open to including the DH-* mechanisms (with appropriate colour-coding/etc to show they're deprecated).

I feel like that would also just be a more appropriate place for them in general, although it complicates the whole yml layout somewhat. Basically creating a new irc_versions.yml file specifically for the docs things, so for batch types and for SASL mechanisms and such (whether in a single file, or as two separate files, and more as we expand it). I can look at this if necessary, it seems like it'd probably be a pretty large change.

@grawity
Copy link
Contributor

grawity commented Dec 17, 2015

Regarding SCRAM-SHA-256, I don't know enough about crypto and SASL to take a proper look at the whole 'required' thing. It looks alright to me, but I'd want some other people to weigh in on whether or not it should be there.

Earlier, XMPP has chosen SCRAM-SHA-1 as the new "required" mechanism (replacing the previous DIGEST-MD5 requirement). As non-cryptographer, I think it looks good when properly implemented – unlike my earlier Atheme module. (That is, services must only store ServerKey and StoredKey, because PBKDF2(password) is password-equivalent.)

@Mikaela
Copy link
Contributor

Mikaela commented Dec 17, 2015

I'm against showing DH-AES and DH-BLOWFISH on the site in the same way we're showing everything else.

👍. Listing them makes it seem that all clients which removed support for them when they were declared insecure and removed wrong Atheme are doing something wrong. At least ZNC did this.

This part has been edited multiple times as I was proven wrong on HexChat and WeeChat which actually do support DH-*.

For that matter, given #11 I'm very iffy on including ECDSA-NIST256P-CHALLENGE on there as well.

I have no own opinion, but would assume that @kaniini doesn't want it to be included based on them not wanting to include it on SASL mechanisms page as seen in #11.

Otherwise I like the idea.

I'm abusing the required attribute to show the ones that people "should" be implementing.

I also ask in this thread, where else than in this PR the mechanisms that "should" be implemented are listed?

@@ -71,6 +80,11 @@
- server-time
- monitor
- userhost-in-names
SASL:
- dh-aes
- dh-blowfish
Copy link
Contributor

Choose a reason for hiding this comment

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

Source for dh-aes and dh-blowfish being supported by HexChat? I see only SASL (username + password) and SASL EXTERNAL (cert) in the login methods and am quite sure that they were removed long time ago.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong again < TingPing> Mikaela, it still does though it was planned to be removed

@DanielOaks
Copy link
Member

I also ask in this thread, where else than in this PR the mechanisms that "should" be implemented are listed?

The http://ircv3.net/docs/sasl-mechs.html page does already list these mechanisms specifically. Aside from that, I'm leaving this issue open for opinions on those 'highlighted' ones.

@DanielOaks DanielOaks merged commit 45cd8d8 into ircv3:master Dec 17, 2015
@DanielOaks
Copy link
Member

Merged this into master manually, with the table now living on the sasl-mechs page

@SadieCat SadieCat deleted the master+sasl branch December 17, 2015 21:47
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.

None yet

5 participants