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

SCRAM-SHA-1(-PLUS) + SCRAM-SHA-256(-PLUS) supports #2442

Closed
Neustradamus opened this issue Sep 6, 2019 · 49 comments
Closed

SCRAM-SHA-1(-PLUS) + SCRAM-SHA-256(-PLUS) supports #2442

Neustradamus opened this issue Sep 6, 2019 · 49 comments
Assignees
Labels
community Non ESL issues and PRs in backlog Community request added to backlog and waiting for implementation.

Comments

@Neustradamus
Copy link

Neustradamus commented Sep 6, 2019

"When using the SASL SCRAM mechanism, the SCRAM-SHA-256-PLUS variant SHOULD be preferred over the SCRAM-SHA-256 variant, and SHA-256 variants [RFC7677] SHOULD be preferred over SHA-1 variants [RFC5802]".

Can you add support for?

SCRAM-SHA-512(-PLUS):

SCRAM-SHA3-512(-PLUS):

https://xmpp.org/extensions/inbox/hash-recommendations.html

-PLUS variants:

LDAP:

  • RFC5803: Lightweight Directory Access Protocol (LDAP) Schema for Storing Salted: Challenge Response Authentication Mechanism (SCRAM) Secrets: https://tools.ietf.org/html/rfc5803

HTTP:

2FA:

IANA:

Note:
RFC6331: Moving DIGEST-MD5 to Historic

Linked to:

@fenek
Copy link
Member

fenek commented Sep 6, 2019

Hi @Neustradamus

This is definitely a very good idea. Sadly, we probably won't manage to add them for the next release but I'll add this request to our internal backlog.

@fenek fenek added the community Non ESL issues and PRs label Sep 6, 2019
@fenek fenek self-assigned this Sep 6, 2019
@fenek fenek added the in backlog Community request added to backlog and waiting for implementation. label Oct 7, 2019
@Neustradamus

This comment was marked as spam.

@fenek
Copy link
Member

fenek commented Nov 12, 2019

We haven't forgotten about this but the upcoming release is currently focused on different items.

@michalwski
Copy link
Contributor

@Neustradamus can you explain (or point as to a documentation) how existing accounts using SCRAM-SHA-1 can be migrated to SCRAM-SHA-256. If I understand it correctly there is no other option as setting new password with the new mechanism.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@michalwski
Copy link
Contributor

@Neustradamus Happy New Year 2020!

The next MongooseIM release is planned for end of January. I'll make sure this issue gets done as one of the first after the release.

And thanks for reminding about this!

@Neustradamus

This comment was marked as spam.

@michalwski
Copy link
Contributor

We have some other things planned for the upcoming release and I'm not sure we'll be able to fit this one.

Just for the record, MongooseIM already supports SCRAM-SHA-1 since version 1.4.0 which is 6 years old.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@michalwski
Copy link
Contributor

Hi @Neustradamus, this is one of our top priorities for the next release. I did initial research regarding the possibility of adding -plus variants and it may not be easy to achieve. The plan minimum is to add support of SCRAM-SHA-256 (SCRAM-SHA-1 is already supported).

Also I'm wondering how important is it to allow users to switch between the methods. If I understand it correctly in order to allow both -SHA-1 and -SHA-256 the password would have to be stored in a way that allows us to get the plain version from which the chosen method can proceed.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@janciesla8818
Copy link
Contributor

Hi @Neustradamus,

Thanks for the provided suggestions, much appreciated! We are still working on those modules and the naming might still change. It will mostly depend on the functionalities and differences to '_sha256' modules. We'll let you know where do we land at.

@michalwski
Copy link
Contributor

Hi @Neustradamus,

Thanks for keeping a close and detailed eye on the SCRAM-SHA-* progress in (probably) all XMPP related projects!

Answering to your questions:

  1. Naming convention, this is a good idea.
    1. All new SCRAM-SHA mechanism will have it's own module cyrsasl_scram_shaNUM module.
    2. There will be one mongoose_scram module, since this is a helper module and all the functions can be parametrise with the hashing algorithm.
    3. Names in tests can be adjusted to what you suggested.
  2. -PLUS variants - We are going to try implementing it. It's difficult, because currently there is no easy way to extract the low-level TLS data in order to do the channel_binding. Current tls library needs to be extended in order to provide us this information. We'll see how the research around this goes, it may turn out that -PLUS variants will be added later.
  3. SCRAM (224/384/512) - Adding SCRAM 256 we are doing required refactoring (also around storing the hashes for different algorithms in db) which will allow us to easily add other algorithms. I think this can be achieved within this release.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@michalwski
Copy link
Contributor

Thanks for the PRs @Neustradamus :) I merged #2719 and the change you proposed in #2720 will be included in a PR which @janciesla8818 is currently working on.

Thanks for keeping a close eye on the SCRAM-SHA-* improvements!

@michalwski
Copy link
Contributor

Regarding the following question from @Neustradamus

