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

CRM-20561 Use Composer to install Auth_SASL #10385

Merged
merged 1 commit into from
May 23, 2017

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented May 19, 2017

@seamuslee001
Copy link
Contributor Author

@totten Tim this is a diff between the composer version and packages version https://gist.github.com/seamuslee001/e24e709a854339924647f9a6fe227103

@totten
Copy link
Member

totten commented May 22, 2017

Discussed a bit on Mattermost. Some general comments:

  • This newer version seems to have some code-style cleanup/improvement. Huzzah.
  • It looks to support more authn modes (and more forgivingly). That's probably good.
  • In theory, the client-server negotiations (with smtpd's and imapd's) could sort out a bit differently when adding new protocols. The only problem scenario I can imagine would be if the new protocol implementations (eg SCRAM*) have incompatibilities. But we're really not in a position to test for smtpd's/imapd incompatibilities -- that's why we rely on a library.

+1 for using updated Auth_SASL

@totten
Copy link
Member

totten commented May 23, 2017

Applied both patches locally:

$ git scan am https://github.com/civicrm/civicrm-packages/pull/185 https://github.com/civicrm/civicrm-core/pull/10385
$ composer install

Tried a couple things:

  • In cv cli, executed $digest = Auth_SASL::factory('digestmd5');. This seemed to work.
  • In Civi GUI, configured an authenticated SMTP service (fastmail.com) and successfully sent a test message.

These sniff-tests are OK but not awesome -- the main limitation is that my testing didn't actually use CRAM-MD5, DIGEST-MD5, or SCRAM*. (The fastmail.com smtpd authenticates using LOGIN/PLAIN-over-SSL.) I'd prefer someone with a relevant smtpd try it out, but I guess it would be OK if we (a) merge #10385 and civicrm/civicrm-packages#185 and (b) also strongly emphasize the need for testing in the 4.7.21 RC.

@Neustradamus
Copy link

Dear @civicrm team,

Can you update Auth_SASL, Net_SMTP, etc.?

@demeritcowboy
Copy link
Contributor

@Neustradamus I'm expecting pear/db to do a release shortly which would be good for our php 8.2 drive. We could look at doing them all at once.

@Neustradamus
Copy link

@demeritcowboy: A lot of people like the improvements of @schengawegga and wait several new repository builds :)
Recently, he has done a comment here: pear/DB#20 (comment).
I hope before christmas for more security...

@schengawegga
Copy link

@demeritcowboy @Neustradamus PEAR/DB v1.12.0 released ;-)

@Neustradamus
Copy link

@schengawegga
Copy link

@schengawegga: Thanks!

PEAR/DB 1.12.1 too:

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

* https://github.com/pear/DB/releases

* https://pear.php.net/package/DB/

There was an urgent needed bugfix in the pear package. Thanks @remicollet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants