Hi Kyle,

On Fri, 23 Aug 2013, Kyle L. Huff wrote:

> In the attached 4 commits I have implemented OAUTH 2.0[1]
> Bearer Token[2] support to SMTP and IMAP.

Many thanks for your efforts here.

Do you know if OAUTH 2.0 is supported in POP3 as well?

> This implementation allows for authentication to an SMTP or
> IMAP server using only the username and an OAUTH 2.0[1]
> Bearer token[2].

Here is the feedback from my initial review (It appears to be long but most
suggestions are fairly minor and revolve around the order of definitions and
functions and the indentation of parameters):

* There is extra whitespace in line 1557 of curl.h
* There is a small typo in curl_sasl.h "he user name" should be "the user
name"
* The parameters of the parameters in lines 56, 57, 68 of curl_sasl.h should
be indented by two more characters to align with the first parameter on line
55
* Could you add a reference to RFC6749 to the top of cur_sasl.c?
* Could you align the [in] / [out] parameter specifiers in the sasl function
comments to be like the other functions - I would suggest aligning with
"user" and "bearer"
* Following the recent changes to use heap based buffers would you be so
kind to allocate your xoauth buffer dynamically and specify the correct size
in the malloc() - alternatively you might want to consider using aprintf()
insterad, rather than using memcpy() type functions - this will also reduce
the need for the separate allocation?
* Could you move the definition of SMTP_AUTH_XOAUTH2 to between the
SMTP_NTLM_TYPE2MSG and SMTP_AUTH_FINAL definitions as I have tried to base
this list loosely on the sasl mechanism definitions - although I appreciate
that LOGIN and PLAIN are the wrong way round at present?
* Could you place the RFC reference in smtp.c in the correct numeric order -
up one line to before the Draft reference?
* Could you move the check for XOAUTH in smtp_endofresp() to after NTLM to
match the order in which the mechanism constants are defined?
* The order of the SMTP states in state() doesn't match the order in the
header file and will give incorrect output in debug builds
* It looks from the code in smtp_perform_authenticate() (as well as that in
the IMAP code) that "XOAUTH" is supported in the AUTH initial response
command - If this is the case then a) state2 should be set to
SMTP_AUTH_FINAL to allow for checking of the response code afterwards and b)
the 3rd and 4th parameters should be indented to match the 1st. If not then
there is no need to perform the call to Curl_sasl_create_xoauth2_message()
there.
* Could you move smtp_state_auth_xoauth2_resp() to after the ntlm
functions?
* The indentation of the 2nd and 3rd parameters should match the 1st
* Could you move the case for SMTP_AUTH_XOAUTH2 to after
SMTP_AUTH_NTLM_TYPE2MSG in smtp_statemach_act()
* I'm not sure the check for conn->xoauth2_bearer is best placed in
smtp_statemach_act() as it a) doesn't allow for returning an error code and
b) isn't checked when the initial response is used - suggest moving it into
smtp_state_auth_xoauth2_resp()
* The preferred mechanism check in smtp_parse_url_options() should match the
order of the mechanism definitions
* Could you move the definition of IMAP_AUTH_XOAUTH2 to between the
IMAP_NTLM_TYPE2MSG and IMAP_AUTH_FINAL definitions as a) the same reason I
gave above for the SMTP definitions and 2) it currently doesn't match the
SMTP definitions which I have tried to keep in sync where possible?
* The indentation of the parameters to Curl_sasl_create_xoauth2_message() in
imap_perform_authenticate() is not correct
* Could you move the check for XOAUTH in imap_state_capability_resp () to
after NTLM to match the order in which the mechanism constants are defined?
* Could you move imap_state_auth_xoauth2_resp() to after the ntlm
functions?
* The indentation of the 2nd and 3rd parameter to
imap_state_auth_xoauth2_resp() is not correct
* Could you move the case for IMAP_AUTHENTICATE_XOAUTH2 to after
IMAP_AUTHENTICATE_NTLM_TYPE2MSG in imap_statemach_act()
* Suggest moving the check for conn->xoauth2_bearer to
imap_state_auth_xoauth2_resp() as per smtp suggestion
* The preferred mechanism check in imap_parse_url_options() should match the
order of the mechanism definitions
* Suggest aligning the comment to xoauth2_bearer variable in tool_cfgable.h
to match the "use_metalink" variable
* In tool_getparam.c you have added a check for config->xoauth2_bearer in
case 'u' (line 1639) but the check for err is still outside that if
statement
* Suggest adding "(IMAP and SMTP)" to the line you added to tool_help.c to
indicate to the user that it is only supported for these protocols

The most important items in this list were the lack of setting state2 in
smtp_perform_authenticate() and the use of a stack based buffer in the sasl
code.

Additionally, could you please split patch 1 into two separate patches: one
for adding bearer token support and the second for adding the support to
SMTP as that will help keep things separate when I come to commit your
changes which I will do one at a time allowing the auto builds to pick up
anything I may have missed.

Many thanks

Steve
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2013-08-24