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

Initial support for external authentication via Macaroons #3604

Merged
merged 10 commits into from Oct 18, 2017

Conversation

3 participants
@albertodonato
Contributor

albertodonato commented Jul 28, 2017

Add support for external authentication via macaroons (https://github.com/go-macaroon/macaroon)

@albertodonato albertodonato changed the title from [RFC] Add support for external authentication via Macaroons to [RFC] Support for external authentication via Macaroons Jul 28, 2017

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Aug 1, 2017

Member

Can you also move the po/ update to its own commit? that should make things a bit easier to review on Github by not having my browser get stuck on the massive po diff :)

Member

stgraber commented Aug 1, 2017

Can you also move the po/ update to its own commit? that should make things a bit easier to review on Github by not having my browser get stuck on the massive po diff :)

@albertodonato

This comment has been minimized.

Show comment
Hide comment
@albertodonato
Contributor

albertodonato commented Aug 2, 2017

@stgraber done

@stgraber stgraber changed the title from [RFC] Support for external authentication via Macaroons to Support for external authentication via Macaroons Oct 5, 2017

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Oct 6, 2017

Member

What's going on with all the sign-offs on the first commit?

Member

stgraber commented Oct 6, 2017

What's going on with all the sign-offs on the first commit?

Show outdated Hide outdated doc/server.md
Show outdated Hide outdated lxd/daemon.go
Show outdated Hide outdated lxd/daemon.go
Show outdated Hide outdated lxd/daemon.go
Show outdated Hide outdated client/lxd.go
Show outdated Hide outdated client/lxd.go
Show outdated Hide outdated client/lxd.go
Show outdated Hide outdated client/lxd.go
@@ -225,7 +225,7 @@ func ensureImage(c lxd.ContainerServer, image string) (string, error) {
var fingerprint string
if strings.Contains(image, ":") {
defaultConfig := config.DefaultConfig
defaultConfig := config.NewConfig("", true)

This comment has been minimized.

@stgraber

stgraber Oct 6, 2017

Member

Does that mean that config.DefaultConfig is no longer going to be working?

If so, that's an API regression in the client library and therefore not acceptable. If NewConfig() is needed to get the new feature but backward compatibility is kept with DefaultConfig, then that's fine.

@stgraber

stgraber Oct 6, 2017

Member

Does that mean that config.DefaultConfig is no longer going to be working?

If so, that's an API regression in the client library and therefore not acceptable. If NewConfig() is needed to get the new feature but backward compatibility is kept with DefaultConfig, then that's fine.

This comment has been minimized.

@albertodonato

albertodonato Oct 6, 2017

Contributor

This should be ok. The reason for NewConfig() is to add the cookiejar, which is only needed if you enable macaroon auth.

Ideally, though, we should deprecate use of DefaultConfig and always go through NewConfig(), in case we need to add logic.

@albertodonato

albertodonato Oct 6, 2017

Contributor

This should be ok. The reason for NewConfig() is to add the cookiejar, which is only needed if you enable macaroon auth.

Ideally, though, we should deprecate use of DefaultConfig and always go through NewConfig(), in case we need to add logic.

Show outdated Hide outdated test/godeps.list
@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Oct 6, 2017

Member

I'm going to need the client commit to be split into:

  • A commit for the stuff under client/
  • A commit for the stuff under lxc/config/
  • A commit for the rest of the lxc/ stuff and the change to the tests
  • A commit for the lxd-benchmark change
Member

stgraber commented Oct 6, 2017

I'm going to need the client commit to be split into:

  • A commit for the stuff under client/
  • A commit for the stuff under lxc/config/
  • A commit for the rest of the lxc/ stuff and the change to the tests
  • A commit for the lxd-benchmark change

@stgraber stgraber added the Incomplete label Oct 6, 2017

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Oct 6, 2017

Member

We'll need this branch to update doc/security.md with a description of the macaroon based auth stuff.

Member

stgraber commented Oct 6, 2017

We'll need this branch to update doc/security.md with a description of the macaroon based auth stuff.

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Oct 6, 2017

Member

Also going to need to get tests in place before we can merge this.

Member

stgraber commented Oct 6, 2017

Also going to need to get tests in place before we can merge this.

@albertodonato

This comment has been minimized.

Show comment
Hide comment
@albertodonato

albertodonato Oct 6, 2017

Contributor

@stgraber updated commits and added doc

Contributor

albertodonato commented Oct 6, 2017

@stgraber updated commits and added doc

@albertodonato

This comment has been minimized.

Show comment
Hide comment
@albertodonato

albertodonato Oct 6, 2017

Contributor

@stgraber also, added the test auth server and a test using external auth

Contributor

albertodonato commented Oct 6, 2017

@stgraber also, added the test auth server and a test using external auth

@albertodonato

This comment has been minimized.

Show comment
Hide comment
@albertodonato

albertodonato Oct 10, 2017

Contributor

I can't figure out what's failing in tests on jenkins.
They pass locally and I don't see any obvious error in the log

Contributor

albertodonato commented Oct 10, 2017

I can't figure out what's failing in tests on jenkins.
They pass locally and I don't see any obvious error in the log

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Oct 10, 2017

Member

Problem seems to be a rather big diff in godeps output

Member

stgraber commented Oct 10, 2017

Problem seems to be a rather big diff in godeps output

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Oct 10, 2017

Member
==> TEST BEGIN: test_static_analysis
2017/10/10 22:27:24 auth - running at http://127.0.0.1:19114
ERROR: you added a new dependency to the client or shared; please make sure this is what you want
--- test/godeps.list	2017-10-10 22:19:12.722359971 +0000
+++ -	2017-10-10 22:27:33.316777216 +0000
@@ -1,20 +1,16 @@
 github.com/golang/protobuf
 github.com/gorilla/websocket
-github.com/juju/errors
 github.com/juju/go4
 github.com/juju/httprequest
 github.com/juju/loggo
 github.com/juju/persistent-cookiejar
-github.com/juju/schema
-github.com/juju/utils
 github.com/juju/webbrowser
 github.com/julienschmidt/httprouter
+github.com/lxc/lxd
 github.com/rogpeppe/fastuuid
 golang.org/x/crypto
 golang.org/x/net
-golang.org/x/sys
 gopkg.in/errgo.v1
-gopkg.in/juju/environschema.v1
 gopkg.in/macaroon-bakery.v2-unstable
 gopkg.in/macaroon.v2-unstable
 gopkg.in/retry.v1
Member

stgraber commented Oct 10, 2017

==> TEST BEGIN: test_static_analysis
2017/10/10 22:27:24 auth - running at http://127.0.0.1:19114
ERROR: you added a new dependency to the client or shared; please make sure this is what you want
--- test/godeps.list	2017-10-10 22:19:12.722359971 +0000
+++ -	2017-10-10 22:27:33.316777216 +0000
@@ -1,20 +1,16 @@
 github.com/golang/protobuf
 github.com/gorilla/websocket
-github.com/juju/errors
 github.com/juju/go4
 github.com/juju/httprequest
 github.com/juju/loggo
 github.com/juju/persistent-cookiejar
-github.com/juju/schema
-github.com/juju/utils
 github.com/juju/webbrowser
 github.com/julienschmidt/httprouter
+github.com/lxc/lxd
 github.com/rogpeppe/fastuuid
 golang.org/x/crypto
 golang.org/x/net
-golang.org/x/sys
 gopkg.in/errgo.v1
-gopkg.in/juju/environschema.v1
 gopkg.in/macaroon-bakery.v2-unstable
 gopkg.in/macaroon.v2-unstable
 gopkg.in/retry.v1
@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Oct 10, 2017

Member

This is masked due to your change in that particular part of the test. To get it visible again, you'll need:

diff --git a/test/suites/static_analysis.sh b/test/suites/static_analysis.sh
index 0cba3df7..e0f918cb 100644
--- a/test/suites/static_analysis.sh
+++ b/test/suites/static_analysis.sh
@@ -67,7 +67,7 @@ test_static_analysis() {
     fi
 
     if which godeps >/dev/null 2>&1; then
-      OUT=$(godeps -T ./client ./lxc/config ./shared/api 2>/dev/null | cut -f1 | diff -u test/godeps.list -)
+      OUT=$(godeps -T ./client ./lxc/config ./shared/api 2>/dev/null | cut -f1 | diff -u test/godeps.list - || true)
       if [ -n "${OUT}" ]; then
         echo "ERROR: you added a new dependency to the client or shared; please make sure this is what you want"
         echo "${OUT}"
Member

stgraber commented Oct 10, 2017

This is masked due to your change in that particular part of the test. To get it visible again, you'll need:

diff --git a/test/suites/static_analysis.sh b/test/suites/static_analysis.sh
index 0cba3df7..e0f918cb 100644
--- a/test/suites/static_analysis.sh
+++ b/test/suites/static_analysis.sh
@@ -67,7 +67,7 @@ test_static_analysis() {
     fi
 
     if which godeps >/dev/null 2>&1; then
-      OUT=$(godeps -T ./client ./lxc/config ./shared/api 2>/dev/null | cut -f1 | diff -u test/godeps.list -)
+      OUT=$(godeps -T ./client ./lxc/config ./shared/api 2>/dev/null | cut -f1 | diff -u test/godeps.list - || true)
       if [ -n "${OUT}" ]; then
         echo "ERROR: you added a new dependency to the client or shared; please make sure this is what you want"
         echo "${OUT}"

@stgraber stgraber changed the title from Support for external authentication via Macaroons to Initial support for external authentication via Macaroons Oct 11, 2017

@stgraber stgraber removed the Incomplete label Oct 11, 2017

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Oct 11, 2017

Member

I'll have to take a closer look at the dependency chain we're getting for client/ and lxc/config/, see if we can make some clever use of interfaces to try to reduce it a bit. I don't care about it too much for LXD itself but I'd like to minimize the impact on downstream users of our client packages (keeping as much of the new dependencies limited to lxc/ and lxd/).

Member

stgraber commented Oct 11, 2017

I'll have to take a closer look at the dependency chain we're getting for client/ and lxc/config/, see if we can make some clever use of interfaces to try to reduce it a bit. I don't care about it too much for LXD itself but I'd like to minimize the impact on downstream users of our client packages (keeping as much of the new dependencies limited to lxc/ and lxd/).

Show outdated Hide outdated client/lxd.go
@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Oct 16, 2017

Member

Plan right now is to merge this on Wednesday after LXD 2.19 is out.

Member

stgraber commented Oct 16, 2017

Plan right now is to merge this on Wednesday after LXD 2.19 is out.

Show outdated Hide outdated doc/security.md
@@ -295,15 +295,19 @@ msgstr ""
msgid "ARCHITECTURE"
msgstr ""

This comment has been minimized.

@pmario

pmario Oct 17, 2017

IMO many elements missing and some translations could be improved. ... help wanted?

@pmario

pmario Oct 17, 2017

IMO many elements missing and some translations could be improved. ... help wanted?

This comment has been minimized.

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Oct 17, 2017

Member
# github.com/lxc/lxd/lxd
lxd/daemon.go:178: too many arguments in call to httpbakery.NewDischargeRequiredError
	have (*bakery.Macaroon, string, *bakery.DischargeRequiredError, *http.Request)
	want (httpbakery.DischargeRequiredErrorParams)
Member

stgraber commented Oct 17, 2017

# github.com/lxc/lxd/lxd
lxd/daemon.go:178: too many arguments in call to httpbakery.NewDischargeRequiredError
	have (*bakery.Macaroon, string, *bakery.DischargeRequiredError, *http.Request)
	want (httpbakery.DischargeRequiredErrorParams)

albertodonato added some commits Jul 19, 2017

server: support for macaroons-based authentication
Signed-off-by: Alberto Donato <alberto.donato@canonical.com>
client: rebuild i18n
Signed-off-by: Alberto Donato <alberto.donato@canonical.com>
add new dependencies
Signed-off-by: Alberto Donato <alberto.donato@canonical.com>
client: support for macaroons-based authentication
Signed-off-by: Alberto Donato <alberto.donato@canonical.com>

albertodonato added some commits Oct 6, 2017

config: add support for CookieJar
Signed-off-by: Alberto Donato <alberto.donato@canonical.com>
lxc: support for macaroons-based authentication
Signed-off-by: Alberto Donato <alberto.donato@canonical.com>
lxd-benchmark: use NewConfig to get a default configuration
Signed-off-by: Alberto Donato <alberto.donato@canonical.com>
doc: add macaroon login flow to security.md
Signed-off-by: Alberto Donato <alberto.donato@canonical.com>
tests: add tests for external authentication
Signed-off-by: Alberto Donato <alberto.donato@canonical.com>
client: make the authentication Interactor configurable
Signed-off-by: Alberto Donato <alberto.donato@canonical.com>

@stgraber stgraber merged commit ffcf961 into lxc:master Oct 18, 2017

3 of 5 checks passed

Testsuite Testsuite failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Branch target Branch target is correct
Details
Signed-off-by All commits signed-off
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@albertodonato albertodonato deleted the albertodonato:external-auth branch Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment