Paths

Table of Contentst

-
lib/libfetch/
-
libfetch/
-
http.c

libfetch: Fix proxy authentication
ClosedPublic
Actions

Authored by kp on Apr 1 2021, 11:59 AM.

Details

Reviewers

kevans
bapt
garga

Commits

rG208c36a005f5: libfetch: Retry with proxy auth when server returns 407
rGd7682961d386: libfetch: Retry with proxy auth when server returns 407
rG345c30a94f64: libfetch: Retry with proxy auth when server returns 407

Summary

This patch is being used on pfSense to fix pkg when proxy is configured and many users reported it works as expected.

libfetch: Retry with proxy authentication when server returns 407

PR:	220468
Submitted by:	Egil Hasting <egil.hasting@higen.org> (based on)
Reviewed by:	kevans, kp
Approved by:	kp	
Sponsored by:	Rubicon Communications, LLC ("Netgate")
Differential Revision:	https://reviews.freebsd.org/D29533

Test Plan

Configure an authenticated proxy server and define HTTP_PROXY and HTTP_PROXY_AUTH environment variables. Without this patch the result is like:

garga@x230 ~ ❯❯❯ env | grep PROXY
HTTP_PROXY_AUTH=basic:*:testproxy:testproxy
HTTP_PROXY=172.21.4.1:3128
garga@x230 ~ ❯❯❯ fetch -v http://ifconfig.me
resolving server address: 172.21.4.1:3128
requesting http://ifconfig.me/
proxy requires authorization
resolving server address: 172.21.4.1:3128
requesting http://ifconfig.me/
302 redirect to https://ifconfig.me/
resolving server address: 172.21.4.1:3128
fetch: http://ifconfig.me: Proxy Authentication Required

After apply the patch:

garga@x230 ~ ❯❯❯ fetch -v http://ifconfig.me
resolving server address: 172.21.4.1:3128
requesting http://ifconfig.me/
proxy requires authorization
resolving server address: 172.21.4.1:3128
requesting http://ifconfig.me/
302 redirect to https://ifconfig.me/
resolving server address: 172.21.4.1:3128
resolving server address: 172.21.4.1:3128
SSL options: 82004854
Peer verification enabled
Using CA cert file: /usr/local/etc/ssl/cert.pem
Verify hostname
TLSv1.3 connection established using TLS_AES_256_GCM_SHA384
Certificate subject: /CN=ifconfig.me
Certificate issuer: /C=US/O=Google Trust Services/CN=GTS CA 1D2
requesting https://ifconfig.me/
remote size / mtime: 8826 / 0
ifconfig.m                                    8826  B   38 MBps    00s

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

garga requested review of this revision.Apr 1 2021, 11:59 AM

garga created this revision.

kp mentioned this in D29532: libfetch: handle 407 (proxy auth) when connecting to HTTPS using connect tunnel.Apr 1 2021, 12:01 PM

kp added inline comments.Apr 1 2021, 12:09 PM

lib/libfetch/http.c
1405	I'm not sure about using warnx() from a library. There are others, but they're for unimplemented functionality (so should arguably be assertions for the calling app), not for runtime errors (i.e. proxy auth information not set).
1453	I don't like '> 0' for an int that's used as a boolean, but that's a question of taste more than anything.
1509	This is wrong though. 'fetch_close(conn)' frees conn, so we can't use or return it. I think we should just not fetch_close().

re: commit message:

Submitted by: Egil Hasting <egil.hasting@higen.org>

Drop this line and --amend the commit with --author="Egil Hasting <egil.hasting@higen.org>", git log should show their name and you will be attributed as the committer.

lib/libfetch/http.c
1408	Just before this, we should explicitly check if `aparams.user == NULL \|\| aparams.password == NULL` and return an error (clean_http_auth_parms first) to propagate the ENOMEM condition properly.
1509	Yes, the caller (below) will close it in ouch if it's not NULL, so just drop this fetch_close() entirely since we're returning a live object.

kevans added inline comments.Apr 1 2021, 12:44 PM

lib/libfetch/http.c
1509	Actually, that's a lie. The caller below needs to observe `conn->err` then close it.