An admin can configurate PLAIN or SCRAM-SHA-1 or SCRAM-SHA-256 or several in same time?

An admin will be able to configure:

  1. The stored password format, by default hashes for all the supported algorithms will be stored, but it will be possible to store only selected algorithms.
  2. The advertised auth mechanisms, when advertising the mechanisms MongoseIM will make sure that only these are advertised which have corresponding hash as defined by the stored password format configuration.

@Neustradamus

This comment was marked as spam.

@michalwski
Copy link
Contributor

Do you mean the order on the list of supported mechanisms when the server advertises it to the client?

@Neustradamus

This comment was marked as spam.

@michalwski
Copy link
Contributor

@Neustradamus the advertised list of authentication mechanisms can be adjusted. @janciesla8818 what do you think?

When it comes to selecting the authentication mechanism by the client, my understanding is that if the client is capable of channel binding and advertises it to the server, it will not be allowed to use other scram mechanisms than -PLUS variant. Is my understanding correct?

Moreover, when the server advertises an authentication mechanism, it will be possible for the client to authenticate with the mechanism unless:

  1. The server doesn't have the hash for a particular version because the list of the mechanisms was extended but passwords were not reset so new hash was not generated.
  2. The client doesn't provide valid data

At this point, I don't see much value (on the server side) in adding such test case which iterates over all the mechanisms.

@Neustradamus

This comment was marked as spam.

@michalwski
Copy link
Contributor

Hi @Neustradamus I think the last PR in this topic is this one #2745
Do you think that after merging it, the issue can be considered done?

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@michalwski
Copy link
Contributor

Hi @Neustradamus, after releasing 3.7 we took a break for breathing and focusing on some other matters. When it comes to the test on xmpp.net the result may be dissatisfying for you. MongooseIM currently advertises and allows to authenticate even before STARTTLS sessions. It can be configured to require STARTTLS, in this case, the authentication will be rejected but still advertised on the plain connection. Could you point us to a XEP, describing the best practices regarding TLS and advertised features?
I think this is something we can improve. It'd be better to cover the requirement in a separate issue.

@michalwski
Copy link
Contributor

When it comes to LDAP authentication backend, currently it works only with SCRAM-EXTERNAL and SCRAM-PLAIN.

@Neustradamus

This comment was marked as spam.

@NelsonVides
Copy link
Collaborator

NelsonVides commented Oct 22, 2020

Hi @Neustradamus,

I'm gonna try to keep these topics organised.

  • When it comes to LDAP+SCRAM, I think we can move that discussion to the new issue you've just opened (LDAP + SCRAM-SHA-1 and more #2917).
  • XMPP.net testing is a different topic unrelated to this open issue that we can discuss appropriately elsewhere. It is on my personal backlog I'd add, but it has lower priority at the moment.

With that said, I'd like to close this old issue, I believe it has been solved. Let me know if you're fine with this.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@NelsonVides
Copy link
Collaborator

Hi @janciesla8818, @NelsonVides, @michalslaski: For your information, there was a SCRAM problem in Conversations:

* [iNPUTmice/Conversations@2a57c92](https://github.com/iNPUTmice/Conversations/commit/2a57c92f63afbe6c8627b84ba959614c2667de96)

Have you feedbacks about SCRAM usage statistics?

@Neustradamus That issue in Conversations has nothing to do with the protocol, but rather with their internal implementation of the serverkey-clientkey, so we have no concerns about that. Usage statistics, we don't have anything to share for the moment.

@Neustradamus

This comment was marked as spam.

@NelsonVides
Copy link
Collaborator

Hi @janciesla8818, @NelsonVides, @michalslaski: It is official for TLS 1.3 Binding!

* RFC 9266: Channel Bindings for TLS 1.3: https://tools.ietf.org/html/rfc9266

Details:

* tls-unique for TLS =< 1.2

* tls-exporter for TLS = 1.3

For your information, SCRAM-BIS is in progress: https://tools.ietf.org/html/draft-melnikov-scram-bis.

Linked to:

* [RFC 9266: Channel Bindings for TLS 1.3 support #3721](https://github.com/esl/MongooseIM/issues/3721)

Hi @Neustradamus,

I've seen the TLS 1.3 channel binding, it's on my personal task list. But first I need support for that in the underlying TLS libraries, and also, some code cleanup in MIM usage of the TLS libraries. So it'll take a while for this to be done, you don't need to remind me, I won't forget, but it won't be soon I'm afraid.

Thanks for the heads up!

PS: you probably wanted to tag @michalwski, not @michalslaski ;)

@Neustradamus

This comment was marked as spam.

@NelsonVides
Copy link
Collaborator

Thanks @NelsonVides for your answer :)

The question is why @michalwski has done a change of username? ^^

No change of username, those are two different Michałs 😛

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

@Neustradamus

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Non ESL issues and PRs in backlog Community request added to backlog and waiting for implementation.
Projects
None yet
Development

No branches or pull requests

5 participants