Skip to content
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

add http-basic auth reading from a file #573

Merged
merged 5 commits into from
Dec 10, 2018
Merged

add http-basic auth reading from a file #573

merged 5 commits into from
Dec 10, 2018

Conversation

andyroyle
Copy link
Contributor

@andyroyle andyroyle commented Nov 16, 2018

Taking implementation cues from #166 I've made an implementation of http basic auth on a per-route basis.

The auth schemes need to be registered with fabio up-front using

proxy.auth = name=mybasicauth;type=basic;file=path/to/file

You then configure your route specifying auth=mybasicauth in the route options.

You can configure multiple auth schemes side-by-side

proxy.auth = name=mybasicauth;type=basic;file=path/to/file;realm=foo
                      name=myotherauth;type=basic;file=path/to/other/file

Currently it uses http basic auth, which reads a single htpasswd file at startup from a file on disk.

If you specify an auth-scheme that doesn't exist then it will log and error in fabio and return a 401 Unauthorized response.

You can optionally set the realm (default is to use the auth name parameter)

@aaronhurt
Copy link
Member

Thoughts of supporting htpasswd format for the file?

@andyroyle
Copy link
Contributor Author

Dead easy to add, do we want to make htpasswd the default for basic auth?

@aaronhurt
Copy link
Member

I would lean to making that default. The tooling to create htpasswd files is already present on most *nix systems and users of a load balancer are probably familiar with them and know how to create/manage those files. What do you think?

Sideline: Thanks for picking up the torch on that old issue.

@andyroyle
Copy link
Contributor Author

Htpasswd is pretty standard stuff. Will make the changes first thing Monday 👍

@aaronhurt
Copy link
Member

Awesome, and thank you again!

@magiconair
Copy link
Contributor

Thank you and yes to htpasswd.

@andyroyle
Copy link
Contributor Author

@magiconair / @leprechau htpasswd support added.

Also added support for an optional realm (used in the WWW-Authenticate: Basic realm="<realm>" response)

@andyroyle
Copy link
Contributor Author

So I vendored in a new library, which depends on golang.org/x/crypt but it seems to have brought with it updates to golang.org/x/net and golang.org/x/text, yet go.mod only shows a single change. Colour me confused.

@shantanugadgil
Copy link
Contributor

I stumbled upon this, and I wonder if it would solve my use case?
(Looking at the diff it seems so, based on the explanation, atleast)

My use case it to provide an "https + password" frontend to the Consul/Nomad GUI via Fabio.
Currently, I am making do with HAProxy http basic auth on a separate port.

Eagerly looking forward to this getting merged. 👏 👏

@andyroyle
Copy link
Contributor Author

@shantanugadgil this is exactly what we are using it for as well 😄

@aaronhurt
Copy link
Member

@andyroyle Sorry for the delay, just getting caught back up after a short vacation. I'll try to give this a review today.

Copy link
Contributor

@magiconair magiconair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor nitpicks. Otherwise LGTM. Thanks a lot!

auth/auth_test.go Outdated Show resolved Hide resolved
auth/auth_test.go Outdated Show resolved Hide resolved
auth/basic_test.go Outdated Show resolved Hide resolved
docs/content/feature/authorization.md Outdated Show resolved Hide resolved
@magiconair
Copy link
Contributor

@andyroyle can you rebase this, please? Then I'll merge it.

@magiconair magiconair added this to the 1.5.11 milestone Dec 7, 2018
@andyroyle
Copy link
Contributor Author

@magiconair thanks for the review (and for merging #575). I've fixed those issues and also rebased on top of master, so this should be good-to-go now.

@magiconair magiconair merged commit 5fb4039 into fabiolb:master Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants