netrc: Implement .netrc parsing #244

davidpolverari · 2019-10-18T02:02:53Z

This implementation parses the $HOME/.netrc (or a file specified by the
user) through the --netrc[=file] or -R[file] options.

It uses a simple FSM in order to parse the file and find the credentials
corresponding to the FTP host, or uses the "default" entry otherwise,
if any, according to the specification found on [1]. Tokens not
applicable to axel usage were not considered.

[1] https://docs.oracle.com/cd/E19455-01/806-0633/6j9vn6q5f/index.html

Closes: #116

Signed-off-by: David Polverari david.polverari@gmail.com

This implementation parses the $HOME/.netrc (or a file specified by the user) through the `--netrc[=file]` or `-R[file]` options. It uses a simple FSM in order to parse the file and find the credentials corresponding to the FTP host, or uses the "default" entry otherwise, if any, according to the specification found on [1]. Tokens not applicable to axel usage were not considered. [1] https://docs.oracle.com/cd/E19455-01/806-0633/6j9vn6q5f/index.html Closes: axel-download-accelerator#116 Signed-off-by: David Polverari <david.polverari@gmail.com>

davidpolverari · 2019-10-18T02:17:57Z

PS: On this implementation, I had to use the // fall through comment in order to placate GCC's -Wimplicit-fallthrough, as the project Makefile WARN_CFLAGS are considering a lot of warnings as errors and in this case it was the desired outcome.

As GCC considers this specific comment as an "explicit fallthrough", the error doesn't show, but I don't know whether other compilers (like clang) will present the same behavior.

ismaell

whatever the parser needs should be into it's own struct, and it should get a pointer to it.
allocate buffers with malloc/calloc to fail gracefully when OOM
the state machine could be encoded as functions using tail merging
don't use variables to stop a loop
the parser shouldn't know anything about axel_t, outputs should be explicit too, pass pointers and lengths, so use strlcpy
have you thought about using mmap? that could simplify the design.

src/netrc.c

ismaell · 2019-10-18T11:04:14Z

src/text.c

@@ -161,6 +162,15 @@ main(int argc, char *argv[])
 				}
 			}
 			break;
+		case 'R':


