[lxc-devel] [lxd/master] lxd: Cleanup authentication code

stgraber on Github lxc-bot at linuxcontainers.org
Fri Mar 1 14:00:36 UTC 2019


A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 354 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20190301/5d91bb39/attachment.bin>
-------------- next part --------------
From 083722f5b52b9f9df932557d981997c0b114ce19 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Fri, 1 Mar 2019 14:54:29 +0100
Subject: [PATCH] lxd: Cleanup authentication code
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 lxd/api_1.0.go      |  3 +-
 lxd/certificates.go |  3 +-
 lxd/daemon.go       | 99 +++++++++++++++++++++++++++++----------------
 lxd/images.go       | 19 ++++-----
 4 files changed, 74 insertions(+), 50 deletions(-)

diff --git a/lxd/api_1.0.go b/lxd/api_1.0.go
index a00b25a3fe..eb33c60fc5 100644
--- a/lxd/api_1.0.go
+++ b/lxd/api_1.0.go
@@ -110,8 +110,7 @@ func api10Get(d *Daemon, r *http.Request) Response {
 	}
 
 	// If untrusted, return now
-	trusted, _ := d.checkTrustedClient(r)
-	if trusted != nil {
+	if d.checkTrustedClient(r) != nil {
 		return SyncResponseETag(true, srv, nil)
 	}
 
diff --git a/lxd/certificates.go b/lxd/certificates.go
index ddd64f1f43..2e4e190ef3 100644
--- a/lxd/certificates.go
+++ b/lxd/certificates.go
@@ -124,8 +124,7 @@ func certificatesPost(d *Daemon, r *http.Request) Response {
 		return SmartError(err)
 	}
 
-	trusted, _ := d.checkTrustedClient(r)
-	if trusted != nil && util.PasswordCheck(secret, req.Password) != nil {
+	if d.checkTrustedClient(r) != nil && util.PasswordCheck(secret, req.Password) != nil {
 		logger.Warn("Bad trust password", log.Ctx{"url": r.URL.RequestURI(), "ip": r.RemoteAddr})
 		return Forbidden(nil)
 	}
diff --git a/lxd/daemon.go b/lxd/daemon.go
index c3f1e39b24..100bd2942f 100644
--- a/lxd/daemon.go
+++ b/lxd/daemon.go
@@ -156,42 +156,60 @@ type Command struct {
 	patch         func(d *Daemon, r *http.Request) Response
 }
 
-// Check whether the request comes from a trusted client.
-func (d *Daemon) checkTrustedClient(r *http.Request) (error, string) {
-	// Check the cluster certificate first, so we return an error if the
-	// notification header is set but the client is not presenting the
-	// cluster certificate (iow this request does not appear to come from a
-	// cluster node).
-	cert, _ := x509.ParseCertificate(d.endpoints.NetworkCert().KeyPair().Certificate[0])
-	clusterCerts := map[string]x509.Certificate{"0": *cert}
+// Convenience function around Authenticate
+func (d *Daemon) checkTrustedClient(r *http.Request) error {
+	trusted, _, err := d.Authenticate(r)
+	if !trusted || err != nil {
+		if err != nil {
+			return err
+		}
+
+		return fmt.Errorf("Not authorized")
+	}
+
+	return nil
+}
+
+// Authenticate validates an incoming http Request
+// It will check over what protocol it came, what type of request it is and
+// will validate the TLS certificate or Macaroon.
+//
+// This does not perform authorization, only validates authentication
+func (d *Daemon) Authenticate(r *http.Request) (bool, string, error) {
+	// Allow internal cluster traffic
 	if r.TLS != nil {
+		cert, _ := x509.ParseCertificate(d.endpoints.NetworkCert().KeyPair().Certificate[0])
+		clusterCerts := map[string]x509.Certificate{"0": *cert}
 		for i := range r.TLS.PeerCertificates {
 			trusted, _ := util.CheckTrustState(*r.TLS.PeerCertificates[i], clusterCerts)
 			if trusted {
-				return nil, ""
+				return true, "", nil
 			}
 		}
 	}
 
-	if isClusterNotification(r) {
-		return fmt.Errorf("cluster notification not using cluster certificate"), ""
-	}
-
+	// Local unix socket queries
 	if r.RemoteAddr == "@" {
-		// Unix socket
-		return nil, ""
+		return true, "", nil
 	}
 
+	// Devlxd unix socket credentials on main API
 	if r.RemoteAddr == "@devlxd" {
-		// Devlxd unix socket
-		return fmt.Errorf("devlxd query"), ""
+		return false, "", fmt.Errorf("Main API query can't come from /dev/lxd socket")
 	}
 
+	// Cluster notification with wrong certificate
+	if isClusterNotification(r) {
+		return false, "", fmt.Errorf("Cluster notification isn't using cluster certificate")
+	}
+
+	// Bad query, no TLS found
 	if r.TLS == nil {
-		return fmt.Errorf("no TLS"), ""
+		return false, "", fmt.Errorf("Bad/missing TLS on network query")
 	}
 
 	if d.externalAuth != nil && r.Header.Get(httpbakery.BakeryProtocolHeader) != "" {
+		// Validate external authentication
 		ctx := httpbakery.ContextWithRequest(context.TODO(), r)
 		authChecker := d.externalAuth.bakery.Checker.Auth(httpbakery.RequestMacaroons(r)...)
 
@@ -202,24 +220,29 @@ func (d *Daemon) checkTrustedClient(r *http.Request) (error, string) {
 
 		info, err := authChecker.Allow(ctx, ops...)
 		if err != nil {
-			return err, ""
+			// Bad macaroon
+			return false, "", err
 		}
 
 		if info != nil && info.Identity != nil {
-			return nil, info.Identity.Id()
+			// Valid identity macaroon found
+			return true, info.Identity.Id(), nil
 		}
 
-		return nil, ""
+		// Valid macaroon with no identity information
+		return true, "", nil
 	}
 
+	// Validate normal TLS access
 	for i := range r.TLS.PeerCertificates {
 		trusted, username := util.CheckTrustState(*r.TLS.PeerCertificates[i], d.clientCerts)
 		if trusted {
-			return nil, username
+			return true, username, nil
 		}
 	}
 
-	return fmt.Errorf("unauthorized"), ""
+	// Reject unauthorized
+	return false, "", nil
 }
 
 func writeMacaroonsRequiredResponse(b *identchecker.Bakery, r *http.Request, w http.ResponseWriter, derr *bakery.DischargeRequiredError, expiry int64) {
@@ -295,29 +318,33 @@ func (d *Daemon) createCmd(restAPI *mux.Router, version string, c Command) {
 			return
 		}
 
-		untrustedOk := (r.Method == "GET" && c.untrustedGet) || (r.Method == "POST" && c.untrustedPost)
-		err, username := d.checkTrustedClient(r)
-		if err == nil {
-			logger.Debug(
-				"handling",
-				log.Ctx{"method": r.Method, "url": r.URL.RequestURI(), "ip": r.RemoteAddr})
+		// Authentication
+		trusted, username, err := d.Authenticate(r)
+		if err != nil {
+			// If not a macaroon discharge request, return the error
+			_, ok := err.(*bakery.DischargeRequiredError)
+			if !ok {
+				InternalError(err).Render(w)
+				return
+			}
+		}
 
+		untrustedOk := (r.Method == "GET" && c.untrustedGet) || (r.Method == "POST" && c.untrustedPost)
+		if trusted {
+			logger.Debug("Handling", log.Ctx{"method": r.Method, "url": r.URL.RequestURI(), "ip": r.RemoteAddr, "user": username})
 			r = r.WithContext(context.WithValue(r.Context(), "username", username))
 		} else if untrustedOk && r.Header.Get("X-LXD-authenticated") == "" {
-			logger.Debug(
-				fmt.Sprintf("allowing untrusted %s", r.Method),
-				log.Ctx{"url": r.URL.RequestURI(), "ip": r.RemoteAddr})
+			logger.Debug(fmt.Sprintf("Allowing untrusted %s", r.Method), log.Ctx{"url": r.URL.RequestURI(), "ip": r.RemoteAddr})
 		} else if derr, ok := err.(*bakery.DischargeRequiredError); ok {
 			writeMacaroonsRequiredResponse(d.externalAuth.bakery, r, w, derr, d.externalAuth.expiry)
 			return
 		} else {
-			logger.Warn(
-				"rejecting request from untrusted client",
-				log.Ctx{"ip": r.RemoteAddr})
+			logger.Warn("Rejecting request from untrusted client", log.Ctx{"ip": r.RemoteAddr})
 			Forbidden(nil).Render(w)
 			return
 		}
 
+		// Dump full request JSON when in debug mode
 		if debug && r.Method != "GET" && isJSONRequest(r) {
 			newBody := &bytes.Buffer{}
 			captured := &bytes.Buffer{}
@@ -331,6 +358,7 @@ func (d *Daemon) createCmd(restAPI *mux.Router, version string, c Command) {
 			shared.DebugJson(captured)
 		}
 
+		// Actually process the request
 		var resp Response
 		resp = NotImplemented(nil)
 
@@ -359,6 +387,7 @@ func (d *Daemon) createCmd(restAPI *mux.Router, version string, c Command) {
 			resp = NotFound(fmt.Errorf("Method '%s' not found", r.Method))
 		}
 
+		// Handle errors
 		if err := resp.Render(w); err != nil {
 			err := InternalError(err).Render(w)
 			if err != nil {
diff --git a/lxd/images.go b/lxd/images.go
index ce18c006ed..c5a2a8b7ad 100644
--- a/lxd/images.go
+++ b/lxd/images.go
@@ -881,9 +881,9 @@ func doImagesGet(d *Daemon, recursion bool, project string, public bool) (interf
 
 func imagesGet(d *Daemon, r *http.Request) Response {
 	project := projectParam(r)
-	trusted, _ := d.checkTrustedClient(r)
+	public := d.checkTrustedClient(r) != nil
 
-	result, err := doImagesGet(d, util.IsRecursionRequest(r), project, trusted != nil)
+	result, err := doImagesGet(d, util.IsRecursionRequest(r), project, public)
 	if err != nil {
 		return SmartError(err)
 	}
@@ -1484,7 +1484,7 @@ func imageValidSecret(fingerprint string, secret string) bool {
 func imageGet(d *Daemon, r *http.Request) Response {
 	project := projectParam(r)
 	fingerprint := mux.Vars(r)["fingerprint"]
-	trusted, _ := d.checkTrustedClient(r)
+	public := d.checkTrustedClient(r) != nil
 	secret := r.FormValue("secret")
 
 	info, response := doImageGet(d.cluster, project, fingerprint, false)
@@ -1492,7 +1492,7 @@ func imageGet(d *Daemon, r *http.Request) Response {
 		return response
 	}
 
-	if !info.Public && trusted != nil && !imageValidSecret(info.Fingerprint, secret) {
+	if !info.Public && public && !imageValidSecret(info.Fingerprint, secret) {
 		return NotFound(fmt.Errorf("Image '%s' not found", info.Fingerprint))
 	}
 
@@ -1646,9 +1646,7 @@ func aliasesGet(d *Daemon, r *http.Request) Response {
 			responseStr = append(responseStr, url)
 
 		} else {
-			trusted, _ := d.checkTrustedClient(r)
-
-			_, alias, err := d.cluster.ImageAliasGet(project, name, trusted == nil)
+			_, alias, err := d.cluster.ImageAliasGet(project, name, d.checkTrustedClient(r) == nil)
 			if err != nil {
 				continue
 			}
@@ -1667,8 +1665,7 @@ func aliasGet(d *Daemon, r *http.Request) Response {
 	project := projectParam(r)
 	name := mux.Vars(r)["name"]
 
-	trusted, _ := d.checkTrustedClient(r)
-	_, alias, err := d.cluster.ImageAliasGet(project, name, trusted == nil)
+	_, alias, err := d.cluster.ImageAliasGet(project, name, d.checkTrustedClient(r) == nil)
 	if err != nil {
 		return SmartError(err)
 	}
@@ -1814,7 +1811,7 @@ func imageExport(d *Daemon, r *http.Request) Response {
 	project := projectParam(r)
 	fingerprint := mux.Vars(r)["fingerprint"]
 
-	trusted, _ := d.checkTrustedClient(r)
+	public := d.checkTrustedClient(r) != nil
 	secret := r.FormValue("secret")
 
 	var imgInfo *api.Image
@@ -1835,7 +1832,7 @@ func imageExport(d *Daemon, r *http.Request) Response {
 			return SmartError(err)
 		}
 
-		if !imgInfo.Public && trusted != nil && !imageValidSecret(imgInfo.Fingerprint, secret) {
+		if !imgInfo.Public && public && !imageValidSecret(imgInfo.Fingerprint, secret) {
 			return NotFound(fmt.Errorf("Image '%s' not found", imgInfo.Fingerprint))
 		}
 	}


More information about the lxc-devel mailing list