-
Notifications
You must be signed in to change notification settings - Fork 766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve install output #38
Conversation
Note to self on how to test this:
|
@@ -235,11 +268,17 @@ install_shadowbox() { | |||
# require new dependencies. | |||
cat <<END_OF_SERVER_OUTPUT | |||
|
|||
Please copy the following configuration to your Outline Manager: | |||
CONGRATULATIONS! Your Outline Server is up and running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outline Server -> Outline server
(and throughout)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
set up, because this host machine has a firewall that is preventing incoming | ||
connections to ports ${SB_API_PORT} and ${ACCESS_KEY_PORT}. | ||
|
||
If you plan to have a single Access Key to access your server, opening those |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Access Key -> access key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
log_for_sentry "Setting SB_PUBLIC_IP" | ||
# TODO(fortuna): Make sure this is IPv4 | ||
readonly SB_PUBLIC_IP=${SB_PUBLIC_IP:-$(curl -4s https://ipinfo.io/ip)} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra blank line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
|
||
log_step "Creating first user" | ||
function check_firewall() { | ||
if ! curl --max-time 5 --cacert "${SB_CERTIFICATE_FILE}" -s "${PUBLIC_API_URL}/access-keys" >/dev/null; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pretty funny that we import the cert only for the connectivity check, not for creating the first user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we shouldn't. The point of using localhost (which requires that we do --insecure) is that we avoid hitting firewall issues and get to install the server completely. The check for firewall can be done last, such that if the admin fixes it post-installation, the Server will work seamlessly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought accessing via localhost
was orthogonal to using the cert, or does the cert include the public IP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cert has the public ip only, so we need to use insecure mode.
We could potentially add localhost to the cert, but we need to figure it out.
It's unclear whether we should worry about a localhost connection being intercepted. You could have a malicious app take over all ports, but I think that may be outside our threat model.
In any case, adding too many options to the cert can make it fingerprintable.
I think it's important to test with the certs, to make sure it's set up properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems totally reasonable, and well worth a comment in the source :-)
|
||
|
||
if [[ ! "$SB_PUBLIC_IP" =~ ^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | ||
echo "Invalid IP lookup result: $SB_PUBLIC_IP" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log_for_error
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
|
||
log_step "Creating first user" | ||
function check_firewall() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect most firewalls to allow connections from localhost addresses, even if they're blocked from the outside. This will have a lot of false positives - but I guess as a placeholder it's okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this may have some false positives. I have no idea how much, but this is definitely an improvement to what we have now.
I've added extra text about the router of cloud provider blocking in case that happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dborkan had the excellent point that he's seen the converse: a firewall that allows connections from localhost but not externally. In that case, this advice is worse than what we have. I think you're right to just always tell the user which ports to open.
|
||
readonly PUBLIC_API_URL="https://${SB_PUBLIC_IP}:${SB_API_PORT}/${SB_API_PREFIX}" | ||
readonly LOCAL_API_URL="https://localhost:${SB_API_PORT}/${SB_API_PREFIX}" | ||
run_step "Waiting for Shadowsocks to be healthy" wait_shadowbox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shadowbox to be healthy, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to Outline Server
|
||
${FIREWALL_STATUS} | ||
To manage your Outline Server, please copy the following text (including curly | ||
brackets) into Step 2 of the Outline Manager interface: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably they've just come from the Outline Manager; I would re-word to something like: "into the Outline Manager".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same screen on the Manager shows both Step 1 and Step 2. I think mentioning Step 2 may feel excessive, but makes it more explicit. The text was suggested by Santi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not going to hold this up on worsmithing but FYI "the Outline Manager interface" is redundant - it's just "the Outline Manager".
FIREWALL_STATUS="\ | ||
You won’t be able to access it externally, despite your server being correctly | ||
set up, because this host machine has a firewall that is preventing incoming | ||
connections to ports ${SB_API_PORT} and ${ACCESS_KEY_PORT}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think right now you're only checking the Shadowbox port, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'm assuming that both are blocked together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, okay.
@fortuna Please consider a squash commit, with a list summarising all of the changes (for example, you removed the call to |
A few open issues which may benefit from this change: |
With this change we display the step before it's done, making it clearer what's going on.
We are now also able to detect if the firewall is blocking the connection.
This should address many of the issues users are having.
@sandrigo FYI
Sample output