Implement SASL authorisation identity #3790

captain-caveman2k · 2019-04-17T23:41:36Z

This patch set adds support for the authorisation identity to IMAP, POP3 and SMTP when using the PLAIN SASL authentication mechanism.

Questions:

Do we need to take the option into account in url.c when re-using connections (I think we do)
Do we need to populate conn->sasl_authzid with old_conn->sasl_authzid in url.c::reuse_conn()

bagder · 2019-04-19T20:37:27Z

docs/cmdline-opts/sasl-authzid.d

+Help: Use this identity during SASL PLAIN authentication
+Added: 7.65.0
+---
+Use this identity during SASL PLAIN authentication.


I think this could use a little expanding as if you're not a SASL PLAIN expert, this is really hard to understand.

I have expanded this - could you please take a look and let me know what you think?

bagder · 2019-04-19T20:38:25Z

docs/libcurl/opts/CURLOPT_SASL_AUTHZID.3

+authorisation identity for the transfer. Only applicable to the following
+supporting SASL authentication mechanisms:
+
+* Plain


Plain? If there's really only one method that curl supports that this works for, then I think having it show as a list seems to complicate the description more than necessary.

I have updated as suggested.

bagder · 2019-04-19T20:39:02Z

docs/libcurl/opts/CURLOPT_SASL_AUTHZID.3

+.SH DESCRIPTION
+Pass a char * as parameter, which should be pointing to the zero terminated
+authorisation identity for the transfer. Only applicable to the following
+supporting SASL authentication mechanisms:


Perhaps also a word about what this string is or is used for for those who hasn't read the RFC lately?

Since we've supported SASL PLAIN logins already since before without this option, I think it could make sense to also describe what happens if a user doesn't set this option while trying SASL PLAIN.

I have updated.

jay · 2019-04-20T22:46:44Z

I think you might be early on the approval correct me if I'm wrong but this looks like WIP. The commits should be squashed into one titled in the present tense and I don't see where conn->authzid comes about from data->set.str[STRING_SASL_AUTHZID] that part looks incomplete. The doc says the default is blank but shouldn't it be NULL? If a dev wants to reset it are they doing this curl_easy_setopt(curl, CURLOPT_SASL_AUTHZID, "") or curl_easy_setopt(curl, CURLOPT_SASL_AUTHZID, NULL)? I think it should be the latter, maybe a blank entry is valid (or maybe not I don't know sasl but anyway I like NULL for consistency with the other options).

captain-caveman2k · 2019-04-21T11:54:48Z

@jay - Yes this is work in progress but this PR allows others to comment and give feedback through the development process and for Savageman to see whether it meets his requirements. Please note: There are still open questions above about the connection re-use.

Initialisation of conn->authzid - Fixed, tidied up (renamed for consistency) and pushed. Thank you.

