Enable basic-auth by default #743

alexellis · 2018-07-03T09:45:57Z

Description

Enable basic-auth by default.

This changes ./deploy_stack.sh to generate and then apply secrets turning
on the built-in basic auth feature of the gateway.

Motivation and Context

I have raised an issue to propose this change (required)

#350 #74

The deployment script will enable basic-auth by default to help
avoid people deploying to a public IP with no protection from
malicious actors.

How Has This Been Tested?

Tested on MacOS with and without --no-auth flag.
Does not apply for armhf or powershell.

Example workflow for testing:

Test with positive and negative workflows: positive - password supplied and consumed, show that auth is working by using wrong password. Negative flow - disable auth and check auth is not required.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

Will need documentation change and change to integration tests to disable auth via --no-auth flag when deploying during testing.

martindekov · 2018-07-03T11:58:04Z

When i test the change on docker playground deploy stack gives me
./deploy_stack.sh: line 27: shasum: not found
and the password field is left blank.

alexellis · 2018-07-03T12:06:20Z

That's good to know (probably because they use Alpine as a shell). Did the updated commit I pushed fix it? It seemed to fix it for me.

martindekov · 2018-07-03T12:11:35Z

Now it works as expected.

alexellis · 2018-07-03T14:37:59Z

Great 👍

I saw some issues on DO with it hanging when I typed "head -c 1 /dev/random" - wonder if this is an issue with using VMs?

viveksyngh · 2018-07-04T05:11:52Z

This works as expected.

But one thing I noticed, when we deploy stack with --no-auth flag it deploys and changes the docker-compose.yml. When we again try to deploy the stack with auth it does not change it back and deploys with basic_auth: false. It took me some time to figure out, I was wondering the auth is not working. I think we should add that check.

johnmccabe

Few comments, have also run locally and it "works for me" (tm).

johnmccabe · 2018-07-04T13:27:49Z

deploy_stack.sh

+then
+  echo ""
+  echo "Enabling basic authentication for gateway.."
+  echo ""


I think you need to include the sed command here as well.

Say for example you deploy a stack with --no-auth this will alter the docker-compose.yml disabling auth, but if you then tear down the stack and run again without disabling auth, then you'd get the same altered compose file so auth would remain disabled.

What about using envvars in the compose file instead (https://docs.docker.com/compose/environment-variables/) set enabled=true/false in these conditionals then run the final deploy with:

OPENFAAS_AUTH=$enabled docker stack deploy func --compose-file docker-compose.yml

I prefer the env-var approach with a default set.

johnmccabe · 2018-07-04T13:28:50Z

deploy_stack.sh

+  sed -i -r.bak 's/basic_auth: \"true\"/basic_auth: \"false\"/' docker-compose.yml
+fi
+
+echo "Deploying OpenFaaS core services"


For consistency you'd want the ellipsis at the end of this output.

johnmccabe · 2018-07-04T13:34:23Z

deploy_stack.sh

+then
+  echo "[Credentials]\n username: admin \n password: $secret\n echo -n "$secret" | faas-cli login --username=admin --password-stdin"
+else
+  echo "[Credentials]\n already exist, not creating"


I'd be inclined to give the user the option to replace these secrets. Could be in a situation where they've been setup in the past but the user is no longer sure what they had been set to (cough me).

How about docker secret create?

How about something like this in the troubleshooting guide?

docker service create --image alpine:3.7 --secret secret-name cat /run/secrets/secret-name

johnmccabe · 2018-07-04T13:37:33Z

deploy_stack.sh

+echo "$secret" | docker secret create basic-auth-password -
+if [ $? = 0 ];
+then
+  echo "[Credentials]\n username: admin \n password: $secret\n echo -n "$secret" | faas-cli login --username=admin --password-stdin"


The echo -n... needs some context rather than just printing along with the username and password. Ie blank line spacer and some text telling the user to use this to log into the gateway with the client.

Or even better, offer to do so on behalf of the user (if faas-cli is found on the path).

You should also make very clear that the user should record/store the generated user credentials as they'll not be made available again.

What context should we give about -n?

alexellis · 2018-07-05T07:59:36Z

@viveksyngh could you try modifying locally with the env-var variation suggestion by John? If that looks good I'll add it too.

viveksyngh · 2018-07-05T08:32:31Z

@alexellis I will try that.

viveksyngh · 2018-07-05T15:49:13Z

I will have this one ready over the weekend.

viveksyngh · 2018-07-05T18:09:12Z

@alexellis I have added it here and It works as I have tested on my local.

viveksyngh@51f7818

ivanayov · 2018-07-06T12:43:36Z

deploy_stack.sh

+# Secrets should be created even if basic-auth is disabled.
+echo "Attempting to create credentials for gateway.."
+echo "admin" | docker secret create basic-auth-user -
+secret=$(head -c 16 /dev/urandom| $sha_cmd | cut -d " " -f 1)


Would be good if there is an easy way for the user to create own credentials or edit the password without the need to set-up secrets manually.

What about passing and argument to deploy_stack.sh?

Possibly in a follow-up PR. I think it's out of scope for this change. The user really must run the faas-cli login command though.

alexellis · 2018-07-13T14:14:24Z

Derek set milestone: 0.8.6

@viveksyngh

The deployment script will enable basic-auth by default to help avoid people deploying to a public IP with no protection from malicious actors. - In deploy_stash.sh /dev/random can hang on some systems, so using urandom will give a better experience, if less "random" data. For the purposes of creating an initial basic auth password this is sufficient. - Alpine Linux does not have the shasum command, but sha256sum. - Tested on MacOS with and without --no-auth flag. - Does not apply for armhf or powershell. BASIC_AUTH env-var added by Vivek Syngh @viveksyngh Signed-off-by: Alex Ellis (VMware) <[email protected]>

alexellis · 2018-07-13T14:21:02Z

Somehow the PR I merged from @viveksyngh brought in way too many commits and I couldn't resolve the issues. I've now rebased and force-pushed with a longer commit message and give credit to @viveksyngh for the part he added.

This is a breaking change so please can: @ivanayov @MrTinD @johnmccabe take another look?

Alex

alexellis · 2018-07-13T14:33:01Z

Docs PR: openfaas/docs#45

martindekov · 2018-07-13T14:57:44Z

Ran new OpenFaas with auto-auth branch on a cloud deployed a function it seems that everything works.

alexellis · 2018-07-14T18:17:18Z

Thanks @MrTinD - did you do a "negative test" too? i.e. deploy with --no-auth?

rgee0 · 2018-07-15T10:00:30Z

Haven't got a great deal of time right now, but since the change 2 days ago I'm seeing deployment generate a password but not apply it - the UI is open.

I'll have a closer look when I can. Logging here in case someone else can have a look in the meantime.

viveksyngh · 2018-07-15T10:16:33Z

@rgee0 can you check if your docker-compose.yml was modified when you tried to deploy it with deploy_stack.sh --no-auth. It happened to me as well. We have fixed that using env-var for BASIC_AUTH. Can you also take a pull and try again?

rgee0 · 2018-07-15T10:19:52Z

@viveksyngh I'm working on the automation & pull clean on each run. It worked last week and it doesn't now. Has the mode of operation changed?

alexellis · 2018-07-15T10:24:05Z

Yes I merged a PR via Vivek into my commit, but don't expect functionality to change.

viveksyngh · 2018-07-15T10:26:51Z

we were using sed command to update the docker-compose.yml when we were deploying with --no-auth but same was not getting updated when we deploy again without --no-auth flag which can cause a similar issue.

rgee0 · 2018-07-15T12:58:47Z

I think I've found it. In docker-compose.yml we see basic_auth: "false" and there is nothing in deploy_stack.sh to alter this & the env-var hasn't propagated through.

viveksyngh · 2018-07-16T06:56:53Z

Yes, that's what I was talking about. 👍

rgee0

Currently this is broken. Needs docker-compose changes and retest before merge

alexellis · 2018-07-15T13:25:19Z

deploy_stack.sh

@@ -5,6 +5,50 @@ if ! [ -x "$(command -v docker)" ]; then
  exit 1
 fi

-echo "Deploying stack"
-docker stack deploy func --compose-file docker-compose.yml
+export BASIC_AUTH="true"


@rgee0 this sets the value to true

- basic_auth is now set to true by default unless overriden in the env-var "BASIC_AUTH" Signed-off-by: Alex Ellis (VMware) <[email protected]>

Secrets for gateway needed to be bound to stack, but were commented-out. Tested on Swarm. Signed-off-by: Alex Ellis (VMware) <[email protected]>

alexellis · 2018-07-16T11:46:11Z

I've restored the changes but can't get the login to work via the faas-cli now.. @rgee0 @viveksyngh

Signed-off-by: Alex Ellis (VMware) <[email protected]>

alexellis · 2018-07-16T11:49:55Z

All fixed now and re-tested with Swarm. The issue was a local stack.yml in the faas folder with a gateway URL not found in the openfaas config file. Maybe we can bubble that up better as an error?

rgee0 · 2018-07-16T15:48:13Z

Glad we caught that. Looks a lot better through my automation now.

alexellis requested review from johnmccabe and rgee0 July 3, 2018 09:46

alexellis mentioned this pull request Jul 3, 2018

Proposal: Enable auth by default for Swarm #744

Closed

alexellis mentioned this pull request Jul 3, 2018

Add SHA checking to get.sh download script openfaas/faas-cli#425

Merged

11 tasks

johnmccabe suggested changes Jul 4, 2018

View reviewed changes

ivanayov reviewed Jul 6, 2018

View reviewed changes

derek bot added this to the 0.8.6 milestone Jul 13, 2018

alexellis force-pushed the auto-auth branch from f711d40 to ee3750f Compare July 13, 2018 14:19

rgee0 suggested changes Jul 16, 2018

View reviewed changes

alexellis commented Jul 16, 2018

View reviewed changes

alexellis added 2 commits July 16, 2018 12:36

Reinstate basic_auth flag

642ba90

- basic_auth is now set to true by default unless overriden in the env-var "BASIC_AUTH" Signed-off-by: Alex Ellis (VMware) <[email protected]>

Bind secrets to stack

95ea2e1

Secrets for gateway needed to be bound to stack, but were commented-out. Tested on Swarm. Signed-off-by: Alex Ellis (VMware) <[email protected]>

Bump gateway version

2b5492a

Signed-off-by: Alex Ellis (VMware) <[email protected]>

alexellis merged commit 8ede896 into master Jul 16, 2018

alexellis deleted the auto-auth branch August 14, 2018 08:34

Enable basic-auth by default #743

Enable basic-auth by default #743

Conversation

alexellis commented Jul 3, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

martindekov commented Jul 3, 2018

alexellis commented Jul 3, 2018

martindekov commented Jul 3, 2018

alexellis commented Jul 3, 2018

viveksyngh commented Jul 4, 2018 • edited Loading

johnmccabe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Jul 5, 2018

viveksyngh commented Jul 5, 2018

viveksyngh commented Jul 5, 2018

viveksyngh commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Jul 13, 2018

alexellis commented Jul 13, 2018

alexellis commented Jul 13, 2018

martindekov commented Jul 13, 2018

alexellis commented Jul 14, 2018

rgee0 commented Jul 15, 2018

viveksyngh commented Jul 15, 2018

rgee0 commented Jul 15, 2018

alexellis commented Jul 15, 2018

viveksyngh commented Jul 15, 2018 • edited Loading

rgee0 commented Jul 15, 2018 • edited Loading

viveksyngh commented Jul 16, 2018 • edited Loading

rgee0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexellis commented Jul 16, 2018

alexellis commented Jul 16, 2018

rgee0 commented Jul 16, 2018

alexellis commented Jul 3, 2018 •

edited

Loading

viveksyngh commented Jul 4, 2018 •

edited

Loading

viveksyngh commented Jul 15, 2018 •

edited

Loading

rgee0 commented Jul 15, 2018 •

edited

Loading

viveksyngh commented Jul 16, 2018 •

edited

Loading