Let's define a negative version, it's necessary for if there's a default in the axel config file. I'm not sure a short version is needed (I've been thinking about removing support for systems not supporting getopt_long anyway).

About negative version of the option, I don't know whether it is a good idea to make .netrc parsing default, as it may break existing users' assumptions/scripts/workflows, etc. What do you think about it?

I don't mean to make it the default, but if you have the option on the configuration file, then you need a way to disable it from the command line.

Ok, I'll add it.

src/conf.h

src/text.c

davidpolverari · 2019-10-18T13:30:42Z

Thanks for the review! I will try to address the points you made as soon as I can.

davidpolverari · 2019-10-19T03:51:37Z

* have you thought about using `mmap`? that could simplify the design.

About this point, I was thinking about using mmap, but using it precludes the use of strtok*, because mmap won't null-terminate the mmaped file and strtok{,_r} will try to change the string, something unacceptable for a mmaped file, and something that will not work on read-only mappings, which would be the right way to mmap .netrc anyway.

I could either malloc(file_size + spare bytes), load file contents on this mem and null-terminate it, in order to work on that copy with strtok*, but in the unlikely event that someone has an absurdly big .netrc file, this approach might fail. Performance-wise, I don't think it would be a burden in most cases.

Or maybe I can just write a tokenizer function by iterating through the mmaped bytes and filling a char [] with each found token, while keeping track of the beginning of the next token by means of local static pointer.

For me, the current fgets approach and the malloc one seem the most KISS. What are your views about it?

Edit: I'm implemented a custom tokenizer to use along with mmap. I will rewrite the PR and will send as soon as I can.

ismaell · 2019-10-20T01:17:31Z

On 18/Oct/2019 20:51, davidpolverari wrote: <...>

Or maybe I can just write a tokenizer function by iterating through the mmaped bytes and filling a `char []` with each found token, while keeping track of the beginning of the next token by means of local static pointer. For me, the current `fgets` approach and the `malloc` one seem the most KISS. What are your views about it?

It would be worth knowing how it compares in practice, I suspect it's less overhead in the end, but for now the stdio API is fine. It doesn't need to copy anything, and since the format is so simple, you can get away with something as simple as strspn + strncmp, altough we would need a limiting version of strspn, but that's about it.

davidpolverari · 2019-10-22T02:52:18Z

I sent a new commit for the current PR. As soon as the code is ready for merging, I'll rebase everything.

src/netrc.c

davidpolverari · 2019-10-23T20:19:37Z

I already addressed most of the points you made on my local repo. As soon as I implement the remaining ones (netrc_t, strspn-like interace, and --no-netrc) I will send the PR.

ismaell · 2019-10-24T12:30:58Z

On 22/Oct/2019 18:15, davidpolverari wrote: I was afraid of using strlcpy because it is not standard (neither ISO C nor POSIX), and thus could cause portability problems. But if it is ok for you, I'll use it.

We provide strlcpy and strlcat for systems that don't have it on libc and don't have libbsd. It's OK to use non-standard functions if they're generic.

davidpolverari · 2019-11-04T16:26:29Z

I didn't forget about this issue. I was busy because of work, but will resume as soon as possible.

Missing: - add negative version (--no-netrc) - add struct for netrc use, and use a pointer to it instead of using netrc_filename

davidpolverari · 2019-11-12T01:49:21Z

I just sent another WIP commit with most of the proposed changes, including using strspn and strcspn-like functions (which I named memspn and memcspn as they don't deal with null-terminated strings).

Later on I will try to refactor next_token() into something like a memtok_r(), and will try to get rid of the global variables by explicitly passing them as arguments to the auxiliary static functions, besides implementing the remaining proposed changes.

src/netrc.c

davidpolverari · 2019-11-27T17:39:01Z

I believe that with those latest PRs most of the requested things were changed. I have doubts now as to where we should keep the netrc_t pointer, at what point we should init the structure, etc. I think it would be easier if we could decouple the username and password from conn_t, and had a struct used globally for it, but it would imply touching on a lot of places.

After that, I will be able to implement the negative (--no-netrc) option.

davidpolverari · 2019-12-09T20:14:47Z

Well, with this last PR I think the changes are almost all done, the only missing piece being the negative option (and removing the debugging printf I put). There are other things that I thought of doing at a later point in time, as moving the memspn, memcspn and memtok functions elsewhere on axel code, but I think that can be postponed.

Any thoughts about it? When everything is on right shape I can rebase it on master and push it again in a more organized way.

ismaell

Looks much better now. I made a couple of comments, the only important task now is to complete the get_creds implementation.

src/text.c

ismaell · 2019-12-19T12:56:34Z

src/conn.c

-			netrc_t * netrc;
-			netrc = netrc_init(conn->conf->netrc_filename, conn->host, conn->user, sizeof(conn->user), conn->pass, sizeof(conn->pass));
-			netrc_parse(netrc);
+		if (conn->conf->netrc) {


Code here shouldn't be conditional, and it should be for all protocols.

We want to support other formats, like authinfo, in the future.

It should be into a separate function, so that refactoring doesn't touch code around it:

conf_auth_setup(conf);

Also auto-login should only happen if we didn't got the credentials from somewhere else, so a check for that is needed.

Well, I will move this code to the conn.c:conn_set() function, as that is the one that deals with parsing URLs and extracting fields like username, password and hostname from them and copying to conn_t. But once parsing is entangled with auth code, I'd rather leave this kind of refactoring to another PR.

src/netrc.c

davidpolverari · 2019-12-19T20:04:47Z

Looks much better now. I made a couple of comments, the only important task now is to complete the get_creds implementation.

What else do you think it is missing on get_creds() implementation?

ismaell · 2020-01-20T11:59:08Z

@davidpolverari please take a look at ismaell@888ec1e.

That fixes both the null deref at the mmap function, the parsing (which now aborts on unkonwn tokens), and removes two globals.

What I'm missing there is a cache, so that each machine is only parsed once.

- removed netrc_free - simplified netrc_mmap - reworked parser

- simplified parser flow - replaced memcpy with strlcpy

davidpolverari · 2020-01-21T02:23:31Z

Your parser implementation is way more elegant than mine 😄. Anyways, I had to make minor changes to make it work and to simplify (IMHO) the program flow. Besides, we can't use memcpy because the pointers returned by memtok aren't NULL-terminated, thus we need strlcpy to NULL-terminate it the copy for us.

As the parser now aborts on unknown tokens, we may need to include the other tokens used by the format which are unused on axel, and turn them in no-ops, otherwise a compliant file might get ignored.

davidpolverari · 2020-01-21T02:38:13Z

Regarding the caching (memoization), I think we could create a function to wrap netrc_parse and populate/verify/return the previous results contained in an array of few elements of a struct (host, username, password) inside netrc_t, acting like a circular buffer. With few elements, we wouldn't need any fancy algorithm to do it.

But don't you think it is better to implement it as a separate PR later?

src/netrc.c

ismaell · 2020-01-23T09:35:39Z

I failed to mention I made changes at https://github.com/ismaell/axel netrc branch

This reverts commit c2fc839.

davidpolverari · 2020-01-23T23:20:46Z

I cherry-picked your commits here, as they would be the same changes I'd have to make.

ismaell · 2020-04-06T20:34:39Z

@davidpolverari hey, 'sup? are you still around?

davidpolverari · 2020-04-07T06:02:45Z

Hi! Yes, was just a little bit busy with other stuff.

ismaell requested changes Oct 18, 2019

View reviewed changes

WIP refactoring

316997e

davidpolverari commented Oct 22, 2019

View reviewed changes

src/netrc.c Outdated Show resolved Hide resolved

ismaell requested changes Oct 22, 2019

View reviewed changes

Latest WIP commit. Includes most of the requested changes.

d5a9df9

Missing: - add negative version (--no-netrc) - add struct for netrc use, and use a pointer to it instead of using netrc_filename

davidpolverari commented Nov 12, 2019

View reviewed changes

src/netrc.c Outdated Show resolved Hide resolved

davidpolverari added 3 commits November 13, 2019 21:56

Replace next_token with memtok and eliminate global vars.

87d7320

Add netrc_t struct

1a5b638

Use netrc_t (wrongly) to test

ba0539d

Refactor netrc module and its usage

1fc0dcc

ismaell requested changes Dec 19, 2019

View reviewed changes

davidpolverari added 5 commits December 19, 2019 12:03

conn.c: Move netrc credential parsing code to conn_set

32f53bc

text.c: Remove netrc_filename var

97b22a1

netrc.c: Break long line

6d2bfa3

text.c: remove useless ternary operator

d0c858a

netrc: Store pointer to mmaped file instead of filename

24a27fa

netrc: Hide netrc_t implementation details from header file

f9192ab

ismaell and others added 2 commits January 20, 2020 20:46

WIP netrc support

5abebdf

- removed netrc_free - simplified netrc_mmap - reworked parser

WIP: netrc support

802ffcf

- simplified parser flow - replaced memcpy with strlcpy

ismaell reviewed Jan 21, 2020

View reviewed changes

src/netrc.c Outdated Show resolved Hide resolved

ismaell reviewed Jan 21, 2020

View reviewed changes

src/netrc.c Outdated Show resolved Hide resolved

ismaell reviewed Jan 22, 2020

View reviewed changes

src/netrc.c Outdated Show resolved Hide resolved

ismaell reviewed Jan 22, 2020

View reviewed changes

src/netrc.c Show resolved Hide resolved

ismaell reviewed Jan 22, 2020

View reviewed changes

src/netrc.c Outdated Show resolved Hide resolved

Minor changes

c2fc839

davidpolverari and others added 4 commits January 23, 2020 18:09

Revert "Minor changes"

e303c11

This reverts commit c2fc839.

WIP netrc: improve netrc_parse token copying

0103c49

WIP netrc: make parser struct R/O and make netrc_parse void

607f701

WIP netrc: move null check to netrc_parse

67eec63

ismaell self-assigned this Mar 25, 2020

ismaell added this to the v2.18 milestone Mar 25, 2020

ismaell force-pushed the master branch from a52c9c0 to 501f2e5 Compare February 25, 2022 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

netrc: Implement .netrc parsing #244

netrc: Implement .netrc parsing #244

davidpolverari commented Oct 18, 2019

davidpolverari commented Oct 18, 2019

ismaell left a comment

ismaell Oct 18, 2019

davidpolverari Oct 22, 2019

ismaell Oct 22, 2019

davidpolverari Oct 23, 2019

davidpolverari commented Oct 18, 2019

davidpolverari commented Oct 19, 2019 •

edited

ismaell commented Oct 20, 2019 via email

davidpolverari commented Oct 22, 2019

davidpolverari commented Oct 23, 2019

ismaell commented Oct 24, 2019 via email

davidpolverari commented Nov 4, 2019

davidpolverari commented Nov 12, 2019

davidpolverari commented Nov 27, 2019

davidpolverari commented Dec 9, 2019

ismaell left a comment

ismaell Dec 19, 2019

davidpolverari Dec 19, 2019

davidpolverari commented Dec 19, 2019

ismaell commented Jan 20, 2020

davidpolverari commented Jan 21, 2020

davidpolverari commented Jan 21, 2020

ismaell commented Jan 23, 2020

davidpolverari commented Jan 23, 2020

ismaell commented Apr 6, 2020

davidpolverari commented Apr 7, 2020

netrc: Implement .netrc parsing #244

Are you sure you want to change the base?

netrc: Implement .netrc parsing #244

Conversation

davidpolverari commented Oct 18, 2019

davidpolverari commented Oct 18, 2019

ismaell left a comment

Choose a reason for hiding this comment

ismaell Oct 18, 2019

Choose a reason for hiding this comment

davidpolverari Oct 22, 2019

Choose a reason for hiding this comment

ismaell Oct 22, 2019

Choose a reason for hiding this comment

davidpolverari Oct 23, 2019

Choose a reason for hiding this comment

davidpolverari commented Oct 18, 2019

davidpolverari commented Oct 19, 2019 • edited

ismaell commented Oct 20, 2019 via email

davidpolverari commented Oct 22, 2019

davidpolverari commented Oct 23, 2019

ismaell commented Oct 24, 2019 via email

davidpolverari commented Nov 4, 2019

davidpolverari commented Nov 12, 2019

davidpolverari commented Nov 27, 2019

davidpolverari commented Dec 9, 2019

ismaell left a comment

Choose a reason for hiding this comment

ismaell Dec 19, 2019

Choose a reason for hiding this comment

davidpolverari Dec 19, 2019

Choose a reason for hiding this comment

davidpolverari commented Dec 19, 2019

ismaell commented Jan 20, 2020

davidpolverari commented Jan 21, 2020

davidpolverari commented Jan 21, 2020

ismaell commented Jan 23, 2020

davidpolverari commented Jan 23, 2020

ismaell commented Apr 6, 2020

davidpolverari commented Apr 7, 2020

davidpolverari commented Oct 19, 2019 •

edited