>Hi Yves,
>
>Thanks for the patch and your efforts!
>
>Do you have a rebased version of this patch for me to try?

I am not sureŠ I tried and got conflicts, then was told by git my branch
was out of sync, then redid a "git pull --rebase upstream" and got a bunch
of conflicts againŠ Solved them, committed but only saw two changed files.
But I see some "15 minutes ago" changes on GitHub from others than me.
So...

I'm new at git, so I may need some guidance here. Sorry :( Check out
what's on https://github.com/ExpediaInc/curl.

>Also, I would like
>to see some test cases/examples of how this is used. That will help me do
>a
>full review. Still, I've read the patch and here are my comments for now:

What's a good format for the test cases/examples. Would something showing
typical use help, or do you mean automated tests? That'd require adding
OAuth 2.0 to the sample server too.

An example of use is:

# Make an unauthenticated request to our server, and see how we get a 401
with a request to authenticate with HTTP MAC
% curl -LsS -H 'Accept: application/json; charset=utf-8'
http://hostname/service/v1/resources -D -
HTTP/1.1 401 Unauthorized
Server: Apache-Coyote/1.1
WWW-Authenticate: MAC error="Authorization header is missing"
Content-Type: text/html;charset=utf-8
Content-Length: 1099
Date: Fri, 08 Feb 2013 01:07:44 GMT

[Š]
# Get a token from our oauth2server

% curl -LsS -H 'Accept: application/json; charset=utf-8'
http://127.0.0.1/oauth2server/tokens -d /dev/null -D -
HTTP/1.0 200 OK
Date: Fri, 08 Feb 2013 01:05:29 GMT
Server: WSGIServer/0.1 Python/2.7.2
Content-Length: 214
Content-Type: application/json

{"token_type": "mac", "mac_key":
"LBy8nZzvidRT8N9lTQmfE4t6ONd4AAsS1GZNkyh0nzMqasXDjt0jJHjOPyBOpEQL",
"mac_algorithm": "hmac-sha-256", "access_token": "qrw6241j8U",
"expires_in": 3600, "refresh_token": "2TEKVY9jrL"}
# Let's store a token (we're getting a new one here, of course, since
we're POSTing again)
% curl -Lss -H 'Accept: application/json; charset=utf-8'
http://127.0.0.1/oauth2server/tokens -d /dev/null -D - >token.json
# Make an authenticated request, and just show the generated Authorization
header for illustration purposes:
% curl -LsS -H 'Accept: application/json; charset=utf-8'
http://hostname/service/v1/resources -D - \

    --oauth2 token.json -v 2>&1 sed -n 's/.*\(Authorization: .*\)/\1/p'
Authorization: MAC id="qrw6241j8U", ts="1360285918", nonce="029j47951",
mac="x+WII3djVZqCRsEn1OP0jmFuGFWR7N6mJSvmU4/PUak="
# Make the same request this time showing the headers and beginning of the
response again

% curl -LsS -H 'Accept: application/json; charset=utf-8'
http://hostname/service/v1/resources -D - --oauth2 token.json
HTTP/1.1 200 OK
Server: Apache-Coyote/1.1
Content-Type: application/json
Date: Fri, 08 Feb 2013 01:13:11 GMT

{
    [Š]
}
%

>My first reaction to for example the Curl_output_mac() function is that
>it is
>a lot of magic functionality with very little comments.

I'll add comments. It's a straightforward implementation of the internet
draft so I guess if you read the draft along it's pretty obvious. I'll
make it stand on its own though.

>
>Curl_output_mac() should also not have to figure out the default port
>numbers
>based on the conn->handler pointer, it can just use conn->remote_port.

Sweet. I changed that. Thanks!

>When you #define macros, I think you should define them outside of the
>funtions so that the definition is clearly not part of the function. I
>can't
>see why you define and use CURL_OUTPUT_BEARER_CONV as its only used once.

Honestly? That's because it's copied straight from http_digest.c. I
honestly don't understand that back-to-ASCII conversion there and just got
it from the file I inspired myself from (including the #define inside the
function, which I thought was a curl idiom). Maybe it's not even needed?

If there are ASCII/non-ASCII issues to be aware of, how is it dealt with?
I need to add some code to validate that some values only include
characters in these ranges: %x20-21 / %x23-5B / %x5D-7E (any printable
ASCII character except for <"> and <\>) and haven't done that yet.

>There's a bunch of case sensitive string comparisons. That seems a little
>unorthodox in protocol land. Are you sure they're not case insensitive in
>the
>protocol?
>
>Why the new curlx_tvgettimeofday() implementation? curlx_tvnow() is there
>already, and if it isn't good enough I figure that's what should fixed.
>Or am
>I missing something?

curlx_tvnow() is not good enough because on Windows for example its
beginning time is relative to a system event (last reboot for example).
This works great for what curl does with it: computing durations by
subtracting the values returned by two calls. However, HTTP MAC requires a
monotically increasing function with a fixed epoch for EACH client (I
quote: "The value MUST be a positive integer set by the client when making
each request to the number of seconds elapsed from a fixed point in
time.")

Again, not knowing curl well, I didn't want at that point to mess up with
curlx_tvnow(), especially if that had consequences on platforms I do not
use.

If a fixed Epoch is okay for curlx_tvnow() on Windows, then yes you could
make the curlx_tvgettimeofday() code the implementation for Windows in
order to simplify.

Speaking of timeŠ In my patch, the code I wrote was made to "break" a
non-strictly compliant HTTP MAC implementation by default (like the one my
team initially deliveredŠ). I have changed this so that by default we use
the Unix Epoch as a reference so that a slightly non-compliant HTTP MAC
validation (one that only works with that Epoch) still works.

>You're suggesting three new functions to the libcurl API but there's no
>docs
>for them. I'm not at all happy with new functions dedicated solely for a
>particular auth method for a particular protocol...

I would of course add docs. You are talking about the functions in
oauth2.h here I am assuming. I am fine not exposing them in the library.
The parsing and freeing could stay in the tool directory. This being said,
they are quite convenient for someone who is just getting started, as they
do a lot of work (parsing tokens in both JSON and
application/x-www-url-encoded formats) that is required once someone gets
a token. Parsing them may not be obvious for the casual user of the
libcurl library. BUT I am certainly fine either wayŠ It could be moved
into the tool (src) directory until someone screams for a builtin parser...

If however the convenience functions make sense, we could only have two by
removing the curl_parse_oauth2_token_file function. It's basically a file
to memory globber. I don't even use it myself I believe (the tool use its
builtin reader to set the token in config, and it's parsed later on from
that string in tool_operate.c).

>Several of the functions are define with the starting brace on the right
>side
>of the function intead of in column 0 on the first line of the function.

Sorry. I thought all such issues had been caught by the build. I'll look
for those.

>Your patch modifies unrelated code in getparameter() where you've changed
>a
>bunch of calls to str2unum() etc. Those changes may be fine, but how are
>they
>oauth2 related?

getparameter() and str2unumŠ Never heard of that. Maybe my patch caught
some other changes by accident as I was rebasing/trying to keep up with
the base. *OR* (I just looked) maybe I did a rename of "err" to "rc" in a
(Emacs) region with a heavy hand because all those changes as I can see
them are about renaming an "err" variable to "rc." If so, I apologize.

YA

>--
>
> / daniel.haxx.se
>-------------------------------------------------------------------
>List admin: http://cool.haxx.se/list/listinfo/curl-library
>Etiquette: http://curl.haxx.se/mail/etiquette.html

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