Page MenuHomekolab.org

Proxy authorization for irony/syncroton
ClosedPublic

Authored by mollekopf on Oct 28 2021, 9:10 PM.

Diff Detail

Repository
rK kolab
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 37723
Build 15184: arc lint + arc unit

Event Timeline

mollekopf requested review of this revision.Oct 28 2021, 9:10 PM
mollekopf created this revision.
mollekopf added a reviewer: Restricted Project.Oct 28 2021, 9:17 PM

Please note this builds on some other patches for the docker related stuff.

machniak requested changes to this revision.Oct 29 2021, 10:06 AM
machniak added a subscriber: machniak.

Aren't we authenticate the user two times this way? Once in nginx and once in syncroton/iRony? Should we have an option in syncroton/iRony to disable authentication?

docker/proxy/rootfs/etc/nginx/nginx.conf
88

Looking at syncroton code all requests are being authenticated.

src/app/Http/Controllers/API/V4/NGINXController.php
33–34

For activesync we have to support user\domain syntax. https://git.kolab.org/diffusion/S/browse/master/lib/kolab_sync.php$123

This revision now requires changes to proceed.Oct 29 2021, 10:06 AM

Aren't we authenticate the user two times this way? Once in nginx and once in syncroton/iRony? Should we have an option in syncroton/iRony to disable authentication?

Yes we are authenticating twice. I don't think we should change that (just like we don't for imap).

Ultimately we should plug in our 2fa capable authentication in saslauthd, at which point we no longer require this nginx variant, and in the meantime I don't think we need to introduce further options for what doesn't seem really problematic anyways.

mollekopf added inline comments.Oct 29 2021, 5:34 PM
docker/proxy/rootfs/etc/nginx/nginx.conf
88

I think outlook on android tried to do some unauthenticated stuff, but as you say, syncroton doesn't support that anyways, so I guess that should be fine.

src/app/Http/Controllers/API/V4/NGINXController.php
33–34

I'll look into that.

mollekopf updated this revision to Diff 8615.Nov 15 2021, 10:04 AM

Now with domain.tld\username normalization

machniak requested changes to this revision.Jan 10 2022, 9:48 AM
machniak added inline comments.
src/app/Http/Controllers/API/V4/NGINXController.php
15

$e is redundant here. And I'd put @throws after @return. Also, please add @params.

128

I'd make it a single Log::debug() call, like in byebye().

134

Well, we already log the request headers above. So, this debug line looks redundant.

146

I'm not sure, but I'm wondering whether for cases like this (and 401 above) we should use byebye() method, of course extended to handle all cases, but it would be some unification on the code level (debug logging).

185

I'd make it a single Log::debug() call, like in byebye().

src/tests/Feature/Controller/NGINXTest.php
229

I'd rather use ned@kolab.org who has 2FA configured already. It will not require to unset the setting for john afterwards.

This revision now requires changes to proceed.Jan 10 2022, 9:48 AM
mollekopf marked 8 inline comments as done.Jan 11 2022, 8:53 PM
mollekopf added inline comments.
src/app/Http/Controllers/API/V4/NGINXController.php
134

I think it makes sense to have an explicit message given we stop processing at this point, but the message could be better indeed.

146

I don't think that would help at the moment. The byebye method only works for the imap/smtp responses which uses headers, and the httpauth module uses the http status code instead. While we could introduce a similar method to combine status code + debug message, it doesn't seem to really improve the situation to me (two lines of code vs 1 line + a function).

src/tests/Feature/Controller/NGINXTest.php
229

Personally I think using the same user is clearer, as this makes explicit that we simply test the same situation with a different setting. I don't particularly mind changing to ned@kolab.org, but note that I can't currently remove resetting the settings, since we also change other settings and I'm resetting in tearDown. So unless it's really worth it to avoid setting the settings one to many times, I'd leave it as is.

mollekopf updated this revision to Diff 9280.Jan 11 2022, 8:54 PM
mollekopf marked 3 inline comments as done.

Addressed comments

mollekopf updated this revision to Diff 9304.Jan 11 2022, 10:22 PM

phpcs fixes

Harbormaster completed remote builds in B37897: Diff 9304.
machniak accepted this revision.Jan 12 2022, 7:55 AM
This revision is now accepted and ready to land.Jan 12 2022, 7:55 AM
This revision was automatically updated to reflect the committed changes.