Opened 4 years ago

Closed 4 years ago

#9974 closed enhancement (fixed)

packaging and installation scripts for facilitator

Reported by: infinity0 Owned by: dcf
Priority: Medium Milestone:
Component: Archived/Flashproxy Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Work coming out of ticket #9668 - I have written some packaging and installation scripts for the facilitator component, which automates much of the process previously needed to be done manually, in facilitator-howto.txt.

https://github.com/infinity0/flashproxy/compare/master...fac-build

Child Tickets

Change History (21)

comment:1 Changed 4 years ago by infinity0

Extra things to do:

  • email-poller already reads some args from /etc/default/facilitator-email-poller, extend it for as many args as possible
  • do the same thing for reg-daemon and facilitator, which currently don't have defaults files
  • perhaps rename them to something more flashproxy-specific, e.g. fp-facilitator

comment:2 Changed 4 years ago by infinity0

Rebased against master, ready for merge:

https://github.com/infinity0/flashproxy/compare/master...fac-build

I've done 1 and 2 in the list above; but not yet renamed the services. (Not sure if you want to.)

comment:3 in reply to:  1 Changed 4 years ago by dcf

Replying to infinity0:

Extra things to do:

  • email-poller already reads some args from /etc/default/facilitator-email-poller, extend it for as many args as possible
  • do the same thing for reg-daemon and facilitator, which currently don't have defaults files

Do you know how compatible defaults files are with distros other than Debian? Also: how much do we care if they're not?

comment:4 Changed 4 years ago by infinity0

I could make the /etc/default path ./configure-able with minimal effort. Not sure if it that important, since distros would typically have their own postinst/prerm scripts, but I suppose it's still good as a reference, and for developers that want to test on those types of machines.

Gentoo and Arch Linux both use /etc/conf.d.

Fedora uses /etc/sysconfig but they aren't too strict about it, and seem to be moving away from initscripts to systemd. (Debian seems to be trying too, so perhaps we could also provide an example service file at some point.)

comment:5 Changed 4 years ago by asn

Status: newneeds_review

Took a look at the fac-build branch. I'm not familiar enough with autotools or distro default scripts to evaluate the correctness of the changes, but I agree with the logic behind the changes.

Here are some nitpicky notes. Feel free to ignore both.

  • In 82e6dc766a2625f14599968e82fe3c6e5fbc704c you moved MaxClients out of the VirtualHost *:443 block, but AFAIK that was not a config error (as the git commit msg claims). Doesn't really matter, I guess.
  • This is not an issue, but it's worth noting that 4811b51af8a5bf5c97dd1053e89c091a07916f4a kind of changes behavior, since it switches the old default gmail address to 'invalid'. This means that the FACILITATOR_EMAIL_ADDR needs to be edited before starting up facilitator-email-poller, which was not the case in the past. Maybe this should be documented somewhere, like we do for /reg-email.pass with "Replace this file's contents with your Gmail app-specific password;"?

comment:6 Changed 4 years ago by infinity0

I pushed a few more commits, addressing the above and also (hopefully) improving the structure of the now-separated docs.

  • The MaxClients thing I think is dependent on which MPM module you're using - on my server it's mpm-worker and having it inside <VirtualHost> causes an error. Having it outside works in all cases.
  • I've changed the behaviour of this now so that imap_host/email/password are all stored in reg-email.pass. This is quite a lot simpler than splitting it across a initscripts config (which technically is a distro-dependent concern). I've documented this in both reg-email.pass itself and email-howto.txt.
    • The previous behaviour was only really used by dcf anyway, since his email address was hard-coded into the program :p I OKed this new approach with him as well, and the new behaviour should fail-fast for his current setup.
  • I also reworked some of the documentation for better consistency and structure. LMK if I did anything wrong!

comment:7 Changed 4 years ago by dcf

What I'd like to do is try these new scripts in setting up a new facilitator from scratch. I want to upgrade the current facilitator that is running on Debian 6. Could we set a soft deadline, say November 15, before which I'll try the install, and after which it's okay to merge if I haven't had a chance to try it yet?

comment:8 Changed 4 years ago by infinity0

That date works for me. I'll set up a proposed-merge commit too so you can test it together with our other recent changes as well.

comment:9 Changed 4 years ago by infinity0