Address a few comments (lightly edited version of Renato's work).

kp added inline comments.Apr 1 2021, 7:25 PM

lib/libfetch/http.c
1408	The code is already somewhat inconsistent about checking for memory allocation failures. On the whole I tend to prefer not to clutter userspace code with such checks, because in the vast majority of cases the system will be overcommitting memory anyway, so the first thing you'll know about an out-of-memory condition is when the OOM killer comes round and terminates you.

Sure

This revision is now accepted and ready to land.Apr 1 2021, 7:35 PM

garga accepted this revision.Apr 1 2021, 8:53 PM

garga edited the summary of this revision. (Show Details)Apr 1 2021, 8:57 PM

garga edited the summary of this revision. (Show Details)Apr 1 2021, 9:00 PM

garga edited the summary of this revision. (Show Details)

Closed by commit rG345c30a94f64: libfetch: Retry with proxy auth when server returns 407 (authored by garga). · Explain WhyApr 1 2021, 9:07 PM

This revision was automatically updated to reflect the committed changes.

garga added a commit: rG345c30a94f64: libfetch: Retry with proxy auth when server returns 407.

kib mentioned this in D29832: ntpd: Change default stack limit to 4096 pages..Apr 26 2021, 11:21 AM

garga added a commit: rGd7682961d386: libfetch: Retry with proxy auth when server returns 407.Apr 28 2021, 7:38 PM

garga added a commit: rG208c36a005f5: libfetch: Retry with proxy auth when server returns 407.

Revision Contents
Changeset List

Path

Size

lib/

libfetch/

http.c

55 lines

Diff 86704

View Options

lib/libfetch/http.c

This file was added.

				/*-
				* SPDX-License-Identifier: BSD-2-Clause
				*
				* Copyright (c) 2023 Dave Cottlehuber <dch@FreeBSD.org>
				*
				* Redistribution and use in source and binary forms, with or without
				* modification, are permitted provided that the following conditions
				* are met:
				* 1. Redistributions of source code must retain the above copyright
				* notice, this list of conditions and the following disclaimer.
				* 2. Redistributions in binary form must reproduce the above copyright
				* notice, this list of conditions and the following disclaimer in the
				* documentation and/or other materials provided with the distribution.
				*
				* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
				* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
				* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
				* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
				* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
				* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
				* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
				* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
				* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
				* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
				* SUCH DAMAGE.
				*
				*/

				#include <sys/cdefs.h>
				__FBSDID("$FreeBSD$");

				#include <errno.h>
				#include <sys/param.h>
				#include <stdio.h>
				#include <string.h>
				#include <fetch.h>

				#include <lua.h>
				#include "lauxlib.h"
				#include "lfetch.h"

				/*
				* Minimal implementation of libfetch
				*/

				static int
				lfetch_parse_url(lua_State *L)
				{
				const char *url = luaL_checkstring(L, 1);
				struct url *u;

				u = fetchParseURL(url);
				if (u == NULL) {
				lua_pushnil(L);
				lua_pushstring(L, "Failed to parse URL");
				return (2);
				}

				lua_newtable(L);
				lua_pushstring(L, u->scheme);
				lua_setfield(L, -2, "scheme");
				lua_pushstring(L, u->user);
				lua_setfield(L, -2, "user");
				lua_pushstring(L, u->pwd);
				lua_setfield(L, -2, "password");
				lua_pushstring(L, u->host);
				lua_setfield(L, -2, "host");
				/* if port is not explicitly set, it defaults to 0, not scheme */
				kevansUnsubmitted Done Inline Actions My gut reaction to this is that we probably shouldn't expose the port at all if it's not explicitly set, just leave it nil in this table to allow for more idiomatic usage where you can simply test the port as truthy before using it.f kevans: My gut reaction to this is that we probably shouldn't expose the port at all if it's not…
				lua_pushinteger(L, u->port);
				lua_setfield(L, -2, "port");
				/* doc is also known as the query string for http(s) */
				lua_pushstring(L, u->doc);
				lua_setfield(L, -2, "doc");
				fetchFreeURL(u);
				return (1);
				}

				static int
				lfetch_get_url(lua_State *L) {
				kevansUnsubmitted Done Inline Actions Brace on its own line kevans: Brace on its own line
				const char *url = luaL_checkstring(L, 1);
				const char *out = luaL_checkstring(L, 2);

				struct url *u;
				kevansUnsubmitted Done Inline Actions This should move up above the blank line kevans: This should move up above the blank line
				u = fetchParseURL(url);
				if (u == NULL) {
				lua_pushnil(L);
				lua_pushstring(L, "Failed to parse URL");
				return (2);
				}

				FILE *file = fopen(out, "wb");
				kevansUnsubmitted Done Inline Actions IIRC style(9) still doesn't allow declarations mid-block, it would need to be at the top of this function. kevans: IIRC style(9) still doesn't allow declarations mid-block, it would need to be at the top of…
				if (file == NULL) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to open output file: %s", strerror(errno));
				return 2;
				}

				FILE *fetch = fetchGet(u, "");
				if (fetch == NULL) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to read from URL: %s", strerror(errno));
				return 2;
				kevansUnsubmitted Done Inline Actions Leaks `file` kevans: Leaks `file`
				}

				int chunk_size = 4096;
				kevansUnsubmitted Done Inline Actions Ditto for these declarations; chunk_size I'd probably just make a `#define` since we're not going to be altering it kevans: Ditto for these declarations; chunk_size I'd probably just make a `#define` since we're not…
				char buf[chunk_size];
				size_t bytes;

				while ((bytes = fread(buf, 1, chunk_size, fetch)) > 0) {
				if (fwrite(buf, 1, bytes, file) != bytes) {
				lua_pushnil(L);
				lua_pushfstring(L, "Failed to write to file: %s", strerror(errno));
				fclose(file);
				kevansUnsubmitted Done Inline Actions Leaks `fetch` kevans: Leaks `fetch`
				dchAuthorUnsubmitted Done Inline Actions If I `fclose(fetch);` here, this segfaults: Lua 5.4.4 Copyright (C) 1994-2022 Lua.org, PUC-Rio f.get_url("https://invalid.site/", "/tmp/i") fish: Job 1, '/usr/libexec/flua $argv' terminated by signal SIGSEGV (Address boundary error) (lldb) thr backtrace all * thread #1, name = 'flua', stop reason = signal SIGSEGV * frame #0: 0x00001252697ad0f9 frame #1: 0xffadb0abacff9a89 dch: If I `fclose(fetch);` here, this segfaults: Lua 5.4.4 Copyright (C) 1994-2022 Lua.org, PUC…
				kevansUnsubmitted Done Inline Actions The context for this one has changed, the comment was originally down in the error path in the loop. This branch looks fine kevans: The context for this one has changed, the comment was originally down in the error path in the…
				return 2;
				}
				}

				fclose(fetch);
				fclose(file);
				fetchFreeURL(u);
				lua_pushboolean(L, 1);
				return 1;
				}

				static const struct
				luaL_Reg fetchlib[] = {
				{"parse_url", lfetch_parse_url},
				{"get_url", lfetch_get_url},
				{NULL, NULL}
				};

				int
				luaopen_fetch(lua_State *L)
				{
				luaL_newlib(L, fetchlib);
				return (1);
				}

libfetch: Fix proxy authenticationClosedPublicActions