Add SCRAM-SHA-1, SCRAM-SHA-224, SCRAM-SHA-256, SCRAM-SHA-384 and SCRAM-SHA-512 support

[schengawegga]

Added SCRAM-SHA-1, SCRAM-SHA-224, SCRAM-SHA-256, SCRAM-SHA-384 and SCRAM-SHA-512 support.
SCRAM-*-PLUS support is at the moment not possible, because pear/Auth_SASL not supports -PLUS features at the moment.
To work well on PHP8 and above, a PR on pear/Auth_SASL is needed: pear/Auth_SASL#6

[jparise]

My last comment is still worth considering, but looks good overall!

[schengawegga]

My last comment is still worth considering, but looks good overall!

Thanks for your approval.
I checked the static call on my test environtment and it works with the current version of PEAR/Auth_SASL.
But i think it would be better to leave the dynamic call, because before pear/Auth_SASL@47a7165 (in 2017) factory was no static function. A static call will break backwards compatibility, if the user has an old version of PEAR/Auth_SASL installed. And PHP8 will not throw an error, if a static method is called dynamicly. so i think, we should leave it dynamic, to ensure backwards compatibility.

@jparise would you agree with that too?

[Neustradamus]

@schengawegga: Really good job!

Linked to:

• SCRAM-SHA-1(-PLUS) + SCRAM-SHA-256(-PLUS) + SCRAM-SHA-512(-PLUS) supports #57

[schengawegga]

@jparise i found some places we have to change for documentation purposes. First, i have to add the new authentication methods in README.rst. This should be no problem. But, we should update the End user documentation on https://pear.php.net/manual/en/package.networking.net-smtp.intro.php. The new method starttls must also be added here. Can you send me a little hint, how i can change this End user documentation?

[jparise]

@jparise i found some places we have to change for documentation purposes. First, i have to add the new authentication methods in README.rst. This should be no problem. But, we should update the End user documentation on https://pear.php.net/manual/en/package.networking.net-smtp.intro.php. The new method starttls must also be added here. Can you send me a little hint, how i can change this End user documentation?

I'm not sure how or where that's currently managed. I haven't touched it since the old CVS days. I would have expected to be somewhere under https://github.com/pear/pearweb, but I can't find it or a separate "pear manual" repository.

[cweiske]

The documentation is still in SVN: https://svn.php.net/viewvc/pear/peardoc/

[Neustradamus]

@cweiske: Maybe, time to migrate on GitHub?

---

At the same time, it is possible to open issues here:

• https://github.com/pear/Auth_SASL
• https://github.com/pear/Auth_SASL2

And it is possible to detach the fork from the Auth_SASL2 original source?

• https://support.github.com/contact?tags=rr-forks

cc: @CloCkWeRX, @till, ...

[schengawegga]

@cweiske: Maybe, time to migrate on GitHub?

At the same time, it is possible to open issues here:

* https://github.com/pear/Auth_SASL

* https://github.com/pear/Auth_SASL2

And it is possible to detach the fork from the Auth_SASL2 original source?

* https://support.github.com/contact?tags=rr-forks

cc: @CloCkWeRX, @till, ...

@Neustradamus @cweiske

I will migrate this PR into master later, when the changes in the documentations are ready.

[schengawegga]

The documentation is still in SVN: https://svn.php.net/viewvc/pear/peardoc/

Thanks for the information. How i can get an account to svn.php.net and the permissions for Net_SMTP?

[schengawegga]

@jparise I changed the README.rst. Are you agree with that?

[jparise]

@jparise I changed the README.rst. Are you agree with that?

Yes, looks good!

[schengawegga]

And LOGIN has no advantage over PLAIN, as it is 2 round trips. So unless you need this for Outlook compatibility, it is not needed either.

Thanks for your reply. So my suggestion is, to keep the deprecation warning for all these methods, including PLAIN. But I will not remove PLAIN in a future major release, and keep it as deprecated.

All other will be removed in a future major release. Not within the next one, but maybe in 3.0.0. We will see.

@Neustradamus @jparise @cweiske @aamelnikov do you agree with this plan?

[cweiske]

I would not mark PLAIN as deprecated, as it's here to stay.
deprecated means it will be removed some day, but we agreed here that it nneds to be kept.

[schengawegga]

I would not mark PLAIN as deprecated, as it's here to stay.
deprecated means it will be removed some day, but we agreed here that it nneds to be kept.

Ok, I can go with that. But I suggest to do an trigger_error with an E_USER_WARNING that using PLAIN is not secure enough anymore.

How about that?

[Neustradamus]

@schengawegga: Like I have said before and like @cweiske: PLAIN must not be noted "DEPRECATED".

[schengawegga]

@schengawegga: Like I have said before and like @cweiske: PLAIN must not be noted "DEPRECATED".

