Merge pull request juju#17606 from kian99/fix-federated-auth-with-deb…

…ug-logs

juju#17606

This PR fixes an issue that prevented CLI users interacting with JAAS from running `juju debug-log`.

When running `juju debug-log` against a model that is hosted on a controller connected to JAAS, the operation hangs and nothing is returned. This is because JAAS proxies all client requests and did not implement the models `/log` endpoint. It also however has no way of authenticating the user.

Running `juju debug-log` does the following:
1. Connects to the Juju controller at `/model/<uuid>/api` creating an API client in the normal fashion, establishing a websocket connection and performing a login request.
2. It then goes on to establish a new separate connection to the `/model/<uuid>/log` endpoint, eventually establishing a websocket but instead of sending an RPC login request, it uses the apiClient's `ConnectStream` method where an HTTP basic auth header is sent that includes the user's password and includes the macaroons as cookies.
3. Proceeds to read from the websocket to stream logs.

With JAAS' introduction of login providers and particularly the session login provider, it was realised that there is no mechanism in place to send the session token used with OIDC login in the basic auth header used in the `ConnectStream` method. Because JAAS acts as a proxy layer for all controller and model level calls, JAAS has no way of authenticating requests to `/model/<uuid>/log`.

To fix this I have made the following changes in this PR.
1. ~~Add a `Token` method to the LoginProvider interface that returns a string. The string represents the token used for login.~~ After some discussion, this was changed to shift more logic into the login provider. Instead of returning a token that was then used as part of an HTTP header, the login provider interface now has an `AuthHeader` method that will return an HTTP header that can be used in places like the stream connector.
2. Added a `loginProvider` field to the `state` struct to keep the provider around, depending on how this is implemented, it's necessary to hold onto the provider because certain providers like the sessionTokenLoginProvider only obtain a login token after completing login.
3. I've replaced the logic in the `connectStream` method to use the login providers token rather than just the password from the state.

