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

isync: add optional support for XOAUTH2 authentication method #235148

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

wentasah
Copy link
Contributor

Description of changes

Based on discussion around #108480 (comment), we should make it easier for users to use isync with support for the XOAUTH2 authentication method. This PR allows users to enable the integration simply by:

isync.override { withCyrusSaslXoauth2 = true; }

cc @jackmac92

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@lheckemann
Copy link
Member

This only adds 0.1MiB of closure size to isync, so I think we should enable this by default, or even make it completely unconditional. The condition name is pretty unwieldy and only discoverable by reading the package expression, and if we enable it by default we'll get a version with support in the binary cache.

@wentasah
Copy link
Contributor Author

Ok. I enabled it by default but kept it conditional if somebody needs an unwrapped binary for some reason.

@lheckemann lheckemann merged commit f75e78d into NixOS:master Jun 1, 2023
21 checks passed
@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

Successfully created backport PR for release-23.05:

@dschrempf
Copy link
Contributor

dschrempf commented Jun 4, 2023

Hi! After this PR, when synchronizing mails with mbsync -a I get

IMAP command 'AUTHENTICATE XOAUTH2 SOMEHASH=' returned an error: NO [AUTHENTICATIONFAILED] Invalid credentials (Failure)

I have been and I am using an application password. Is there a tutorial or more extensive information available about how to set up xoauth2 with mbsync? Thank you!

EDIT: Deactivating XOAUTH2 by using

  pkgs.isync.override { withCyrusSaslXoauth2 = false; };

fixes the authentication issue, but of course also deactivates XOAUTH2.

@wentasah
Copy link
Contributor Author

wentasah commented Jun 4, 2023

Hi, I'm currently traveling without a computer. I guess that xoauth2 authentication gets higher priority than other methods. You might be able to disable it by AuthMechs in mbsync config.

I use xoauth2 with https://github.com/harishkrupo/oauth2ms, which gets the authentication token. I'll write more after getting to a computer.

@dschrempf
Copy link
Contributor

Hi.

I tried using

AuthMechs PLAIN

in my .mbsyncrc, but then mbsync complains:

IMAP error: selected SASL mechanism(s) not available;
   selected: PLAIN
   available: EXTERNAL XOAUTH2

I think this is because this PR overwrites the SASL lib dir, which only provides XOAUTH2. Could that be?

Further, I realized the XOAUTH2 is obsolete: See http://www.iana.org/assignments/sasl-mechanisms/sasl-mechanisms.xhtml.

At the moment, I need to use the override to be able to use mbsync. I would think this also affects others.

primeos added a commit to primeos/nixpkgs that referenced this pull request Jun 4, 2023
The XOAUTH2 support was recently added in 47eda8e but apparently it
causes regressions when using other SASL methods.
An example error message:
```
IMAP command 'AUTHENTICATE XOAUTH2 SOMEHASH=' returned an error: NO [AUTHENTICATIONFAILED] Invalid credentials (Failure)
```

The cause seems to be that overriding `SASL_PATH` drops all available
SASL mechanisms from `cyrus_sasl`, so only `XOAUTH2` (and `EXTERNAL`)
will be left. See [0] and the following comments for more details.

We'd need to set `SASL_PATH` to a combination/merge of
`${cyrus_sasl}/lib/sasl2` and `${cyrus-sasl-xoauth2}/lib/sasl2`.
Anyway, it seems best to disable the XOAUTH2 support by default due to
the two other concerns mentioned in the comment.

[0]: NixOS#235148 (comment)

Reported-by: Dominik Schrempf <dominik.schrempf@gmail.com>
@primeos
Copy link
Member

primeos commented Jun 4, 2023

Thanks a lot for your report @dschrempf! :)
IMO there are definitely enough reasons to disable the XOAUTH2 support by default so I've opened #235959 for it.

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

Successfully merging this pull request may close these issues.

None yet

5 participants