Yes, I will remove that. But I think we should inform all users wich using this method that they better should use a more secure method.

So I will remove the deprecation for plain but I will trigger a E_USER_NOTICE that PLAIN is unsecure if it will used without TLS.

[Neustradamus]

@schengawegga: Have you progressed on it?

[Neustradamus]

@schengawegga: Several people wait you about the change for "PLAIN" (PLAIN must not be noted "DEPRECATED") and the merging and the release...

Thanks a lot in advance, you have already done an important job...

[schengawegga]

@Neustradamus I was very busy the last few weeks, but now it´s getting better. So i will do that in the next few days.

[Neustradamus]

@schengawegga: Thanks for your last commit!

All people are okay?

[Neustradamus]

@schengawegga: Thanks and good job for 1.11.0!

Can you create new build for Auth_SASL/Auth_SASL2 too?
Please change to have the dev code in master and CRAM-MD5 (etc.) changes too before the new builds:

• https://github.com/pear/Auth_SASL/branches
• https://github.com/pear/Auth_SASL2/branches

[mlocati]

Adding a new trigger is a behaviour change, isn't it?

And changing the behaviour of a point release doesn't respect semver (this PR just broke our systems)

[schengawegga]

Adding a new trigger is a behaviour change, isn't it?

And changing the behaviour of a point release doesn't respect semver (this PR just broke our systems)

I am sorry. I don't understand the problem. This is a backwards compatible change, because I only added a new authentication method and trigger a user deprecation error to the error_handler. This should not have any effect on the previous functioning.

What's the exact problem with server?

[mlocati]

trigger a user deprecation error to the error_handler

Isn't this a behaviour change? What if there's an error handler that breaks the execution in case it intercept trigger_error calls?

[schengawegga]

If triggering a deprecation warning is a behavior change, every PHP Version thats adding a deprecation warning must result in a new major version.

But what is your suggestion for a solution?

[mlocati]

I don't have any. But semver is a contract between developers and users, and that should be respected.

[schengawegga]

I don't have any. But semver is a contract between developers and users, and that should be respected.

I read the semver specification again and think I did the release semver compliant by increasing the minor version:

https://semver.org/#spec-item-7
https://semver.org/#how-should-i-handle-deprecating-functionality

Can you give me a detailed description of what exactly broke your systems and how the configuration of these look like?

[schengawegga]

If I did the release not semver compliant, please let me know what I've done wrong exactly, with a link to the semver specification, and how I can fix it.

I am always open for discussions and constructive critics.

But only writing "I have no suggestion for you" doesn't help me to do things better in the future.

[mlocati]

I see the "if any public API functionality is marked as deprecated" as adding a phpdoc like this

/**
 * @deprecated use bar() instead
 */
public function foo()
{
}

Our systems are pretty picky: they catch any error and notice that's being thrown (as trigger_error() does).

PS: a side note. With this code:

$smtp = Mail::factory(
    'smtp',
    [
        'host' => $host,
        'port' => $port,
        'auth' => true,
        'username' => $username,
        'password' => $password,
        'localhost' => $heloDomain === '' ? 'localhost' : $heloDomain,
    ]
);

getBestAuthMethod() picks LOGIN (deprecated) instead of PLAIN (not deprecated) since it comes first.

Furthermore, I don't get why LOGIN is deprecated and PLAIN not - they both send unencrypted credentials

[Neustradamus]

@mlocati: History: LOGIN has been replaced by PLAIN and PLAIN must be used with TLS connection.

Comments here:

• Add SCRAM-SHA-1, SCRAM-SHA-224, SCRAM-SHA-256, SCRAM-SHA-384 and SCRAM-SHA-512 support #76 (comment)
• Add SCRAM-SHA-1, SCRAM-SHA-224, SCRAM-SHA-256, SCRAM-SHA-384 and SCRAM-SHA-512 support #76 (comment)
• Add SCRAM-SHA-1, SCRAM-SHA-224, SCRAM-SHA-256, SCRAM-SHA-384 and SCRAM-SHA-512 support #76 (comment)

PLAIN is in SASL2 I-D Draft:

• https://datatracker.ietf.org/doc/html/draft-melnikov-sasl2

[mlocati]

Thank you for this useful summary (this PR has ways too many comments provided my limited time)

[Neustradamus]

@mlocati: It is normal that there are a lot of comments, it is an important PR, a good job of @schengawegga!
Now it must be added in other packages too...

[schengawegga]

@mlocati i released a bugfix now. there are no error_triggers left. i will activate them in 2.0.0, to mark it clearly as a change of the behavior and the finally deprecation warning. all deprecated authentication methods will be deleted at version 3.0.0. I think, that is a good way to publish this deprecations. do you agree?

[mlocati]

Yep, I agree.
But IMHO it'd be fine to to remove those deprecated methods in v2 too without having to wait for v3.