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

Implement SCRAM-SHA-256 #175

Closed
DanielOaks opened this issue Dec 23, 2017 · 10 comments · Fixed by #1764
Closed

Implement SCRAM-SHA-256 #175

DanielOaks opened this issue Dec 23, 2017 · 10 comments · Fixed by #1764
Milestone

Comments

@DanielOaks
Copy link
Member

This is something I wanna do as per https://tools.ietf.org/html/rfc7677 and it being recommended by IRCv3. Keep in mind ircv3/ircv3-specifications#326

@slingamn
Copy link
Member

slingamn commented Feb 6, 2019

This will help: https://github.com/jesopo/bitbot/blob/master/modules/sasl/scram.py

@slingamn slingamn added this to the v1.2 milestone May 17, 2019
@slingamn slingamn modified the milestones: v1.2, v1.3 Nov 8, 2019
@slingamn
Copy link
Member

This implementation is officially endorsed by Mongo (heh heh): https://github.com/xdg-go/scram

@slingamn slingamn mentioned this issue Dec 16, 2019
@slingamn
Copy link
Member

I've decided this isn't a priority for me, but I'm leaving the "help wanted" tag up because I'm definitely interested in outside PRs that add this.

@slingamn slingamn removed this from the v1.3 milestone Dec 27, 2019
@Mikaela
Copy link
Contributor

Mikaela commented Jun 10, 2021

Considering how recent Charybdis/fork+Atheme support this mechanism, this may be a offered mechanism downgrade for networks migrating to Ergo.

I think e.g. WeeChat lets you specify only one mechanism to be used for SASL authentication instead of preferred mechanism list and it recently added support for this.

Another argument for supporting the mechanism is that Ergo is leading in all other IRCv3 implementation and testing SCRAM has to be done separately somewhere else.

@slingamn
Copy link
Member

Another argument for supporting the mechanism is that Ergo is leading in all other IRCv3 implementation and testing SCRAM has to be done separately somewhere else.

I like this point!

I have been avoiding this because it is somewhat difficult to implement: it's not just a matter of adding the algorithm, we also have to bump the CredentialsVersion and handle re-hashing passwords as users log back in:

ergo/irc/accounts.go

Lines 1894 to 1902 in 97d1786

type CredentialsVersion int
const (
CredentialsLegacy CredentialsVersion = 0
CredentialsSHA3Bcrypt CredentialsVersion = 1
// negative numbers for migration
CredentialsAtheme = -1
CredentialsAnope = -2
)

since a bcrypt hash is not enough to support this mechanism, we need to obtain the plaintext password (temporarily) and hash it with PBKDF2.

@Mikaela
Copy link
Contributor

Mikaela commented Jul 8, 2021

I have seen SASL SCRAM also been suggested as a mitigation towards not leaking plaintext password to a typosquatting network recently.

@slingamn
Copy link
Member

https://github.com/xdg-go/scram adds about a megabyte to the binary size on linux/amd64, mostly due to this:

https://github.com/xdg-go/stringprep

It's unclear whether we can get away with commenting out the uses of stringprep. RFC 5802 says:

Note that implementations MUST either implement SASLprep or disallow use of non US-ASCII Unicode codepoints in "str".

which would seemingly apply to both usernames and passwords.

@slingamn
Copy link
Member

Proposal:

  1. Fork xdg-go/scram, remove stringprep
  2. SCRAM is not supported for account names with non-US-ASCII characters; we'll send ERR_SASLFAIL at some point in the process. Compare consider making 'ascii' the default/recommended casefolding #1718.
  3. Going forward, non-US-ASCII characters are disallowed by validatePassphrase

@Mikaela
Copy link
Contributor

Mikaela commented Jul 27, 2021

SCRAM is not supported for account names with non-US-ASCII characters; we'll send ERR_SASLFAIL at some point in the process. Compare consider making 'ascii' the default/recommended casefolding #1718.

I am not sure on this point as it seems very anglocentric and I think it's in clash with

('precis' would remain fully supported in the codebase, especially because operators can't safely switch between casefoldings, at the risk of making account or channel registrations unusable.)

from #1718 (comment).

@slingamn
Copy link
Member

From discussion, we're going to reimplement stringprep, relying on PRECIS to enforce most of the constraints. If an account name passes PRECIS, only a few more steps should be necessary:

  1. Remove 20 or so prohibited runes: https://github.com/xdg-go/stringprep/blob/4fcaaf19bc2a8e388ba275a82eb34c29abcd0bc9/tables.go#L411
  2. Normalize some space characters (these are likely prohibited by PRECIS anyway): https://github.com/xdg-go/stringprep/blob/4fcaaf19bc2a8e388ba275a82eb34c29abcd0bc9/saslprep.go#L3
  3. Apply NFKC: https://github.com/xdg-go/stringprep/blob/4fcaaf19bc2a8e388ba275a82eb34c29abcd0bc9/profile.go#L36

PRECIS doesn't apply to passwords, but we can probably just get away with violating the spec for the (hopefully small and atypical) set of Unicode passphrases that don't pass bidi or similar.

@slingamn slingamn linked a pull request Jul 30, 2021 that will close this issue
@slingamn slingamn added this to the v2.8 milestone Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants