From 3ac7a6a4cc17b06d50afbd88191954056e7567e0 Mon Sep 17 00:00:00 2001 From: Sergey Kostrukov Date: Fri, 29 Apr 2022 11:34:05 -0700 Subject: [PATCH 1/3] Use MSAL for OBO token requests --- go.mod | 2 +- pkg/azuredx/azureauth/aad_client.go | 69 ++++++++++++++ pkg/azuredx/azureauth/auth.go | 94 +++---------------- pkg/azuredx/azureauth/auth_test.go | 136 +++++++++++++--------------- 4 files changed, 144 insertions(+), 157 deletions(-) create mode 100644 pkg/azuredx/azureauth/aad_client.go diff --git a/go.mod b/go.mod index 3fecc6dc..b6ccf272 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/grafana/azure-data-explorer-datasource go 1.17 require ( + github.com/AzureAD/microsoft-authentication-library-for-go v0.4.0 github.com/Azure/azure-sdk-for-go/sdk/azidentity v0.13.2 github.com/google/go-cmp v0.5.7 github.com/grafana/grafana-azure-sdk-go v1.1.0 @@ -17,7 +18,6 @@ require ( require ( github.com/Azure/azure-sdk-for-go/sdk/azcore v0.22.0 // indirect github.com/Azure/azure-sdk-for-go/sdk/internal v0.9.1 // indirect - github.com/AzureAD/microsoft-authentication-library-for-go v0.4.0 // indirect github.com/apache/arrow/go/arrow v0.0.0-20211112161151-bc219186db40 // indirect github.com/beorn7/perks v1.0.1 // indirect github.com/cespare/xxhash/v2 v2.1.2 // indirect diff --git a/pkg/azuredx/azureauth/aad_client.go b/pkg/azuredx/azureauth/aad_client.go new file mode 100644 index 00000000..3ceee690 --- /dev/null +++ b/pkg/azuredx/azureauth/aad_client.go @@ -0,0 +1,69 @@ +package azureauth + +import ( + "context" + "errors" + "fmt" + "net/http" + "regexp" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" + "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/AzureAD/microsoft-authentication-library-for-go/apps/confidential" + "github.com/grafana/grafana-azure-sdk-go/azcredentials" + "github.com/grafana/grafana-azure-sdk-go/azsettings" +) + +// Abstraction over confidential.Client from MSAL for Go +type aadClient interface { + AcquireTokenOnBehalfOf(ctx context.Context, userAssertion string, scopes []string) (confidential.AuthResult, error) +} + +func newAADClient(credentials *azcredentials.AzureClientSecretCredentials, httpClient *http.Client) (aadClient, error) { + authority, err := resolveAuthorityForCloud(credentials.AzureCloud) + if err != nil { + return nil, fmt.Errorf("invalid Azure credentials: %w", err) + } + + if !validTenantId(credentials.TenantId) { + return nil, errors.New("invalid tenantId") + } + + clientCredential, err := confidential.NewCredFromSecret(credentials.ClientSecret) + if err != nil { + return nil, err + } + + authorityOpts := confidential.WithAuthority(runtime.JoinPaths(string(authority), credentials.TenantId)) + httpClientOPts := confidential.WithHTTPClient(httpClient) + + client, err := confidential.New(credentials.ClientId, clientCredential, authorityOpts, httpClientOPts) + if err != nil { + return nil, err + } + + return client, nil +} + +func resolveAuthorityForCloud(cloudName string) (azidentity.AuthorityHost, error) { + // Known Azure clouds + switch cloudName { + case azsettings.AzurePublic: + return azidentity.AzurePublicCloud, nil + case azsettings.AzureChina: + return azidentity.AzureChina, nil + case azsettings.AzureUSGovernment: + return azidentity.AzureGovernment, nil + default: + err := fmt.Errorf("the Azure cloud '%s' not supported", cloudName) + return "", err + } +} + +func validTenantId(tenantId string) bool { + match, err := regexp.MatchString("^[0-9a-zA-Z-.]+$", tenantId) + if err != nil { + return false + } + return match +} diff --git a/pkg/azuredx/azureauth/auth.go b/pkg/azuredx/azureauth/auth.go index 8add2e51..0e73cdb6 100644 --- a/pkg/azuredx/azureauth/auth.go +++ b/pkg/azuredx/azureauth/auth.go @@ -2,17 +2,11 @@ package azureauth import ( "context" - "encoding/json" "errors" "fmt" - "io" "net/http" - "net/url" - "strconv" - "strings" "time" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/grafana/grafana-azure-sdk-go/azcredentials" "github.com/grafana/grafana-azure-sdk-go/azsettings" "github.com/grafana/grafana-azure-sdk-go/aztokenprovider" @@ -42,26 +36,19 @@ type ServiceCredentials interface { type ServiceCredentialsImpl struct { models.DatasourceSettings - // HTTPDo is the http.Client Do method. - HTTPDo func(req *http.Request) (*http.Response, error) - authority azidentity.AuthorityHost tokenProvider aztokenprovider.AzureTokenProvider tokenCache *cache + aadClient aadClient scopes []string } func NewServiceCredentials(settings *models.DatasourceSettings, azureSettings *azsettings.AzureSettings, - client *http.Client) (ServiceCredentials, error) { + httpClient *http.Client) (ServiceCredentials, error) { azureCloud, err := normalizeAzureCloud(settings.AzureCloud) if err != nil { return nil, fmt.Errorf("invalid Azure credentials: %w", err) } - authority, err := resolveAuthorityForCloud(azureCloud) - if err != nil { - return nil, fmt.Errorf("invalid Azure credentials: %w", err) - } - credentials := &azcredentials.AzureClientSecretCredentials{ AzureCloud: azureCloud, TenantId: settings.TenantID, @@ -74,6 +61,11 @@ func NewServiceCredentials(settings *models.DatasourceSettings, azureSettings *a return nil, fmt.Errorf("invalid Azure configuration: %w", err) } + aadClient, err := newAADClient(credentials, httpClient) + if err != nil { + return nil, fmt.Errorf("invalid Azure configuration: %w", err) + } + scopes, err := getAzureScopes(credentials, settings.ClusterURL) if err != nil { return nil, fmt.Errorf("invalid Azure configuration: %w", err) @@ -81,10 +73,9 @@ func NewServiceCredentials(settings *models.DatasourceSettings, azureSettings *a return &ServiceCredentialsImpl{ DatasourceSettings: *settings, - HTTPDo: client.Do, - authority: authority, tokenProvider: tokenProvider, tokenCache: newCache(), + aadClient: aadClient, scopes: scopes, }, nil } @@ -147,71 +138,10 @@ func (c *ServiceCredentialsImpl) queryDataOnBehalfOf(ctx context.Context, req *b return "Bearer " + onBehalfOfToken, nil } -// OnBehalfOf resolves a token which impersonates the subject of userToken. -// UserToken has to be an ID token. See the following link for more detail. -// https://docs.microsoft.com/en-us/azure/active-directory/develop/v2-oauth2-on-behalf-of-flow -func (c *ServiceCredentialsImpl) onBehalfOf(ctx context.Context, userToken string) (onBehalfOfToken string, expire time.Time, err error) { - params := make(url.Values) - params.Set("client_id", c.ClientID) - params.Set("client_secret", c.Secret) - params.Set("grant_type", "urn:ietf:params:oauth:grant-type:jwt-bearer") - params.Set("assertion", userToken) - params.Set("scope", strings.Join(c.scopes, " ")) - params.Set("requested_token_use", "on_behalf_of") - reqBody := strings.NewReader(params.Encode()) - - // https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-v2-protocols#endpoints - tokenURL := fmt.Sprintf("%s%s/oauth2/v2.0/token", c.authority, url.PathEscape(c.TenantID)) - req, err := http.NewRequestWithContext(ctx, "POST", tokenURL, reqBody) - if err != nil { - return "", time.Time{}, fmt.Errorf("on-behalf-of grant request <%q> instantiation: %w", tokenURL, err) - } - req.Header.Set("Content-Type", "application/x-www-form-urlencoded") - req.Header.Set("Accept", "application/json") - - // HTTP Exchange - reqStart := time.Now() - resp, err := c.HTTPDo(req) +func (c *ServiceCredentialsImpl) onBehalfOf(ctx context.Context, idToken string) (onBehalfOfToken string, expire time.Time, err error) { + result, err := c.aadClient.AcquireTokenOnBehalfOf(ctx, idToken, c.scopes) if err != nil { - return "", time.Time{}, fmt.Errorf("on-behalf-of grant POST <%q>: %w", tokenURL, err) - } - body, err := io.ReadAll(resp.Body) - if err != nil { - return "", time.Time{}, fmt.Errorf("on-behalf-of grant POST <%q> response: %w", tokenURL, err) - } - onBehalfOfLatencySeconds.WithLabelValues(strconv.Itoa(resp.StatusCode)).Observe(float64(time.Since(reqStart)) / float64(time.Second)) - if resp.StatusCode/100 != 2 { - var deny struct { - Desc string `json:"error_description"` - } - _ = json.Unmarshal(body, &deny) - return "", time.Time{}, fmt.Errorf("on-behalf-of grant POST <%q> status %q: %q", tokenURL, resp.Status, deny.Desc) - } - - // Parse Essentials - var grant struct { - Token string `json:"access_token"` - Expire int `json:"expires_in"` - } - if err := json.Unmarshal(body, &grant); err != nil { - return "", time.Time{}, fmt.Errorf("malformed response from on-behalf-of grant POST <%q>: %w", tokenURL, err) - } - - expire = time.Now().Add(time.Duration(grant.Expire) * time.Second) - return grant.Token, expire, nil -} - -func resolveAuthorityForCloud(cloudName string) (azidentity.AuthorityHost, error) { - // Known Azure clouds - switch cloudName { - case azsettings.AzurePublic: - return azidentity.AzurePublicCloud, nil - case azsettings.AzureChina: - return azidentity.AzureChina, nil - case azsettings.AzureUSGovernment: - return azidentity.AzureGovernment, nil - default: - err := fmt.Errorf("the Azure cloud '%s' not supported", cloudName) - return "", err + return "", time.Time{}, err } + return result.AccessToken, result.ExpiresOn.UTC(), nil } diff --git a/pkg/azuredx/azureauth/auth_test.go b/pkg/azuredx/azureauth/auth_test.go index 9d0be194..bdba3fd4 100644 --- a/pkg/azuredx/azureauth/auth_test.go +++ b/pkg/azuredx/azureauth/auth_test.go @@ -2,108 +2,96 @@ package azureauth import ( "context" - "errors" - "fmt" - "io" - "net/http" - "strings" "testing" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/AzureAD/microsoft-authentication-library-for-go/apps/confidential" "github.com/grafana/grafana-plugin-sdk-go/backend" ) -func TestQueryDataAuthorization(t *testing.T) { - // reusables - const testToken = "some.test.token" - wantNoHTTP := func(*http.Request) (*http.Response, error) { - return nil, errors.New("unwanted HTTP invocation") - } - +func TestOnBehalfOf(t *testing.T) { golden := []struct { - Want string - WantErr string + RequestUser *backend.User + RequestAuthorizationHeader string + RequestIDTokenHeader string - // HTTP client representation - HTTPDoMock func(*http.Request) (*http.Response, error) - - *backend.User - AuthHeader string - IDTokenHeader string + ShouldRequestToken bool + ExpectedError string }{ // happy flow - 0: {Want: "Bearer test.exchange.token", WantErr: "", - HTTPDoMock: func(req *http.Request) (*http.Response, error) { - const wantURL = "https://login.microsoftonline.com/0AF0528A-F473-4E0C-891F-3FF8ACDC4E5F/oauth2/v2.0/token" - if s := req.URL.String(); s != wantURL { - return nil, fmt.Errorf("got URL %q, want %q", s, wantURL) - } - - if err := req.ParseForm(); err != nil { - return nil, fmt.Errorf("test form values: %w", err) - } - if s := req.PostForm.Get("assertion"); s != testToken { - return nil, fmt.Errorf("POST with assertion pramameter %q, want %q", s, testToken) - } - - return &http.Response{ - StatusCode: 200, - Body: io.NopCloser(strings.NewReader(`{ - "access_token": "test.exchange.token" - }`)), - }, nil - }, - IDTokenHeader: testToken, - User: &backend.User{Login: "alice"}}, - - 1: {Want: "", WantErr: "non-user requests not permitted with on-behalf-of configuration", - HTTPDoMock: wantNoHTTP, - IDTokenHeader: testToken, - User: nil}, - - 2: {Want: "", WantErr: "system accounts are denied with on-behalf-of configuration", - HTTPDoMock: wantNoHTTP, - User: &backend.User{Login: "alice"}}, - - 3: {Want: "", WantErr: "ID token absent for data request", - HTTPDoMock: wantNoHTTP, - AuthHeader: "arbitrary", - User: &backend.User{Login: "alice"}}, + 0: { + RequestUser: &backend.User{Login: "alice"}, + RequestIDTokenHeader: "ID-TOKEN", + ShouldRequestToken: true, + ExpectedError: "", + }, + + 1: { + RequestUser: nil, + RequestIDTokenHeader: "ID-TOKEN", + ShouldRequestToken: false, + ExpectedError: "non-user requests not permitted with on-behalf-of configuration", + }, + + 2: { + RequestUser: &backend.User{Login: "alice"}, + ShouldRequestToken: false, + ExpectedError: "system accounts are denied with on-behalf-of configuration", + }, + + 3: { + RequestUser: &backend.User{Login: "alice"}, + RequestAuthorizationHeader: "arbitrary", + ShouldRequestToken: false, + ExpectedError: "ID token absent for data request", + }, } for index, g := range golden { // compose request var req backend.QueryDataRequest - req.PluginContext.User = g.User + req.PluginContext.User = g.RequestUser req.Headers = make(map[string]string) - if g.AuthHeader != "" { - req.Headers["Authorization"] = g.AuthHeader + if g.RequestAuthorizationHeader != "" { + req.Headers["Authorization"] = g.RequestAuthorizationHeader } - if g.IDTokenHeader != "" { - req.Headers["X-ID-Token"] = g.IDTokenHeader + if g.RequestIDTokenHeader != "" { + req.Headers["X-ID-Token"] = g.RequestIDTokenHeader } // setup & test - c := &ServiceCredentialsImpl{HTTPDo: g.HTTPDoMock} - c.TenantID = "0AF0528A-F473-4E0C-891F-3FF8ACDC4E5F" + fakeAADClient := &FakeAADClient{} + + c := &ServiceCredentialsImpl{ + tokenCache: newCache(), + aadClient: fakeAADClient, + } c.OnBehalfOf = true - c.authority = azidentity.AzurePublicCloud - c.tokenCache = newCache() + auth, err := c.QueryDataAuthorization(context.Background(), &req) + switch { case err != nil: switch { - case g.WantErr == "": + case g.ExpectedError == "": t.Errorf("%d: got error %q", index, err) - case err.Error() != g.WantErr: - t.Errorf("%d: got error %q, want %q", index, err, g.WantErr) + case err.Error() != g.ExpectedError: + t.Errorf("%d: got error %q, expected %q", index, err, g.ExpectedError) } - case g.WantErr != "": - t.Errorf("%d: got authorization %q, want error %q", index, auth, g.WantErr) + case g.ExpectedError != "": + t.Errorf("%d: got authorization %q, want error %q", index, auth, g.ExpectedError) - case auth != g.Want: - t.Errorf("%d: got authorization %q, want %q", index, auth, g.Want) + case g.ShouldRequestToken != fakeAADClient.TokenRequested: + t.Errorf("%d: should request token = %t, requested = %t", index, g.ShouldRequestToken, fakeAADClient.TokenRequested) } } } + +type FakeAADClient struct { + TokenRequested bool +} + +func (c *FakeAADClient) AcquireTokenOnBehalfOf(_ context.Context, _ string, _ []string) (confidential.AuthResult, error) { + c.TokenRequested = true + return confidential.AuthResult{}, nil +} From 16955f7e4d68fb24a812ad019046d55abda4a656 Mon Sep 17 00:00:00 2001 From: Sergey Kostrukov Date: Fri, 29 Apr 2022 11:56:51 -0700 Subject: [PATCH 2/3] Remove unused fields --- pkg/azuredx/azureauth/auth.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/azuredx/azureauth/auth.go b/pkg/azuredx/azureauth/auth.go index 0e73cdb6..5f71507c 100644 --- a/pkg/azuredx/azureauth/auth.go +++ b/pkg/azuredx/azureauth/auth.go @@ -35,7 +35,9 @@ type ServiceCredentials interface { } type ServiceCredentialsImpl struct { - models.DatasourceSettings + OnBehalfOf bool + QueryTimeout time.Duration + tokenProvider aztokenprovider.AzureTokenProvider tokenCache *cache aadClient aadClient @@ -72,11 +74,12 @@ func NewServiceCredentials(settings *models.DatasourceSettings, azureSettings *a } return &ServiceCredentialsImpl{ - DatasourceSettings: *settings, - tokenProvider: tokenProvider, - tokenCache: newCache(), - aadClient: aadClient, - scopes: scopes, + OnBehalfOf: settings.OnBehalfOf, + QueryTimeout: settings.QueryTimeout, + tokenProvider: tokenProvider, + tokenCache: newCache(), + aadClient: aadClient, + scopes: scopes, }, nil } From 25d05dba734033a9110398b88c9b27be8665a23b Mon Sep 17 00:00:00 2001 From: Sergey Kostrukov Date: Tue, 3 May 2022 10:06:07 -0700 Subject: [PATCH 3/3] Remove unused Prometheus metric --- CHANGELOG.md | 3 +++ pkg/azuredx/azureauth/aad_client.go | 4 ++-- pkg/azuredx/azureauth/auth.go | 16 +--------------- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e5e1119c..1818aa2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,10 @@ _Not released yet_ - Breaking Change: Azure Data Explorer plugin now requires Grafana 8.0+ to run. +- Breaking Change: obo_latency_seconds metric was removed - Bugfix: Filter dynamic columns from Where/Aggregate/Group by clauses to prevent syntax errors. +- Internal: Client secret authentication via Grafana Azure SDK +- Internal: OBO authentication via MSAL for Go ## [3.7.1] diff --git a/pkg/azuredx/azureauth/aad_client.go b/pkg/azuredx/azureauth/aad_client.go index 3ceee690..ebbc5be5 100644 --- a/pkg/azuredx/azureauth/aad_client.go +++ b/pkg/azuredx/azureauth/aad_client.go @@ -35,9 +35,9 @@ func newAADClient(credentials *azcredentials.AzureClientSecretCredentials, httpC } authorityOpts := confidential.WithAuthority(runtime.JoinPaths(string(authority), credentials.TenantId)) - httpClientOPts := confidential.WithHTTPClient(httpClient) + httpClientOpts := confidential.WithHTTPClient(httpClient) - client, err := confidential.New(credentials.ClientId, clientCredential, authorityOpts, httpClientOPts) + client, err := confidential.New(credentials.ClientId, clientCredential, authorityOpts, httpClientOpts) if err != nil { return nil, err } diff --git a/pkg/azuredx/azureauth/auth.go b/pkg/azuredx/azureauth/auth.go index 5f71507c..26d164e4 100644 --- a/pkg/azuredx/azureauth/auth.go +++ b/pkg/azuredx/azureauth/auth.go @@ -7,27 +7,13 @@ import ( "net/http" "time" + "github.com/grafana/azure-data-explorer-datasource/pkg/azuredx/models" "github.com/grafana/grafana-azure-sdk-go/azcredentials" "github.com/grafana/grafana-azure-sdk-go/azsettings" "github.com/grafana/grafana-azure-sdk-go/aztokenprovider" "github.com/grafana/grafana-plugin-sdk-go/backend" - "github.com/prometheus/client_golang/prometheus" - - "github.com/grafana/azure-data-explorer-datasource/pkg/azuredx/models" ) -var onBehalfOfLatencySeconds = prometheus.NewHistogramVec(prometheus.HistogramOpts{ - Namespace: "grafana", - Subsystem: "plugin_adx", - Name: "obo_latency_seconds", - Help: "On-behalf-of token exchange duration.", - Buckets: []float64{0.01, 0.05, 0.10, 0.20, 0.40, 1.00}, -}, []string{"http_status"}) - -func init() { - prometheus.MustRegister(onBehalfOfLatencySeconds) -} - // ServiceCredentials provides authorization for cloud service usage. type ServiceCredentials interface { ServicePrincipalAuthorization(ctx context.Context) (string, error)