Squashing - as you know I am dead against having huge monoethnic commits and encourage more contributors to split their commits up into smaller functional changes/units (just like I was encouraged to do back in 2011 when I joined the project), as it a) helps the review process and b) helps identify where problems are introduced through the use of git bisect for example. (As per our guidelines: https://curl.haxx.se/dev/contribute.html). The only commit that should probably be squashed is the correction of existing tests which I have now reworked into it's own functional unit (Now the second commit).

Docs - Default is blank or NULL. I have no preference. blank is what was present from the document I copied to create this one (either CUROPT_MAIL_FROM or CURLOPT_PASSWORD - I can't remember which).

Reset of option - Either should work. In the event that NULL is passed to the Curl_auth_create_plain_message() function, it will detect this and set zlen to 0. In the event that "" is passed it will use strlen() which will return 0 and also set zlen to the same.

bagder · 2019-04-21T20:26:51Z

Can I suggest you use [WIP] (or similar) in the title then while the PR is not yet ready?

jay · 2019-04-21T20:30:18Z

Yes this is work in progress but this PR allows others to comment and give feedback through the development process and for Savageman to see whether it meets his requirements.

Yes, totally appropriate. I was only pointing out it looked like a WIP (sometimes titled as [WIP]) and early on the approval.

Squashing - as you know I am dead against having huge monoethnic commits and encourage more contributors to split their commits up into smaller functional changes/units

Respectfully though people do that in PRs all the time they're squashed before going upstream because they address a single issue. It may be useful for review just not useful upstream. There is no benefit to having --sasl-authzid separate from CURLOPT_SASL_AUTHZID separate from a working implementation in the library. That's 3 piecemeal commits that could be one with a commit message that outlines each of those things.

That said one of the other commits arguably does address a separate issue by not sending authcid as authzid. My advice is send that one commit upstream with refs to 3653 and here then rebase on that. (edit: Travis seems to be bogged down so the CI still isn't complete, so I can't tell if the modified tests passed)

...

...

Ref: https://github.com/curl/curl/issues/3653
Ref: https://github.com/curl/curl/pull/3790

captain-caveman2k · 2019-04-21T21:11:24Z

Can I suggest you use [WIP] (or similar) in the title then while the PR is not yet ready?

Added. I did look for a WiP label but couldn't see one - I even contemplated adding one but didn't in the end ;-)

bagder · 2019-04-21T21:18:30Z

I've pondered that too, but I've used [WIP] before myself. It's possibly slightly more visible.

github also allows the creation of a PR that is sort of wip, but for some reason our travis jobs don't run in those so they're not as useful right now.

Added the ability for the calling program to specify the authorisation identity (authzid), the identity to act as, in addition to the authentication identity (authcid) and password when using SASL PLAIN authentication. Fixed curl#3653 Closes curl#3790

- Change data and protocol sections to CRLF line endings. Prior to this change the tests would fail or hang, which is because certain sections such as protocol require CRLF line endings. Follow-up to a9499ff from today which added the tests. Ref: #3790

jay · 2019-05-23T08:04:52Z

Something happened between the final version here and when you sent it upstream because upstream had LFs instead of CRLFs in the tests in commit a9499ff. Fixed in c2a8d52. Also to reiterate ideally if the commits are split they should have reference points.

jay · 2019-05-26T05:12:47Z

Temporarily reverted in db8ec1f due to pending patch release.

Added the ability for the calling program to specify the authorisation identity (authzid), the identity to act as, in addition to the authentication identity (authcid) and password when using SASL PLAIN authentication. Fixed curl#3653 Closes curl#3790

- Change data and protocol sections to CRLF line endings. Prior to this change the tests would fail or hang, which is because certain sections such as protocol require CRLF line endings. Follow-up to a9499ff from today which added the tests. Ref: curl#3790

Added the ability for the calling program to specify the authorisation identity (authzid), the identity to act as, in addition to the authentication identity (authcid) and password when using SASL PLAIN authentication. Fixes curl#3653 Closes curl#3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #xxxx

Ref: curl#3653 Ref: curl#3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #xxxx

- Change data and protocol sections to CRLF line endings. Prior to this change the tests would fail or hang, which is because certain sections such as protocol require CRLF line endings. Follow-up to grandparent commit which added the tests. Ref: curl#3653 Ref: curl#3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #xxxx

Ref: curl#3653 Ref: curl#3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #xxxx

Added the ability for the calling program to specify the authorisation identity (authzid), the identity to act as, in addition to the authentication identity (authcid) and password when using SASL PLAIN authentication. Fixes curl#3653 Closes curl#3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #xxxx

Ref: curl#3653 Ref: curl#3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #xxxx

- Change data and protocol sections to CRLF line endings. Prior to this change the tests would fail or hang, which is because certain sections such as protocol require CRLF line endings. Follow-up to grandparent commit which added the tests. Ref: curl#3653 Ref: curl#3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #xxxx

Ref: curl#3653 Ref: curl#3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #xxxx

Added the ability for the calling program to specify the authorisation identity (authzid), the identity to act as, in addition to the authentication identity (authcid) and password when using SASL PLAIN authentication. Fixes #3653 Closes #3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #4186

Ref: #3653 Ref: #3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #4186

- Change data and protocol sections to CRLF line endings. Prior to this change the tests would fail or hang, which is because certain sections such as protocol require CRLF line endings. Follow-up to grandparent commit which added the tests. Ref: #3653 Ref: #3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #4186

Ref: #3653 Ref: #3790 NOTE: This commit was cherry-picked and is part of a series of commits that added the authzid feature for upcoming 7.66.0. The series was temporarily reverted in db8ec1f so that it would not ship in a 7.65.x patch release. Closes #4186

vszakats · 2019-08-07T15:15:50Z

Is it this intented to be merged again?

jay · 2019-08-07T16:51:44Z

Is it this intented to be merged again?

I moved it to #4186 for CI rather than take this one over. It landed yesterday.

vszakats · 2019-08-07T22:17:59Z

@jay: Great news — thank you!

captain-caveman2k mentioned this pull request Apr 17, 2019

Ability to specify SASL authorization identity #feature-request #3653

Closed

captain-caveman2k force-pushed the sasl-authzid branch 6 times, most recently from 7846957 to dd19a7b Compare April 19, 2019 16:14

bagder reviewed Apr 19, 2019

View reviewed changes

captain-caveman2k force-pushed the sasl-authzid branch 2 times, most recently from 8f0dd36 to 71870ce Compare April 20, 2019 18:47

bagder approved these changes Apr 20, 2019

View reviewed changes

captain-caveman2k force-pushed the sasl-authzid branch from 71870ce to 30fa720 Compare April 21, 2019 11:54

captain-caveman2k force-pushed the sasl-authzid branch 2 times, most recently from 27a1def to f986d23 Compare April 21, 2019 20:00

captain-caveman2k force-pushed the sasl-authzid branch from f986d23 to 73e4fa2 Compare April 21, 2019 21:03

captain-caveman2k changed the title ~~Implement SASL authorisation identity~~ Implement SASL authorisation identity [WIP] Apr 21, 2019

captain-caveman2k force-pushed the sasl-authzid branch from 73e4fa2 to b838f35 Compare April 22, 2019 09:02

captain-caveman2k added authentication IMAP POP3 SMTP labels Apr 22, 2019

captain-caveman2k force-pushed the sasl-authzid branch 2 times, most recently from d81aac5 to 3383ff3 Compare April 23, 2019 19:31

captain-caveman2k force-pushed the sasl-authzid branch from c43bfe1 to 4377882 Compare May 22, 2019 07:06

captain-caveman2k removed the feature-window A merge of this requires an open feature window label May 22, 2019

captain-caveman2k changed the title ~~[WIP] Implement SASL authorisation identity~~ Implement SASL authorisation identity May 22, 2019

captain-caveman2k closed this in a14d72c May 22, 2019

captain-caveman2k deleted the sasl-authzid branch May 22, 2019 22:00

jay mentioned this pull request Aug 2, 2019

Redux: Implement SASL authorisation identity #4186

Closed

lock bot locked as resolved and limited conversation to collaborators Nov 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SASL authorisation identity #3790

Implement SASL authorisation identity #3790

captain-caveman2k commented Apr 17, 2019 •

edited

bagder Apr 19, 2019

captain-caveman2k Apr 20, 2019

bagder Apr 20, 2019

bagder Apr 19, 2019

captain-caveman2k Apr 20, 2019

bagder Apr 20, 2019

bagder Apr 19, 2019

captain-caveman2k Apr 20, 2019

jay commented Apr 20, 2019

captain-caveman2k commented Apr 21, 2019

bagder commented Apr 21, 2019

jay commented Apr 21, 2019 •

edited

captain-caveman2k commented Apr 21, 2019

bagder commented Apr 21, 2019 •

edited

jay commented May 23, 2019

jay commented May 26, 2019

vszakats commented Aug 7, 2019

jay commented Aug 7, 2019

vszakats commented Aug 7, 2019

Implement SASL authorisation identity #3790

Implement SASL authorisation identity #3790

Conversation

captain-caveman2k commented Apr 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jay commented Apr 20, 2019

captain-caveman2k commented Apr 21, 2019

bagder commented Apr 21, 2019

jay commented Apr 21, 2019 • edited

captain-caveman2k commented Apr 21, 2019

bagder commented Apr 21, 2019 • edited

jay commented May 23, 2019

jay commented May 26, 2019

vszakats commented Aug 7, 2019

jay commented Aug 7, 2019

vszakats commented Aug 7, 2019

captain-caveman2k commented Apr 17, 2019 •

edited

jay commented Apr 21, 2019 •

edited

bagder commented Apr 21, 2019 •

edited