OK, here it is. Please comment. Some observations: When the cache parses the HTTP header we just attach any authentication headers to the cache entry. Nothing else happens until we process the clients. Just before iterating over the clients we check if all the data is in. If it is then we do any necessary authentication. Opening the authentication dialog runs the FLTK event loop, which means that incoming data can call the cache again, triggering CCC warnings. We work around it by deferring the dialog to a timeout so that the cache finishes iterating over the clients before the dialog appears. This is annoying but I don't see a better way at the moment. The authentication dialog does not know which browser window opened the URL so it can't call a_Nav_reload(). Instead I implemented a_Nav_reload_url(). There's an interesting case when the server demands authentication for a realm that we have already authenticated ourselves to. It could be that we didn't send authentication because we didn't know the path was in the realm, or it could be that the authentication we sent was wrong. This requires the code to deduce what it did last time it saw this URL and rely on what it will do the next time. It works, but it feels fragile to me. I would prefer the URL to explicitly record authentication attempts so that the code doesn't have to second-guess itself. Thoughts? I implemented a full token-value list parser as per the RFC. Maybe this is overkill for Basic authentication, but it will be necessary for Digest authentication. Yes, it has gotos; I began by setting flags but decided that gotos were actually cleaner. Maybe it should be rewritten in state-machine style? If the user opens several pages in a realm in a very short time then it is conceivable that multiple authentication dialogs for the same realm could pop up (just like the https DPI sometimes does with certificate dialogs). I don't address this at the moment because I think it won't be a problem in practice. If it needs fixing then we will have to monitor the list of currently open dialogs to avoid duplicates. Regards, Jeremy Henty
On Thu, Dec 04, 2008 at 11:11:03AM +0000, Jeremy Henty wrote:
Opening the authentication dialog runs the FLTK event loop, which means that incoming data can call the cache again, triggering CCC warnings. We work around it by deferring the dialog to a timeout so that the cache finishes iterating over the clients before the dialog appears. This is annoying but I don't see a better way at the moment.
Have you thought about how other protocols can be integrated with this? Examples are: - TLS with client side authentication or verification for the server certificate - non-anonymous FTP Joerg
On Thu, Dec 04, 2008 at 01:36:39PM +0100, Joerg Sonnenberger wrote:
Have you thought about how other protocols can be integrated with this?
No, except that I've deliberately made the User/Password dialog generic so that any other code can reuse it with a different callback. Other than that I think that all of my code is entirely specific to HTTP Basic authentication. I can't see how other protocols could be integrated with it in any useful sense. But my opinion doesn't count for much since I know next to nothing about how those other protocols work!
Examples are: - TLS with client side authentication or verification for the server certificate
I have no idea how this works. Every time I try to study certificates and authentication my head starts spinning and I have to lie down. Does anyone know of some gentle introductions?
- non-anonymous FTP
The FTP DPI could check the output of wget. If wget complained that the FTP server needed authentication than it could pop up the user/password dialog with a callback that retries the wget. The download DPI would have to be modified to receive the user and password. (Or we could stuff them into the download URL, but that's insecure since the password would appear in the process listing.) We would have to extend the DPI API to let DPIs use the user/password dialog but that is straightforward (I already implemented it when I thought authentication was going to be a DPI). Regards, Jeremy Henty
Jeremy wrote:
...
I haven't looked into how anything fits together yet at all, but skimming through:
@@ -234,12 +236,13 @@ Dstr *a_Http_make_query_str(const DilloUrl *url, bool_t use_proxy) "User-Agent: Dillo/%s\r\n" "Content-Length: %ld\r\n" "Content-Type: %s\r\n" - "%s" + "%s" /* cookies */ + "%s" /* auth */ "\r\n", full_path->str, URL_AUTHORITY(url), proxy_auth->str, referer, VERSION, URL_DATA(url)->len, content_type->str, - cookies); + cookies, auth ? auth : ""); dStr_append_l(query, URL_DATA(url)->str, URL_DATA(url)->len); dStr_free(content_type, TRUE); } else {
Remember that there's a header ordering convention.
+/* + * Return the host that contains a URL, or NULL if there is no such host. + */ +static AuthHost *Host_from_url(const DilloUrl *url) +{ + AuthHost *host; + int i; + + for (i = 0; (host = dList_nth_data(auth_hosts, i)); i++) + if (((strcmp(URL_SCHEME(url), host->scheme) == 0) && + (strcmp(URL_AUTHORITY(url), host->authority) == 0))) + return host; + + return NULL; +}
dStrcasecmp
+static void Realm_add_path(AuthRealm *realm, const char *path) +{ + int len, i; + char *realm_path; + + len = strlen(path); + /* ignore a trailing '/' */ + if (len && path[len - 1] == '/') + len--; + + /* delete existing paths that are inside the new one */ + for (i = 0; (realm_path = dList_nth_data(realm->paths, i)); i++) { + if (path_is_inside(realm_path, path, len)) { + dList_remove_fast(realm->paths, realm_path); + i--; /* reconsider this slot */ + } + } + + dList_append(realm->paths, dStrndup(path, len)); +}
I think you need a dFree in there. PS I took the liberty of adding an entry for Basic auth to Plans.html
On Thu, Dec 04, 2008 at 05:59:21PM +0000, corvid wrote:
Remember that there's a header ordering convention.
D'oh, I saved the link to the discussion of this and then forget to review it! Will fix.
+/* + * Return the host that contains a URL, or NULL if there is no such host. + */ +static AuthHost *Host_from_url(const DilloUrl *url) +{ + AuthHost *host; + int i; + + for (i = 0; (host = dList_nth_data(auth_hosts, i)); i++) + if (((strcmp(URL_SCHEME(url), host->scheme) == 0) && + (strcmp(URL_AUTHORITY(url), host->authority) == 0))) + return host; + + return NULL; +}
dStrcasecmp
When comparing the scheme, the authority, or both?
+static void Realm_add_path(AuthRealm *realm, const char *path) +{ + int len, i; + char *realm_path; + + len = strlen(path); + /* ignore a trailing '/' */ + if (len && path[len - 1] == '/') + len--; + + /* delete existing paths that are inside the new one */ + for (i = 0; (realm_path = dList_nth_data(realm->paths, i)); i++) { + if (path_is_inside(realm_path, path, len)) { + dList_remove_fast(realm->paths, realm_path); + i--; /* reconsider this slot */ + } + } + + dList_append(realm->paths, dStrndup(path, len)); +}
I think you need a dFree in there.
For the realm_path that gets passed to dList_remove_fast()? Will fix.
PS I took the liberty of adding an entry for Basic auth to Plans.html
No problem. Thanks for looking at this. Jeremy Henty
Jeremy wrote:
On Thu, Dec 04, 2008 at 05:59:21PM +0000, corvid wrote:
+/* + * Return the host that contains a URL, or NULL if there is no such host. + */ +static AuthHost *Host_from_url(const DilloUrl *url) +{ + AuthHost *host; + int i; + + for (i = 0; (host = dList_nth_data(auth_hosts, i)); i++) + if (((strcmp(URL_SCHEME(url), host->scheme) == 0) && + (strcmp(URL_AUTHORITY(url), host->authority) == 0))) + return host; + + return NULL; +}
dStrcasecmp
When comparing the scheme, the authority, or both?
I think both. *pokes through rfc2396 for a bit* "When a scheme uses elements of the common syntax, it will also use the common syntax equivalence rules, namely that the scheme and hostname are case insensitive..."
Hi Jeremy, Committed a modified version of the last patch. Here I'm adding some comments. On Thu, Dec 04, 2008 at 11:11:03AM +0000, Jeremy Henty wrote:
OK, here it is. Please comment. Some observations:
When the cache parses the HTTP header we just attach any authentication headers to the cache entry. Nothing else happens until we process the clients. Just before iterating over the clients we check if all the data is in. If it is then we do any necessary authentication.
Just moved the test after the for loop.
Opening the authentication dialog runs the FLTK event loop, which means that incoming data can call the cache again, triggering CCC warnings. We work around it by deferring the dialog to a timeout so that the cache finishes iterating over the clients before the dialog appears. This is annoying but I don't see a better way at the moment.
That's the way to do it.
The authentication dialog does not know which browser window opened the URL so it can't call a_Nav_reload(). Instead I implemented a_Nav_reload_url().
Removed a_Nav_reload_url() and set bw as a parameter.
There's an interesting case when the server demands authentication for a realm that we have already authenticated ourselves to. It could be that we didn't send authentication because we didn't know the path was in the realm, or it could be that the authentication we sent was wrong. This requires the code to deduce what it did last time it saw this URL and rely on what it will do the next time. It works, but it feels fragile to me. I would prefer the URL to explicitly record authentication attempts so that the code doesn't have to second-guess itself. Thoughts?
This will need debugging. Doesn't seem critical though.
I implemented a full token-value list parser as per the RFC. Maybe this is overkill for Basic authentication, but it will be necessary for Digest authentication. Yes, it has gotos; I began by setting flags but decided that gotos were actually cleaner. Maybe it should be rewritten in state-machine style?
I'd appreciate it in smaller functions and without gotos.
If the user opens several pages in a realm in a very short time then it is conceivable that multiple authentication dialogs for the same realm could pop up (just like the https DPI sometimes does with certificate dialogs). I don't address this at the moment because I think it won't be a problem in practice. If it needs fixing then we will have to monitor the list of currently open dialogs to avoid duplicates.
Just added a check. It doesn't solve the problem, but avoids the storm and sends a warning to stdout. Please review the changes. It almost has no testing. Some changes I remember: - Modified a bit the code ignoring the last '/' of a path. - Reordered function so no massive forward declarations are necessary. - Used VOID2INT macro. - Added some comments (there're still plenty of uncommented functions) - Prefixed all functions in auth.c with "Auth" - Suffixed "_t" the typedefed data structures. - Removed a_Bw_each() - Removed compiler warnings. - some others... * Does Auth_path_is_inside() need to be case sensitive? Well, now it seems is time to fix the missing points and to real-world test it! -- Cheers Jorge.-
I reviewed the final auth patch. No changes to suggest, just a few comments.
The authentication dialog does not know which browser window opened the URL so it can't call a_Nav_reload(). Instead I implemented a_Nav_reload_url().
Removed a_Nav_reload_url() and set bw as a parameter.
<facepalm/> Why didn't I think of that? :-)
There's an interesting case when the server demands authentication for a realm that we have already authenticated ourselves to. It could be that we didn't send authentication because we didn't know the path was in the realm, or it could be that the authentication we sent was wrong. This requires the code to deduce what it did last time it saw this URL and rely on what it will do the next time. It works, but it feels fragile to me. I would prefer the URL to explicitly record authentication attempts so that the code doesn't have to second-guess itself. Thoughts?
This will need debugging. Doesn't seem critical though.
I've been testing it locally and it seems to work as expected.
I implemented a full token-value list parser as per the RFC.
I'd appreciate it in smaller functions and without gotos.
OK, I'll investigate breaking it down.
If the user opens several pages in a realm in a very short time then it is conceivable that multiple authentication dialogs for the same realm could pop up
Just added a check. It doesn't solve the problem, but avoids the storm and sends a warning to stdout.
What happens if the user opens pages for different realms at almost the same time. Will the first authentication dialog suppress the other? If so then the user will have to reload the second page before they can log in.
* Does Auth_path_is_inside() need to be case sensitive?
I think so. From RFC 2616 Hypertext Transfer Protocol -- HTTP/1.1: 3.2.3 URI Comparison When comparing two URIs to decide if they match or not, a client SHOULD use a case-sensitive octet-by-octet comparison of the entire URIs, with these exceptions: None of the exceptions apply to the path component (except for the condition that "" and "/" are the same path). Since "/foo" and "/FOO" are different paths I deduce that "/foo/bar" is not inside "/FOO". Regards, Jeremy Henty
participants (4)
-
corvid＠lavabit.com -
jcid＠dillo.org -
joerg.sonnenberger＠web.de -
onepoint＠starurchin.org