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

[🚧 WIP] SASL refactoring and new mechanisms #78

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

[🚧 WIP] SASL refactoring and new mechanisms #78

wants to merge 2 commits into from

Conversation

nevans
Copy link
Collaborator

@nevans nevans commented Nov 21, 2022 •
edited

NOTE: This PR started as a big rough-draft for many of the other PRs listed below. Rather than close the PR and create a new tracking issue, I've been keeping the branch around as a set of experimental implementations for some of the TODO list items, while cherry-picking parts of it into their own PRs when they are ready.

@nevans nevans requested review from hsbt and shugo November 21, 2022 17:44
@nevans
Copy link
Collaborator Author

nevans commented Nov 21, 2022

n.b this PR is currently based on #70, #71, #72, #73, #74, #75, and #76. I can rebase part of all of this on master, but I think those will be merged before this is.

Also, it's currently failing 2.6, because I used some numbered parameters in a few places. As discussed on #68, we can remove support for 2.6.

@nevans nevans mentioned this pull request Nov 21, 2022
Closed
@Neustradamus
Copy link

@nevans: Good job!

@nevans nevans changed the base branch from docs-etc to docs December 1, 2022 22:27
@nevans nevans force-pushed the sasl-sasl-sasl branch 2 times, most recently from 87a0365 to 26b4d74 Compare December 1, 2022 23:29
@nevans nevans deleted the branch master December 20, 2022 17:36
@nevans nevans closed this Dec 20, 2022
@nevans nevans reopened this Dec 20, 2022
@nevans nevans changed the base branch from docs to client-docs December 20, 2022 19:49
nevans added a commit that referenced this pull request Dec 21, 2022
@nevans
📚 Update docs for #authenticate, #login, etc...
5343336 
The documentation on these methods is meant to complement a new
SASL::Authenticator base class and new documentation on each of the
individual authenticator classes.  See #78 and #82.

That base class is added in another PR (#78), but the documentation for
these methods can be updated without it.

Also, somehow I misremembered `LOGINDISABLED`: it only applies to
`LOGIN`, not `AUTHENTICATE`!  This fixes that error.
Base automatically changed from client-docs to master December 22, 2022 15:05
@Neustradamus
Copy link

@nevans: Happy New Year!

Little question, have you a timeline?

@nevans nevans added the SASL 🔒 Authentication and authentication mechanisms label Feb 12, 2023
@nevans nevans mentioned this pull request Apr 28, 2023
Merged
@nevans nevans force-pushed the sasl-sasl-sasl branch 2 times, most recently from 078ea33 to fd56ca3 Compare June 13, 2023 22:35
@nevans nevans force-pushed the sasl-sasl-sasl branch 2 times, most recently from 43180a5 to 1a8deb3 Compare July 29, 2023 14:54
@Neustradamus
Copy link

@nevans: I think that you can remove CRAM-MD5, DIGEST-MD5, LOGIN from all.

@nevans nevans force-pushed the sasl-sasl-sasl branch 3 times, most recently from 3076a03 to 0af0c6d Compare September 13, 2023 12:56
@nevans nevans force-pushed the sasl-sasl-sasl branch 4 times, most recently from 52c19d6 to de7fea5 Compare September 25, 2023 13:01
@nevans nevans force-pushed the sasl-sasl-sasl branch 2 times, most recently from efcfcf4 to 9222f15 Compare September 26, 2023 06:44
@nevans
🔒 SASL DIGEST-MD5: realm, host, service_name, etc
cc5f09c 
Yes, DIGEST-MD5 is deprecated!  But that also means that it was lower
risk for experimenting with other SASL changes.  It's complexity vs most
other mechanisms makes it a good test-bed for the completeness of
net-imap's SASL implementation: e.g: it demonstrated that we were
missing features such as `done?`, demonstrates the utility of using
callbacks for attributes such as `realm` (the user might select from a
server-provided list), it shows that `service` cannot be hard-coded to
`imap` and must be provided by the client, and requires other attributes
that should be provided by the client such as `host`, `port` (also used
by `OAUTHBEARER`).

I improved the existing authenticator in several ways:

* ✨ User can configure `realm`, `host`, `service_name`, `service`.
  This allows a correct "digest-uri" for non-IMAP clients.
* 🔒 Use SecureRandom for cnonce (not Time.now + insecure PRNG!)
* ✨ Default `qop=auth` (as in RFC)
* ✨ Enforce requirements for `sparam` keys (required and no-multiples).
* ♻️  Refactor toward the style used in the new ScramAuthenticator.

However... it's still deprecated, so don't use it! 🙃
@nevans
🚨 SASL: add_authenticator warns on reassignment (🚧 ???)
a11f1ce 
The question is: do we even want to this?
@nevans nevans mentioned this pull request May 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsbt hsbt Awaiting requested review from hsbt

@shugo shugo Awaiting requested review from shugo

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
SASL 🔒 Authentication and authentication mechanisms
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nevans @Neustradamus