A benefit of this is that it further reduces the use of `state.password` ~~which could probably be removed with some further refactoring.~~ and now it completely removes the need for storing `tag`, `password`, `nonce`, and `macaroon` on the state struct since they are nicely contained within the login provider. 

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
- [ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing
- [ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages

## QA steps

- `make install`
- `juju bootstrap localhost test`
- `juju add-model test`
- `juju debug-log`

api/apiclient.go

@@ -20,7 +20,6 @@ import (
 	"sync/atomic"
 	"time"
 
-	"github.com/go-macaroon-bakery/macaroon-bakery/v3/bakery"
 	"github.com/go-macaroon-bakery/macaroon-bakery/v3/httpbakery"
 	"github.com/gorilla/websocket"
 	"github.com/juju/clock"
@@ -34,7 +33,6 @@ import (
 
 	"github.com/juju/juju/api/base"
 	"github.com/juju/juju/core/facades"
-	coremacaroon "github.com/juju/juju/core/macaroon"
 	"github.com/juju/juju/core/network"
 	jujuproxy "github.com/juju/juju/proxy"
 	"github.com/juju/juju/rpc"
@@ -162,7 +160,7 @@ func Open(info *Info, opts DialOpts) (Connection, error) {
 	// is refactored we fall back to using the user-pass login provider
 	// with information from Info.
 	if loginProvider == nil {
-		loginProvider = NewUserpassLoginProvider(info.Tag, info.Password, info.Nonce, info.Macaroons, bakeryClient, CookieURLFromHost(host))
+		loginProvider = NewLegacyLoginProvider(info.Tag, info.Password, info.Nonce, info.Macaroons, bakeryClient, CookieURLFromHost(host))
 	}
 
 	st := &state{
@@ -176,18 +174,14 @@ func Open(info *Info, opts DialOpts) (Connection, error) {
 		pingerFacadeVersion: pingerFacadeVersions[len(pingerFacadeVersions)-1],
 		serverScheme:        "https",
 		serverRootAddress:   dialResult.addr,
-		// We populate the username and password before
-		// login because, when doing HTTP requests, we'll want
-		// to use the same username and password for authenticating
-		// those. If login fails, we discard the connection.
-		tag:          tagToString(info.Tag),
-		password:     info.Password,
-		macaroons:    info.Macaroons,
-		nonce:        info.Nonce,
-		tlsConfig:    dialResult.tlsConfig,
-		bakeryClient: bakeryClient,
-		modelTag:     info.ModelTag,
-		proxier:      dialResult.proxier,
+		// We keep the login provider around to provide auth headers
+		// when doing HTTP requests.
+		// If login fails, we discard the connection.
+		loginProvider: loginProvider,
+		tlsConfig:     dialResult.tlsConfig,
+		bakeryClient:  bakeryClient,
+		modelTag:      info.ModelTag,
+		proxier:       dialResult.proxier,
 	}
 	if !info.SkipLogin {
 		if err := loginWithContext(dialCtx, st, loginProvider); err != nil {
@@ -366,23 +360,12 @@ func (st *state) connectStream(path string, attrs url.Values, extraHeaders http.
 		Proxy:           proxy.DefaultConfig.GetProxy,
 		TLSClientConfig: st.tlsConfig,
 	}
-	var requestHeader http.Header
-	if st.tag != "" {
-		requestHeader = jujuhttp.BasicAuthHeader(st.tag, st.password)
-	} else {
-		requestHeader = make(http.Header)
-	}
-	requestHeader.Set(params.JujuClientVersion, jujuversion.Current.String())
-	requestHeader.Set("Origin", "http://localhost/")
-	if st.nonce != "" {
-		requestHeader.Set(params.MachineNonceHeader, st.nonce)
-	}
-	// Add any cookies because they will not be sent to websocket
-	// connections by default.
-	err := st.addCookiesToHeader(requestHeader)
+	requestHeader, err := st.loginProvider.AuthHeader()
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
+	requestHeader.Set(params.JujuClientVersion, jujuversion.Current.String())
+	requestHeader.Set("Origin", "http://localhost/")
 	for header, values := range extraHeaders {
 		for _, value := range values {
 			requestHeader.Add(header, value)
@@ -430,39 +413,6 @@ func readInitialStreamError(ws base.Stream) error {
 	return nil
 }
 
-// addCookiesToHeader adds any cookies associated with the
-// API host to the given header. This is necessary because
-// otherwise cookies are not sent to websocket endpoints.
-func (st *state) addCookiesToHeader(h http.Header) error {
-	// net/http only allows adding cookies to a request,
-	// but when it sends a request to a non-http endpoint,
-	// it doesn't add the cookies, so make a request, starting
-	// with the given header, add the cookies to use, then
-	// throw away the request but keep the header.
-	req := &http.Request{
-		Header: h,
-	}
-	cookies := st.bakeryClient.Client.Jar.Cookies(st.cookieURL)
-	for _, c := range cookies {
-		req.AddCookie(c)
-	}
-	if len(cookies) == 0 && len(st.macaroons) > 0 {
-		// These macaroons must have been added directly rather than
-		// obtained from a request. Add them. (For example in the
-		// logtransfer connection for a migration.)
-		// See https://bugs.launchpad.net/juju/+bug/1650451
-		for _, macaroon := range st.macaroons {
-			cookie, err := httpbakery.NewCookie(coremacaroon.MacaroonNamespace, macaroon)
-			if err != nil {
-				return errors.Trace(err)
-			}
-			req.AddCookie(cookie)
-		}
-	}
-	h.Set(httpbakery.BakeryProtocolHeader, fmt.Sprint(bakery.LatestVersion))
-	return nil
-}
-
 // apiEndpoint returns a URL that refers to the given API slash-prefixed
 // endpoint path and query parameters.
 func (st *state) apiEndpoint(path, query string) (*url.URL, error) {
@@ -511,14 +461,6 @@ func apiPath(model, path string) (string, error) {
 	return modelRoot + model + path, nil
 }
 
-// tagToString returns the value of a tag's String method, or "" if the tag is nil.
-func tagToString(tag names.Tag) string {
-	if tag == nil {
-		return ""
-	}
-	return tag.String()
-}
-
 // dialResult holds a dialed connection, the URL
 // and TLS configuration used to connect to it.
 type dialResult struct {

api/clientcredentialsloginprovider.go

@@ -5,8 +5,10 @@ package api
 
 import (
 	"context"
+	"net/http"
 
 	"github.com/juju/errors"
+	jujuhttp "github.com/juju/http/v2"
 
 	"github.com/juju/juju/api/base"
 	"github.com/juju/juju/rpc/params"
@@ -32,6 +34,12 @@ type clientCredentialsLoginProvider struct {
 	clientSecret string
 }
 
+// AuthHeader implements the [LoginProvider.AuthHeader] method.
+// It returns an HTTP header with basic auth set.
+func (p *clientCredentialsLoginProvider) AuthHeader() (http.Header, error) {
+	return jujuhttp.BasicAuthHeader(p.clientID, p.clientSecret), nil
+}
+
 // Login implements the LoginProvider.Login method.
 //
 // It authenticates as the entity using client credentials.

api/clientcredentialsloginprovider_test.go

@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 
 	"github.com/juju/errors"
+	jujuhttp "github.com/juju/http/v2"
 	"github.com/juju/names/v5"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
@@ -15,15 +16,16 @@ import (
 	"github.com/juju/juju/api/base"
 	jujutesting "github.com/juju/juju/juju/testing"
 	"github.com/juju/juju/rpc/params"
+	coretesting "github.com/juju/juju/testing"
 )
 
-type clientCredentialsLoginProviderProviderSuite struct {
+type clientCredentialsLoginProviderSuite struct {
 	jujutesting.JujuConnSuite
 }
 
-var _ = gc.Suite(&clientCredentialsLoginProviderProviderSuite{})
+var _ = gc.Suite(&clientCredentialsLoginProviderSuite{})
 
-func (s *clientCredentialsLoginProviderProviderSuite) Test(c *gc.C) {
+func (s *clientCredentialsLoginProviderSuite) TestClientCredentialsLogin(c *gc.C) {
 	info := s.APIInfo(c)
 
 	clientID := "test-client-id"
@@ -66,14 +68,32 @@ func (s *clientCredentialsLoginProviderProviderSuite) Test(c *gc.C) {
 		return nil
 	})
 
+	lp := api.NewClientCredentialsLoginProvider(clientID, clientSecret)
 	apiState, err := api.Open(&api.Info{
 		Addrs:          info.Addrs,
 		ControllerUUID: info.ControllerUUID,
 		CACert:         info.CACert,
 	}, api.DialOpts{
-		LoginProvider: api.NewClientCredentialsLoginProvider(clientID, clientSecret),
+		LoginProvider: lp,
 	})
 	c.Assert(err, jc.ErrorIsNil)
-
 	defer func() { _ = apiState.Close() }()
+	c.Check(err, jc.ErrorIsNil)
+}
+
+// A separate suite for tests that don't need to communicate with a Juju controller.
+type clientCredentialsLoginProviderBasicSuite struct {
+	coretesting.BaseSuite
+}
+
+var _ = gc.Suite(&clientCredentialsLoginProviderBasicSuite{})
+
+func (s *clientCredentialsLoginProviderBasicSuite) TestClientCredentialsAuthHeader(c *gc.C) {
+	clientID := "test-client-id"
+	clientSecret := "test-client-secret"
+	lp := api.NewClientCredentialsLoginProvider(clientID, clientSecret)
+	expectedHeader := jujuhttp.BasicAuthHeader(clientID, clientSecret)
+	got, err := lp.AuthHeader()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Check(got, jc.DeepEquals, expectedHeader)
 }

api/httpclient.go

@@ -5,9 +5,7 @@ package api
 
 import (
 	"bytes"
-	"encoding/base64"
 	"encoding/json"
-	"fmt"
 	"io"
 	"net/http"
 	"net/url"
@@ -16,7 +14,6 @@ import (
 	"github.com/go-macaroon-bakery/macaroon-bakery/v3/httpbakery"
 	"github.com/juju/errors"
 	"gopkg.in/httprequest.v1"
-	"gopkg.in/macaroon.v2"
 
 	"github.com/juju/juju/rpc/params"
 	jujuversion "github.com/juju/juju/version"
@@ -67,10 +64,7 @@ var _ httprequest.Doer = httpRequestDoer{}
 func (doer httpRequestDoer) Do(req *http.Request) (*http.Response, error) {
 	if err := authHTTPRequest(
 		req,
-		doer.st.tag,
-		doer.st.password,
-		doer.st.nonce,
-		doer.st.macaroons,
+		doer.st.loginProvider,
 	); err != nil {
 		return nil, errors.Trace(err)
 	}
@@ -89,44 +83,23 @@ func (doer httpRequestDoer) Do(req *http.Request) (*http.Response, error) {
 // AuthHTTPRequest adds Juju auth info (username, password, nonce, macaroons)
 // to the given HTTP request, suitable for sending to a Juju API server.
 func AuthHTTPRequest(req *http.Request, info *Info) error {
-	var tag string
-	if info.Tag != nil {
-		tag = info.Tag.String()
-	}
-	return authHTTPRequest(req, tag, info.Password, info.Nonce, info.Macaroons)
+	lp := NewLegacyLoginProvider(info.Tag, info.Password, info.Nonce, info.Macaroons, nil, nil)
+	return authHTTPRequest(req, lp)
 }
 
-func authHTTPRequest(req *http.Request, tag, password, nonce string, macaroons []macaroon.Slice) error {
-	if tag != "" {
-		// Note that password may be empty here; we still
-		// want to pass the tag along. An empty password
-		// indicates that we're using macaroon authentication.
-		req.SetBasicAuth(tag, password)
+func authHTTPRequest(req *http.Request, lp LoginProvider) error {
+	header, err := lp.AuthHeader()
+	if err != nil {
+		return errors.Trace(err)
 	}
-	if nonce != "" {
-		req.Header.Set(params.MachineNonceHeader, nonce)
+	// Copy headers to the request, using the first available value for each key.
+	for key := range header {
+		req.Header.Set(key, header.Get(key))
 	}
 	req.Header.Set(params.JujuClientVersion, jujuversion.Current.String())
-	req.Header.Set(httpbakery.BakeryProtocolHeader, fmt.Sprint(bakery.LatestVersion))
-	for _, ms := range macaroons {
-		encoded, err := encodeMacaroonSlice(ms)
-		if err != nil {
-			return errors.Trace(err)
-		}
-		req.Header.Add(httpbakery.MacaroonsHeader, encoded)
-	}
 	return nil
 }
 
-// encodeMacaroonSlice base64-JSON-encodes a slice of macaroons.
-func encodeMacaroonSlice(ms macaroon.Slice) (string, error) {
-	data, err := json.Marshal(ms)
-	if err != nil {
-		return "", errors.Trace(err)
-	}
-	return base64.StdEncoding.EncodeToString(data), nil
-}
-
 func isJSONMediaType(header http.Header) bool {
 	return header.Get("Content-Type") == "application/json"
 }

api/interface.go

@@ -8,6 +8,7 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"net"
+	"net/http"
 	"net/url"
 	"time"
 
@@ -29,6 +30,11 @@ import (
 // AnonymousUsername is the special username to use for anonymous logins.
 const AnonymousUsername = "jujuanonymous"
 
+const (
+	// ErrorLoginFirst indicates that login has not taken place yet.
+	ErrorLoginFirst = errors.ConstError("login provider needs to be logged in")
+)
+
 // Info encapsulates information about a server holding juju state and
 // can be used to make a connection to it.
 type Info struct {
@@ -185,6 +191,13 @@ func NewLoginResultParams(result params.LoginResult) (*LoginResultParams, error)
 type LoginProvider interface {
 	// Login performs log in when connecting to the controller.
 	Login(ctx context.Context, caller base.APICaller) (*LoginResultParams, error)
+	// AuthHeader returns an HTTP header used for authentication.
+	// This is normally used as part of basic authentication in scenarios where a client
+	// makes use of a StreamConnector like when fetching logs using `juju debug-log`.
+	// Can return [ErrorLoginFirst] when the provider requires an RPC login before basic auth
+	// can be performed.
+	// Other errors are also possible indicating an internal error in the provider.
+	AuthHeader() (http.Header, error)
 }
 
 // DialOpts holds configuration parameters that control the