Ping! I've done a proposed-merge of fac-build and current master here: https://github.com/infinity0/flashproxy/compare/master...merge-next

Last edited 4 years ago by infinity0 (previous) (diff)

comment:10 Changed 4 years ago by dcf

Good job on this. I paid for a new server to try installing this new code on, but I haven't tried setting it up yet. I will try to do so today or tomorrow.

It's not clear to me in what order I should read the different howto documents to set up a new facilitator. I guess it is:

  1. facilitator/doc/server-howto.txt
  2. facilitator/doc/http-howto.txt
  3. facilitator/INSTALL

(Followed by facilitator/doc/appspot-howto.txt and facilitator/doc/email-howto.txt if those were not already set up.) What is your imagined narrative of someone reading and following the instructions? What will they naturally try reading first, and what steps do you want them to take? I checked facilitator/INSTALL first; maybe that file should contain references to the others.

Setting FP_FACILITATOR in fp-reg.go. It's good to make this externally configurable. I think it's better, however, to make the the configurable part a whole URL, and not just the domain part. We should support facilitators running on different ports and with different base paths. Run go fmt after making changes to Go files.

Removing separate mention of the url registration method. I think of url as being distinct from the the http method. http is a relict from an earlier age--we could get rid of http but still keep url. url and appspot are more alike than are url and http. http uses a POST with plaintext parameters in the request body. url uses a GET with an encrypted payload in the path. With the http method, you contact the facilitator yourself, while with the url method the URL is retrieved by some third party. url isn't very nice by itself because it can't find out your external address and therefore can't work automatically, but I think it makes sense to discuss it separately in the context of someone operating a facilitator.

Changes to name resolution in facilitator-email-poller.

  • smarter default IMAP host; and resolve DNS by default - previously one had to use an IP address
  • add a nameOk option to parse_addr_spec as certificate verification requires imap host to be a name

This is a good bug you found. I'm not wild about the new more complicated interface to parse_addr_spec with the nameOk parameter. It's a reasonable thing to add, but I wonder if there is a better way. Maybe add a new lower-level function that additionally returns the host part string along with what parse_addr_spec normally returns--then parse_addr_spec could call the lower-level function and throw the extra information away.

I don't think we need the heuristic of guessing imap.<domain> based on the email address. Just make the user be explicit. (Realistically, zero people are ever going to change the IMAP host, because the program simply won't work with any other mail provider than Gmail. If we were to add another provider, it might be a good design to have a separate poller program to do that, tailored for that other provider's idiosyncrasies.)

comment:11 in reply to:  10 ; Changed 4 years ago by infinity0

Replying to dcf:

Good job on this. I paid for a new server to try installing this new code on, but I haven't tried setting it up yet. I will try to do so today or tomorrow.

It's not clear to me in what order I should read the different howto documents to set up a new facilitator. I guess it is:

  1. facilitator/doc/server-howto.txt
  2. facilitator/doc/http-howto.txt
  3. facilitator/INSTALL

(Followed by facilitator/doc/appspot-howto.txt and facilitator/doc/email-howto.txt if those were not already set up.) What is your imagined narrative of someone reading and following the instructions? What will they naturally try reading first, and what steps do you want them to take? I checked facilitator/INSTALL first; maybe that file should contain references to the others.

facilitator/README is a general overview that contains references to the others. INSTALL is mentioned first, then later "At a very minimum, you must configure and enable the HTTP method" then after that server-howto.txt. (I could move it up a bit, but it's now just some non-canonical suggestions, and only configures SSH and a firewall.)

Setting FP_FACILITATOR in fp-reg.go. It's good to make this externally configurable. I think it's better, however, to make the the configurable part a whole URL, and not just the domain part. We should support facilitators running on different ports and with different base paths. Run go fmt after making changes to Go files.

I believe the existing code already supports this, since we only put https:// and /reg/ around FP_FACILITATOR which is just a string. (/reg/ is hard-coded into facilitator.cgi so I thought to leave it hard-coded here too.) I'll fix the docstring to mention this though, right now it just says "host[:port]".

Removing separate mention of the url registration method. I think of url as being distinct from the the http method. http is a relict from an earlier age--we could get rid of http but still keep url. url and appspot are more alike than are url and http. http uses a POST with plaintext parameters in the request body. url uses a GET with an encrypted payload in the path. With the http method, you contact the facilitator yourself, while with the url method the URL is retrieved by some third party. url isn't very nice by itself because it can't find out your external address and therefore can't work automatically, but I think it makes sense to discuss it separately in the context of someone operating a facilitator.

I had added some notes to this effect here. We can tweak it to make the distinction clearer if you want, too.

Changes to name resolution in facilitator-email-poller.

  • smarter default IMAP host; and resolve DNS by default - previously one had to use an IP address
  • add a nameOk option to parse_addr_spec as certificate verification requires imap host to be a name

This is a good bug you found. I'm not wild about the new more complicated interface to parse_addr_spec with the nameOk parameter. It's a reasonable thing to add, but I wonder if there is a better way. Maybe add a new lower-level function that additionally returns the host part string along with what parse_addr_spec normally returns--then parse_addr_spec could call the lower-level function and throw the extra information away.

I think a separate function would only move the complexity around. I found it weird that previously the "resolve" param controlled both input acceptance and output type. Ultimately, we need three behaviours:

  1. accepts name/ip as input, gives ip as output (nameOk, resolve)
  2. accepts name/ip as input, gives original as output (nameOk, not resolve)
  3. accepts ip as input, gives original as output (not nameOk, not resolve)

In the newer version, "nameOk" controls which inputs are accepted and "resolve" controls the type of output. If we delegate (2) to a separate function, we'd only need one flag (resolve) but another function for users to know about. I think I prefer the two-simple-flags approach rather than one-complex-flag-and-one-separate-function.

Incidentally, (2) is what parse_addr_spec from the client code does - accepts DNS names (so you can say e.g. localhost:9000), but doesn't resolve them. (But I still think 2-params is better.)

I don't think we need the heuristic of guessing imap.<domain> based on the email address. Just make the user be explicit. (Realistically, zero people are ever going to change the IMAP host, because the program simply won't work with any other mail provider than Gmail. If we were to add another provider, it might be a good design to have a separate poller program to do that, tailored for that other provider's idiosyncrasies.)

OK, I can get rid of this for now. I suppose it's easier for the case where you have a gmail address that doesn't end in @gmail.com.

comment:12 Changed 4 years ago by dcf

Status: needs_reviewneeds_revision

Installation of the merge-next branch (dfd80a48930b328a8410172bc74fcfdf8593428f) went pretty smoothly. Thank you for your work on this. I can see that you have been diligent and taken care of many details. I took some notes during the installation.

I made a trivial commit on top of your merge-next branch:

I found that I couldn't follow facilitator/INSTALL and facilitator/doc/http-howto.txt in strict sequence. Instead, I had to first install Apache, then install the facilitator, then edit the Apache config. The reason is that running configure for the facilitator requires Apache so that the CGI directory can be inferred:

$ ./configure --enable-initscripts
...
configure: WARNING: could not determine system CGI executables dir, using $(libexecdir); set cgibindir to override.
...

...and the Apache config file facilitator/examples/fp-facilitator.conf doesn't exist until after you've done ./configure and make in the facilitator directory.

Commit 0080b8dff74c583682867eb190e49237fee3b9db turned Apache logging on, and the commit log didn't have a reason why. I disabled logging manually.

I had copied the reg-email.pass file from the other facilitator. Of course that meant it wasn't in the proper new format. The error message printed the actual secret password to the screen, which it shouldn't do. It didn't appear to write the password to any log files that I could find.

# /etc/init.d/facilitator-email-poller start
Failed to parse password file "/usr/local/etc/flashproxy/reg-email.pass": could not find email or password: <actual password>

I didn't find any documentation telling you to edit $(sysconfdir)/default/* to set RUN_DAEMON=yes. I had to do that for facilitator, facilitator-reg-daemon, and facilitator-email-poller.

The install scripts should probably create the $(localstatedir)/log and $(localstatedir)/run directories. With the --localstatedir given in the instructions they probably already exist, but when I had localstatedir=/usr/local/var, I got errors:

Traceback (most recent call last):
  File "/usr/local/bin/facilitator-reg-daemon", line 212, in <module>
    main()
  File "/usr/local/bin/facilitator-reg-daemon", line 176, in main
    options.log_file = open(options.log_filename, "a")
IOError: [Errno 2] No such file or directory: '/usr/local/var/log/facilitator-reg-daemon.log'

  File "/usr/local/bin/facilitator", line 518, in <module>
    main()
  File "/usr/local/bin/facilitator", line 499, in main
    f = open(options.pid_filename, "w")
IOError: [Errno 2] No such file or directory: '/usr/local/var/run/facilitator.pid'

I didn't find a mention in the documentation of the need to install $(sysconfdir)/flashproxy/relays and what should go in there. I got an error at startup:

# /etc/init.d/facilitator start
Traceback (most recent call last):
  File "/usr/local/bin/facilitator", line 518, in <module>
    main()
  File "/usr/local/bin/facilitator", line 472, in main
    with open(options.relay_filename) as fp:
IOError: [Errno 2] No such file or directory: '/usr/local/etc/flashproxy/relays'

The defaults files say:

# Uncomment this to log potentially sensitive information from your users.
# This may be useful for debugging or diagnosing functional problems, but
# should be avoided in a high-risk environment.
#UNSAFE_LOGGING="yes"

I don't disagree with the sentiment, and the default setting is correct. What's a high-risk environment? We should rather recommend it to everyone, even those in a trusted environment. Call it defense in depth or whatever, it's better not to be sitting on a pile of IP addresses.

I got an error when I tried to start facilitator-email-poller. It looks like the --email option was not removed from the init script nor the getopt call. FACILITATOR_EMAIL_ADDR doesn't seem to be defined anywhere.

/etc/init.d/facilitator-email-poller:
DAEMON_ARGS="$DAEMON_ARGS --email $FACILITATOR_EMAIL_ADDR"

# /etc/init.d/facilitator-email-poller start
Traceback (most recent call last):
  File "/usr/local/bin/facilitator-email-poller", line 169, in <module>
    "unsafe-logging",
  File "/usr/lib/python2.7/getopt.py", line 131, in gnu_getopt
    opts, args = do_longs(opts, args[0][2:], longopts, args[1:])
  File "/usr/lib/python2.7/getopt.py", line 156, in do_longs
    raise GetoptError('option --%s requires argument' % opt, opt)
getopt.GetoptError: option --email requires argument

Seeing this error made me think. I like what you did with changing the format of the reg-email.pass file. Suppose we made that file a mapping of email→password, potentially with many lines. Then put back the --email and --imap options, which would cause the proper line to be selected. The email address and password file path could be set in defaults/facilitator-email-poller.

I was surprised that the App Engine files ended up in $(sysconfdir)/flashproxy.

$ ls -l /usr/local/etc/flashproxy/reg-appspot/
total 4
lrwxrwxrwx 1 root root  58 Nov 17 06:15 app.yaml -> /usr/local/share/flashproxy-facilitator/appengine/app.yaml
-rw-r--r-- 1 root root 137 Nov 17 06:15 config.go
lrwxrwxrwx 1 root root  59 Nov 17 06:15 fp-reg.go -> /usr/local/share/flashproxy-facilitator/appengine/fp-reg.go
lrwxrwxrwx 1 root root  56 Nov 17 06:15 README -> /usr/local/share/flashproxy-facilitator/appengine/README

I can kind of see why it would make sense, after all you do have to configure config.go. And you do want someone installing a facilitator to have easy access to the source code. I don't think it matches what I see as the recommended use case, though--people shouldn't be uploading their App Engine code from the facilitator itself. That can be done with a separate Google account whose password can be kept offline. People are also going to want to try at least compiling the code before uploading it, and it seems weird to be doing that in etc. I think it's sufficient to install the files in /usr/local/share and then refer to them in documentation.

comment:13 in reply to:  11 ; Changed 4 years ago by dcf

Replying to infinity0:

Replying to dcf:

Setting FP_FACILITATOR in fp-reg.go. It's good to make this externally configurable. I think it's better, however, to make the the configurable part a whole URL, and not just the domain part. We should support facilitators running on different ports and with different base paths. Run go fmt after making changes to Go files.

I believe the existing code already supports this, since we only put https:// and /reg/ around FP_FACILITATOR which is just a string. (/reg/ is hard-coded into facilitator.cgi so I thought to leave it hard-coded here too.) I'll fix the docstring to mention this though, right now it just says "host[:port]".

I'm actually thinking about the case where someone does

        ScriptAliasMatch ^mybasepath/(.*) /usr/local/bin/facilitator.cgi$1

instead of

        ScriptAliasMatch ^(.*) /usr/local/bin/facilitator.cgi$1

Then the reg path would be at /mybasepath/reg, rather than /reg. I think it's fine to leave the "reg" part hardcoded. I've thought about changing to something like this so we can serve an actual HTML page at /, with information about what the facilitator is. But I can easily take care of making the configuration a URL. url.Parse is what you would use to resolve a relative path against a base URL.

comment:14 in reply to:  11 ; Changed 4 years ago by dcf

Replying to infinity0:

Replying to dcf:

Changes to name resolution in facilitator-email-poller.

  • smarter default IMAP host; and resolve DNS by default - previously one had to use an IP address
  • add a nameOk option to parse_addr_spec as certificate verification requires imap host to be a name

This is a good bug you found. I'm not wild about the new more complicated interface to parse_addr_spec with the nameOk parameter. It's a reasonable thing to add, but I wonder if there is a better way. Maybe add a new lower-level function that additionally returns the host part string along with what parse_addr_spec normally returns--then parse_addr_spec could call the lower-level function and throw the extra information away.

I think a separate function would only move the complexity around. I found it weird that previously the "resolve" param controlled both input acceptance and output type. Ultimately, we need three behaviours:

  1. accepts name/ip as input, gives ip as output (nameOk, resolve)
  2. accepts name/ip as input, gives original as output (nameOk, not resolve)
  3. accepts ip as input, gives original as output (not nameOk, not resolve)

In the newer version, "nameOk" controls which inputs are accepted and "resolve" controls the type of output. If we delegate (2) to a separate function, we'd only need one flag (resolve) but another function for users to know about. I think I prefer the two-simple-flags approach rather than one-complex-flag-and-one-separate-function.

Incidentally, (2) is what parse_addr_spec from the client code does - accepts DNS names (so you can say e.g. localhost:9000), but doesn't resolve them. (But I still think 2-params is better.)

I see. The facilitator's parse_addr_spec really is a different kind of beast. For that reason, maybe we should make it the exception, with a different function name, rather than foisting its more complicated interface on the other programs that need parse_addr_spec. Maybe a separate resolve(host, port) function would do it. Unless I'm mistaken, the facilitator's weird mutant parse_addr_spec is the same as flashproxy.util.parse_addr_spec, with the additional step of parsing/name resolution.

I see only one place with resolve=True, and resolution arguably isn't needed there. In parsing client registrations, we still want to socket.getaddrinfo(socket.AI_NUMERICHOST) somehow, because we don't want to resolve untrusted DNS names nor give them to proxies. The only other place fac.parse_addr_spec is used is in flashproxy-email-poller to parse the IMAP server, and there it's clear we should be using plain flashproxy.util.parse_addr_spec instead.

That's my proposal: Use good old flashproxy.util.parse_addr_spec everywhere, and in the few cases where we need to resolve the address or ensure that it is numeric, do that as a separate step. (The parse_addr_spec+extra step can be encapsulated in a function, of course.)

For what it's worth, the resolve parameter doesn't control the output format. The "gives original as output" case doesn't exist in the facilitator's parse_addr_spec as it is written now. The IP address is always given to socket.getaddrinfo to be either parsed or resolved: if resolve is false, then name resolution is disabled with the socket.AI_NUMERICHOST flag. It seems like it's returning the original string because Python's representation of an IP address is a string, which in this case happens to be equal to the input string. Some examples show that it really is parsing the address and not just echoing its input:

>>> fac.parse_addr_spec("[1:0:0:0:0:0:0:2]:9999", resolve=True)
('1::2', 9999)
>>> fac.parse_addr_spec("[1:0:0:0:0:0:0:2]:9999", resolve=False)
('1::2', 9999)
>>> fac.parse_addr_spec("example.com:9999", resolve=False)
ValueError: Bad host or port: "example.com" "9999": [Errno -2] Name or service not known
Last edited 4 years ago by dcf (previous) (diff)

comment:15 in reply to:  13 Changed 4 years ago by infinity0

Replying to dcf:

Installation of the merge-next branch (dfd80a48930b328a8410172bc74fcfdf8593428f) went pretty smoothly. Thank you for your work on this. I can see that you have been diligent and taken care of many details. I took some notes during the installation.

I made a trivial commit on top of your merge-next branch:

I found that I couldn't follow facilitator/INSTALL and facilitator/doc/http-howto.txt in strict sequence. Instead, I had to first install Apache, then install the facilitator, then edit the Apache config. The reason is that running configure for the facilitator requires Apache so that the CGI directory can be inferred

I've added instructions to install apache2 to INSTALL, which takes care of the install-from-source case. OTOH distros can set cgibindir manually in their packaging scripts, and avoid installing apache2 at build-time. We'll probably do something like Recommends: httpd-cgi | apache2 in the Debian package.

Commit 0080b8dff74c583682867eb190e49237fee3b9db turned Apache logging on, and the commit log didn't have a reason why. I disabled logging manually.

I've put /dev/null back. Sorry, this was an oversight. I think I was a bit too keen in "making things consistent".

I had copied the reg-email.pass file from the other facilitator. Of course that meant it wasn't in the proper new format. The error message printed the actual secret password to the screen, which it shouldn't do. It didn't appear to write the password to any log files that I could find.

# /etc/init.d/facilitator-email-poller start
Failed to parse password file "/usr/local/etc/flashproxy/reg-email.pass": could not find email or password: <actual password>

I've change this to print the line number instead.

I didn't find any documentation telling you to edit $(sysconfdir)/default/* to set RUN_DAEMON=yes. I had to do that for facilitator, facilitator-reg-daemon, and facilitator-email-poller.

Fixed

The install scripts should probably create the $(localstatedir)/log and $(localstatedir)/run directories. With the --localstatedir given in the instructions they probably already exist, but when I had localstatedir=/usr/local/var, I got errors:

Traceback (most recent call last):
  File "/usr/local/bin/facilitator-reg-daemon", line 212, in <module>
    main()
  File "/usr/local/bin/facilitator-reg-daemon", line 176, in main
    options.log_file = open(options.log_filename, "a")
IOError: [Errno 2] No such file or directory: '/usr/local/var/log/facilitator-reg-daemon.log'

  File "/usr/local/bin/facilitator", line 518, in <module>
    main()
  File "/usr/local/bin/facilitator", line 499, in main
    f = open(options.pid_filename, "w")
IOError: [Errno 2] No such file or directory: '/usr/local/var/run/facilitator.pid'

I'll have to think about the best way to do this... those directories are more strictly the site-local admin's concern; if they are intentionally installing stuff there, they should have created those directories manually. But this is true for /var/local too, run and log don't exist by default there.

I didn't find a mention in the documentation of the need to install $(sysconfdir)/flashproxy/relays and what should go in there. I got an error at startup:

# /etc/init.d/facilitator start
Traceback (most recent call last):
  File "/usr/local/bin/facilitator", line 518, in <module>
    main()
  File "/usr/local/bin/facilitator", line 472, in main
    with open(options.relay_filename) as fp:
IOError: [Errno 2] No such file or directory: '/usr/local/etc/flashproxy/relays'

Fixed

The defaults files say:

# Uncomment this to log potentially sensitive information from your users.
# This may be useful for debugging or diagnosing functional problems, but
# should be avoided in a high-risk environment.
#UNSAFE_LOGGING="yes"

I don't disagree with the sentiment, and the default setting is correct. What's a high-risk environment? We should rather recommend it to everyone, even those in a trusted environment. Call it defense in depth or whatever, it's better not to be sitting on a pile of IP addresses.

Changed to "most other cases".

I got an error when I tried to start facilitator-email-poller. It looks like the --email option was not removed from the init script nor the getopt call. FACILITATOR_EMAIL_ADDR doesn't seem to be defined anywhere.

/etc/init.d/facilitator-email-poller:
DAEMON_ARGS="$DAEMON_ARGS --email $FACILITATOR_EMAIL_ADDR"

# /etc/init.d/facilitator-email-poller start
Traceback (most recent call last):
  File "/usr/local/bin/facilitator-email-poller", line 169, in <module>
    "unsafe-logging",
  File "/usr/lib/python2.7/getopt.py", line 131, in gnu_getopt
    opts, args = do_longs(opts, args[0][2:], longopts, args[1:])
  File "/usr/lib/python2.7/getopt.py", line 156, in do_longs
    raise GetoptError('option --%s requires argument' % opt, opt)
getopt.GetoptError: option --email requires argument

Fixed

Replying to dcf:

Replying to infinity0:

Replying to dcf:

Setting FP_FACILITATOR in fp-reg.go. It's good to make this externally configurable. I think it's better, however, to make the the configurable part a whole URL, and not just the domain part. We should support facilitators running on different ports and with different base paths. Run go fmt after making changes to Go files.

I believe the existing code already supports this, since we only put https:// and /reg/ around FP_FACILITATOR which is just a string. (/reg/ is hard-coded into facilitator.cgi so I thought to leave it hard-coded here too.) I'll fix the docstring to mention this though, right now it just says "host[:port]".

I'm actually thinking about the case where someone does

        ScriptAliasMatch ^mybasepath/(.*) /usr/local/bin/facilitator.cgi$1

instead of

        ScriptAliasMatch ^(.*) /usr/local/bin/facilitator.cgi$1

Then the reg path would be at /mybasepath/reg, rather than /reg. I think it's fine to leave the "reg" part hardcoded. I've thought about changing to something like this so we can serve an actual HTML page at /, with information about what the facilitator is. But I can easily take care of making the configuration a URL. url.Parse is what you would use to resolve a relative path against a base URL.

er, I'm still slightly confused here. The current code already supports this - FP_FACILITATOR is just a string, which you can set to "myfacilitator.com/basepath" and the appengine program will just prepend "https://" and append "/reg/" to make "https://myfacilitator.com/basepath/reg/". You could parse it as a URL for validation, though.

comment:16 in reply to:  12 Changed 4 years ago by infinity0

Replying to dcf:

Seeing this error made me think. I like what you did with changing the format of the reg-email.pass file. Suppose we made that file a mapping of email→password, potentially with many lines. Then put back the --email and --imap options, which would cause the proper line to be selected. The email address and password file path could be set in defaults/facilitator-email-poller.

(Not done this yet.) With the current setup, --imap is unnecessary since it's already in the file. However, if you wanted to read the password from say, /etc/exim4/passwd.client, then --imap would be useful since that file only contains SMTP address patterns. (But it also uses : as a separator rather than space.) Is that the use-case you had in mind?

I was surprised that the App Engine files ended up in $(sysconfdir)/flashproxy.

$ ls -l /usr/local/etc/flashproxy/reg-appspot/
total 4
lrwxrwxrwx 1 root root  58 Nov 17 06:15 app.yaml -> /usr/local/share/flashproxy-facilitator/appengine/app.yaml
-rw-r--r-- 1 root root 137 Nov 17 06:15 config.go
lrwxrwxrwx 1 root root  59 Nov 17 06:15 fp-reg.go -> /usr/local/share/flashproxy-facilitator/appengine/fp-reg.go
lrwxrwxrwx 1 root root  56 Nov 17 06:15 README -> /usr/local/share/flashproxy-facilitator/appengine/README

I can kind of see why it would make sense, after all you do have to configure config.go. And you do want someone installing a facilitator to have easy access to the source code. I don't think it matches what I see as the recommended use case, though--people shouldn't be uploading their App Engine code from the facilitator itself. That can be done with a separate Google account whose password can be kept offline. People are also going to want to try at least compiling the code before uploading it, and it seems weird to be doing that in etc. I think it's sufficient to install the files in /usr/local/share and then refer to them in documentation.

My main aim was to reduce the work needed to set that up - simply installing it to a readonly location where you can't edit config.go, isn't good enough from that perspective. Installing symlinks is something that could and should be automated

AFAICS, if nothing goes wrong, you only really need run appcfg.py once for the upload. In this case, you can run it from the same machine as the facilitator and give --no_cookies to appcfg.py, so that it doesn't store authcookies to disk. So I'm not sure it's really worth the effort to run it from a separate machine. For testing, it's probably best to run it on a non-production server.

I've updated the docs to mention --no_cookies and testing on a separate machine, but I could also follow-up with these options if you really want to get rid of the /etc/ symlinks.

  • install to /var/lib instead, but it makes it a bit more confusing for the user. (mediawiki does this, symlinking to both /etc/ config files and /usr/share program files)
  • add a shell script that the user can run, to create these symlinks and copy config.go to a directory of his choice (maybe automatically run this on /var/lib on post-install)
  • separate Debian binary package for the appengine files
  • could even have a separate install script, but that's a bit overkill

comment:17 in reply to:  14 ; Changed 4 years ago by infinity0

Replying to infinity0:

Replying to dcf:

The install scripts should probably create the $(localstatedir)/log and $(localstatedir)/run directories. With the --localstatedir given in the instructions they probably already exist, but when I had localstatedir=/usr/local/var, I got errors:

Traceback (most recent call last):
  File "/usr/local/bin/facilitator-reg-daemon", line 212, in <module>
    main()
  File "/usr/local/bin/facilitator-reg-daemon", line 176, in main
    options.log_file = open(options.log_filename, "a")
IOError: [Errno 2] No such file or directory: '/usr/local/var/log/facilitator-reg-daemon.log'

  File "/usr/local/bin/facilitator", line 518, in <module>
    main()
  File "/usr/local/bin/facilitator", line 499, in main
    f = open(options.pid_filename, "w")
IOError: [Errno 2] No such file or directory: '/usr/local/var/run/facilitator.pid'

I'll have to think about the best way to do this... those directories are more strictly the site-local admin's concern; if they are intentionally installing stuff there, they should have created those directories manually. But this is true for /var/local too, run and log don't exist by default there.

Fixed, just did it the simple way in Makefile.am in install-daemon

Replying to dcf:

I see. The facilitator's parse_addr_spec really is a different kind of beast. For that reason, maybe we should make it the exception, with a different function name, rather than foisting its more complicated interface on the other programs that need parse_addr_spec. Maybe a separate resolve(host, port) function would do it. Unless I'm mistaken, the facilitator's weird mutant parse_addr_spec is the same as flashproxy.util.parse_addr_spec, with the additional step of parsing/name resolution.

I see only one place with resolve=True, and resolution arguably isn't needed there. In parsing client registrations, we still want to socket.getaddrinfo(socket.AI_NUMERICHOST) somehow, because we don't want to resolve untrusted DNS names nor give them to proxies. The only other place fac.parse_addr_spec is used is in flashproxy-email-poller to parse the IMAP server, and there it's clear we should be using plain flashproxy.util.parse_addr_spec instead.

That's my proposal: Use good old flashproxy.util.parse_addr_spec everywhere, and in the few cases where we need to resolve the address or ensure that it is numeric, do that as a separate step. (The parse_addr_spec+extra step can be encapsulated in a function, of course.)

Fixed.

Everything is here: https://github.com/infinity0/flashproxy/compare/merge-next...fac-build

(I decided to work on top of the previous merge for a simpler diff; it also saves my earlier conflict resolution into the git history.)

Things left to do are are --email as a selector into reg-email.pass and better handling of the appspot files, but if you're alright with it I'll do those after the final merge into master.

comment:18 in reply to:  17 Changed 4 years ago by dcf

Status: needs_revisionneeds_review

Replying to infinity0:

Everything is here: https://github.com/infinity0/flashproxy/compare/merge-next...fac-build

(I decided to work on top of the previous merge for a simpler diff; it also saves my earlier conflict resolution into the git history.)

Things left to do are are --email as a selector into reg-email.pass and better handling of the appspot files, but if you're alright with it I'll do those after the final merge into master.

I'm fine with doing those things after merging. I'd like to take another look at the overall diff, but go ahead and merge it anyway if I haven't done so by Thursday 21 November.

comment:19 Changed 4 years ago by infinity0

updated the branch pointers:

https://github.com/infinity0/flashproxy/compare/_old_fac-build...fac-build - the edits since your last review.

https://github.com/infinity0/flashproxy/compare/master...merge-next - the "overall diff" for the entire branch from way back when

comment:20 Changed 4 years ago by dcf

Latest changes look good. Thanks for your work on this.

comment:21 Changed 4 years ago by infinity0

Resolution: fixed
Status: needs_reviewclosed

Merged, thanks!

Note: See TracTickets for help on using tickets.