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 apikey subcommand #3063

Closed
wants to merge 44 commits into from
Closed
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
f75f48d
Add apikey subcommand
jalvz Dec 5, 2019
3214f42
Fix some tests
jalvz Dec 16, 2019
1d7f204
Some fixes
jalvz Dec 18, 2019
2420c35
Revert all the config stuff
jalvz Dec 18, 2019
4fcbd9d
Add apikey subcommand
jalvz Dec 5, 2019
32bbcb6
Fix some tests
jalvz Dec 16, 2019
2aaa746
Some fixes
jalvz Dec 18, 2019
1c692eb
Revert all the config stuff
jalvz Dec 18, 2019
5defc03
key composite literals
graphaelli Dec 19, 2019
484763a
Merge branch 'master' into apikey-command
graphaelli Dec 19, 2019
82a0c2f
some linting
graphaelli Dec 19, 2019
58f51a1
Merge branch 'apikey-command' of github.com:jalvz/apm-server into api…
jalvz Dec 20, 2019
fbf9280
round of review comments
jalvz Dec 20, 2019
9e68751
require credentials
jalvz Dec 20, 2019
7843321
more code review comments
jalvz Dec 20, 2019
510414f
Merge remote-tracking branch 'elastic/master' into apikey-command
jalvz Jan 6, 2020
653bdd9
Add integration tests
jalvz Jan 8, 2020
1c222b0
return Cobra errors
jalvz Jan 8, 2020
87ab92b
A few more code review comments
jalvz Jan 9, 2020
302c562
Merge remote-tracking branch 'elastic/master' into apikey-command
jalvz Jan 9, 2020
caefc89
Add a few tests
jalvz Jan 9, 2020
b2bfde0
No help
jalvz Jan 9, 2020
e73a09b
More renames
jalvz Jan 9, 2020
d1123e0
query privileges before creating them, and fix issue intruducen in la…
jalvz Jan 9, 2020
11345d1
Merge remote-tracking branch 'elastic/master' into apikey-command
jalvz Jan 9, 2020
939b9ae
please the linter
jalvz Jan 10, 2020
fa827ed
fix unit test
jalvz Jan 10, 2020
5fa3a39
Merge remote-tracking branch 'elastic/master' into apikey-command
jalvz Jan 10, 2020
6f28f3c
Reset libbeat api key
jalvz Jan 10, 2020
0eff8dd
make update
jalvz Jan 10, 2020
790a4fe
undo golint refactor and update user in test assertion
jalvz Jan 10, 2020
f135da3
fix another golint refactor screwup
jalvz Jan 10, 2020
baa566f
another one
jalvz Jan 10, 2020
256b434
logic for checking any privileges action (*)
jalvz Jan 13, 2020
022ae2e
Add changelog
jalvz Jan 13, 2020
8fc8929
Merge remote-tracking branch 'elastic/master' into apikey-command
jalvz Jan 13, 2020
2d6001d
more golint && fmt
jalvz Jan 13, 2020
82285ca
a bit more code review
jalvz Jan 13, 2020
5c852da
Update beater/authorization/apikey.go
jalvz Jan 14, 2020
3da1348
Update cmd/apikey.go
jalvz Jan 14, 2020
a708f59
Update beater/authorization/apikey.go
jalvz Jan 14, 2020
cef8ef4
Update beater/authorization/apikey.go
jalvz Jan 14, 2020
983714f
fix more errors introduced lately
jalvz Jan 14, 2020
950ca80
Merge branch 'apikey-command' of github.com:jalvz/apm-server into api…
jalvz Jan 14, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions _meta/beat.yml
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ apm-server:

#protocol: "http"

# Username and password are only needed for the apm-server apikey sub-command, and they are ignored otherwise
# See `apm-server apikey --help` for details.
#username: "elastic"
#password: "changeme"
jalvz marked this conversation as resolved.
Show resolved Hide resolved

# Optional HTTP Path.
#path: ""

Expand Down
5 changes: 5 additions & 0 deletions apm-server.docker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ apm-server:

#protocol: "http"

# Username and password are only needed for the apm-server apikey sub-command, and they are ignored otherwise
# See `apm-server apikey --help` for details.
#username: "elastic"
#password: "changeme"

# Optional HTTP Path.
#path: ""

Expand Down
5 changes: 5 additions & 0 deletions apm-server.yml
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ apm-server:

#protocol: "http"

# Username and password are only needed for the apm-server apikey sub-command, and they are ignored otherwise
# See `apm-server apikey --help` for details.
#username: "elastic"
#password: "changeme"

# Optional HTTP Path.
#path: ""

Expand Down
12 changes: 6 additions & 6 deletions beater/api/mux.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func NewMux(beaterConfig *config.Config, report publish.Reporter) (*http.ServeMu
mux := http.NewServeMux()
logger := logp.NewLogger(logs.Handler)

auth, err := authorization.NewBuilder(beaterConfig)
auth, err := authorization.NewBuilder(*beaterConfig)
axw marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -125,7 +125,7 @@ func NewMux(beaterConfig *config.Config, report publish.Reporter) (*http.ServeMu

func profileHandler(cfg *config.Config, builder *authorization.Builder, reporter publish.Reporter) (request.Handler, error) {
h := profile.Handler(systemMetadataDecoder(cfg, emptyDecoder), transform.Config{}, reporter)
authHandler := builder.ForPrivilege(authorization.PrivilegeEventWrite)
authHandler := builder.ForPrivilege(authorization.PrivilegeEventWrite.Action)
return middleware.Wrap(h, backendMiddleware(cfg, authHandler, profile.MonitoringMap)...)
}

Expand All @@ -137,7 +137,7 @@ func backendIntakeHandler(cfg *config.Config, builder *authorization.Builder, re
MaxEventSize: cfg.MaxEventSize,
},
reporter)
authHandler := builder.ForPrivilege(authorization.PrivilegeEventWrite)
authHandler := builder.ForPrivilege(authorization.PrivilegeEventWrite.Action)
return middleware.Wrap(h, backendMiddleware(cfg, authHandler, intake.MonitoringMap)...)
}

Expand All @@ -162,12 +162,12 @@ func sourcemapHandler(cfg *config.Config, builder *authorization.Builder, report
return nil, err
}
h := sourcemap.Handler(systemMetadataDecoder(cfg, decoder.DecodeSourcemapFormData), psourcemap.Processor, *tcfg, reporter)
authHandler := builder.ForPrivilege(authorization.PrivilegeSourcemapWrite)
authHandler := builder.ForPrivilege(authorization.PrivilegeSourcemapWrite.Action)
return middleware.Wrap(h, sourcemapMiddleware(cfg, authHandler)...)
}

func backendAgentConfigHandler(cfg *config.Config, builder *authorization.Builder, _ publish.Reporter) (request.Handler, error) {
authHandler := builder.ForPrivilege(authorization.PrivilegeAgentConfigRead)
authHandler := builder.ForPrivilege(authorization.PrivilegeAgentConfigRead.Action)
return agentConfigHandler(cfg, authHandler, backendMiddleware)
}

Expand All @@ -193,7 +193,7 @@ func agentConfigHandler(cfg *config.Config, authHandler *authorization.Handler,

func rootHandler(cfg *config.Config, builder *authorization.Builder, _ publish.Reporter) (request.Handler, error) {
return middleware.Wrap(root.Handler(),
rootMiddleware(cfg, builder.ForAnyOfPrivileges(authorization.PrivilegesAll))...)
rootMiddleware(cfg, builder.ForAnyOfPrivileges(authorization.ActionAny))...)
}

func apmMiddleware(m map[request.ResultID]*monitoring.Int) []middleware.Middleware {
Expand Down
2 changes: 1 addition & 1 deletion beater/api/mux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func requestToMuxer(cfg *config.Config, r *http.Request) (*httptest.ResponseReco

func testHandler(t *testing.T, fn func(*config.Config, *authorization.Builder, publish.Reporter) (request.Handler, error)) request.Handler {
cfg := config.DefaultConfig(beatertest.MockBeatVersion())
builder, err := authorization.NewBuilder(cfg)
builder, err := authorization.NewBuilder(*cfg)
require.NoError(t, err)
h, err := fn(cfg, builder, beatertest.NilReporter)
require.NoError(t, err)
Expand Down
2 changes: 1 addition & 1 deletion beater/api/root/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func TestRootHandler(t *testing.T) {

t.Run("authorized", func(t *testing.T) {
c, w := beatertest.ContextWithResponseRecorder(http.MethodGet, "/")
builder, err := authorization.NewBuilder(&config.Config{SecretToken: "abc"})
builder, err := authorization.NewBuilder(config.Config{SecretToken: "abc"})
require.NoError(t, err)
c.Authorization = builder.ForPrivilege("").AuthorizationFor("Bearer", "abc")
Handler()(c)
Expand Down
4 changes: 3 additions & 1 deletion beater/authorization/allow.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@

package authorization

import "github.com/elastic/apm-server/elasticsearch"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unify imports for authorization means; in some files you import "github.com/elastic/apm-server/elasticsearch" and in some "es github.com/elastic/apm-server/elasticsearch".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there any problem with that? when there is an exhaustive usage of the package, an alias can help, otherwise is just fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no problem, purely consistency (it would just be super easy to unify, and makes it more readable). I marked the comment as nit anyways.


// AllowAuth implements the Authorization interface. It allows all authorization requests.
type AllowAuth struct{}

// AuthorizedFor always returns true
func (AllowAuth) AuthorizedFor(_ string) (bool, error) {
func (AllowAuth) AuthorizedFor(_ elasticsearch.Resource) (bool, error) {
return true, nil
}

Expand Down
95 changes: 37 additions & 58 deletions beater/authorization/apikey.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,43 +18,39 @@
package authorization

import (
"encoding/json"
"net/http"
"strings"
"fmt"
"time"

"github.com/pkg/errors"

"github.com/elastic/apm-server/beater/headers"
"github.com/elastic/apm-server/elasticsearch"
es "github.com/elastic/apm-server/elasticsearch"
)

const (
//DefaultResource for apm backend enabled API Keys
DefaultResource = "-"
const cleanupInterval = 60 * time.Second

application = "apm"
sep = `","`

cleanupInterval = 60 * time.Second
var (
jalvz marked this conversation as resolved.
Show resolved Hide resolved
// Application is a constant mapped to the "application" field for the Elasticsearch security API
// This identifies privileges and keys created for APM
Application = es.AppName("apm")
jalvz marked this conversation as resolved.
Show resolved Hide resolved
// ResourceInternal is only valid for first authorization of a request.
// The API Key needs to grant privileges to additional resources for successful processing of requests.
ResourceInternal = es.Resource("-")
ResourceAny = es.Resource("*")
)

type apikeyBuilder struct {
esClient elasticsearch.Client
esClient es.Client
cache *privilegesCache
anyOfPrivileges []string
anyOfPrivileges []es.PrivilegeAction
}

type apikeyAuth struct {
*apikeyBuilder
// key is base64(id:apiKey)
key string
}

type hasPrivilegesResponse struct {
Applications map[string]map[string]privileges `json:"application"`
}

func newApikeyBuilder(client elasticsearch.Client, cache *privilegesCache, anyOfPrivileges []string) *apikeyBuilder {
func newApikeyBuilder(client es.Client, cache *privilegesCache, anyOfPrivileges []es.PrivilegeAction) *apikeyBuilder {
return &apikeyBuilder{client, cache, anyOfPrivileges}
}

Expand All @@ -69,12 +65,8 @@ func (a *apikeyAuth) IsAuthorizationConfigured() bool {

// AuthorizedFor checks if the configured api key is authorized.
// An api key is considered to be authorized when the api key has the configured privileges for the requested resource.
// Privileges are fetched from Elasticsearch and then cached in a global cache.
func (a *apikeyAuth) AuthorizedFor(resource string) (bool, error) {
if resource == "" {
resource = DefaultResource
}

// Permissions are fetched from Elasticsearch and then cached in a global cache.
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for using "permission" rather than "privilege"? The APIs are all defined in terms of privileges AFAIK. I think it's best to stick to the same vocabulary.

I'm guessing you're using "permission" to refer to the combination of a privilege name and whether the user has it. Rather than introducing new vocabulary, how about we just don't name the type, and pass around map[PermissionAction]bool.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having a type Privilege and a type Permission the comments and variables should also follow this naming, as otherwise it is really confusing. That's why I asked to change the terminology consistently.

func (a *apikeyAuth) AuthorizedFor(resource es.Resource) (bool, error) {
//fetch from cache
if allowed, found := a.fromCache(resource); found {
return allowed, nil
Expand All @@ -98,7 +90,7 @@ func (a *apikeyAuth) AuthorizedFor(resource string) (bool, error) {
return allowed, nil
}

func (a *apikeyAuth) fromCache(resource string) (allowed bool, found bool) {
func (a *apikeyAuth) fromCache(resource es.Resource) (allowed bool, found bool) {
privileges := a.cache.get(id(a.key, resource))
if privileges == nil {
return
Expand All @@ -114,43 +106,30 @@ func (a *apikeyAuth) fromCache(resource string) (allowed bool, found bool) {
return
}

func (a *apikeyAuth) queryES(resource string) (privileges, error) {
query := buildQuery(PrivilegesAll, resource)
statusCode, body, err := a.esClient.SecurityHasPrivilegesRequest(strings.NewReader(query),
http.Header{headers.Authorization: []string{headers.APIKey + " " + a.key}})
if err != nil {
return nil, err
}
defer body.Close()
if statusCode != http.StatusOK {
// return nil privileges for queried apps to ensure they are cached
return privileges{}, nil
func (a *apikeyAuth) queryES(resource es.Resource) (es.Permissions, error) {
request := es.HasPrivilegesRequest{
Applications: []es.Application{
{
Name: Application,
// it is important to query all privilege actions because they are cached by api key+resources
// querying a.anyOfPrivileges would result in an incomplete cache entry
Privileges: ActionsAll(),
Resources: []es.Resource{resource},
},
},
}

var decodedResponse hasPrivilegesResponse
if err := json.NewDecoder(body).Decode(&decodedResponse); err != nil {
info, err := es.HasPrivileges(a.esClient, request, a.key)
if err != nil {
return nil, err
}
if resources, ok := decodedResponse.Applications[application]; ok {
if privileges, ok := resources[resource]; ok {
return privileges, nil
if resources, ok := info.Application[Application]; ok {
if permissions, ok := resources[resource]; ok {
return permissions, nil
}
}
return privileges{}, nil
}

func buildQuery(privileges []string, resource string) string {
var b strings.Builder
b.WriteString(`{"application":[{"application":"`)
b.WriteString(application)
b.WriteString(`","privileges":["`)
b.WriteString(strings.Join(privileges, sep))
b.WriteString(`"],"resources":"`)
b.WriteString(resource)
b.WriteString(`"}]}`)
return b.String()
return es.Permissions{}, nil
}

func id(apiKey, resource string) string {
return apiKey + "_" + resource
func id(apiKey string, resource es.Resource) string {
return apiKey + "_" + fmt.Sprintf("%v", resource)
jalvz marked this conversation as resolved.
Show resolved Hide resolved
}
32 changes: 16 additions & 16 deletions beater/authorization/apikey_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,11 @@ func TestApikeyBuilder(t *testing.T) {
handler2 := tc.builder.forKey(key)

// add existing privileges to shared cache
privilegesValid := privileges{}
privilegesValid := elasticsearch.Permissions{}
for _, p := range PrivilegesAll {
privilegesValid[p] = true
privilegesValid[p.Action] = true
}
resource := "service-go"
resource := elasticsearch.Resource("service-go")
tc.cache.add(id(key, resource), privilegesValid)

// check that cache is actually shared between apiKeyHandlers
Expand Down Expand Up @@ -85,12 +85,12 @@ func TestAPIKey_AuthorizedFor(t *testing.T) {
tc.setup(t)
key := ""
handler := tc.builder.forKey(key)
resourceValid := "foo"
resourceInvalid := "bar"
resourceMissing := "missing"
resourceValid := elasticsearch.Resource("foo")
resourceInvalid := elasticsearch.Resource("bar")
resourceMissing := elasticsearch.Resource("missing")

tc.cache.add(id(key, resourceValid), privileges{tc.anyOfPrivileges[0]: true})
tc.cache.add(id(key, resourceInvalid), privileges{tc.anyOfPrivileges[0]: false})
tc.cache.add(id(key, resourceValid), elasticsearch.Permissions{tc.anyOfPrivileges[0]: true})
tc.cache.add(id(key, resourceInvalid), elasticsearch.Permissions{tc.anyOfPrivileges[0]: false})

valid, err := handler.AuthorizedFor(resourceValid)
require.NoError(t, err)
Expand Down Expand Up @@ -143,9 +143,9 @@ func TestAPIKey_AuthorizedFor(t *testing.T) {
handler := tc.builder.forKey("12a3")

valid, err := handler.AuthorizedFor("xyz")
require.NoError(t, err)
require.Error(t, err)
assert.False(t, valid)
assert.Equal(t, 1, tc.cache.cache.ItemCount())
assert.Equal(t, 0, tc.cache.cache.ItemCount())
})

t.Run("decode error from ES", func(t *testing.T) {
Expand All @@ -163,7 +163,7 @@ type apikeyTestcase struct {
transport *estest.Transport
client elasticsearch.Client
cache *privilegesCache
anyOfPrivileges []string
anyOfPrivileges []elasticsearch.PrivilegeAction

builder *apikeyBuilder
}
Expand All @@ -174,19 +174,19 @@ func (tc *apikeyTestcase) setup(t *testing.T) {
if tc.transport == nil {
tc.transport = estest.NewTransport(t, http.StatusOK, map[string]interface{}{
"application": map[string]interface{}{
application: map[string]privileges{
"foo": {PrivilegeAgentConfigRead: true, PrivilegeEventWrite: true, PrivilegeSourcemapWrite: false},
"bar": {PrivilegeAgentConfigRead: true, PrivilegeEventWrite: false},
"apm": map[string]map[string]interface{}{
"foo": {"config_agent:read": true, "event:write": true, "sourcemap:write": false},
"bar": {"config_agent:read": true, "event:write": false},
}}})
}
tc.client, err = estest.NewElasticsearchClient(tc.transport)
require.NoError(t, err)
}
if tc.cache == nil {
tc.cache = newPrivilegesCache(time.Millisecond, 5)
tc.cache = newPrivilegesCache(time.Minute, 5)
}
if tc.anyOfPrivileges == nil {
tc.anyOfPrivileges = []string{PrivilegeEventWrite, PrivilegeSourcemapWrite}
tc.anyOfPrivileges = []elasticsearch.PrivilegeAction{PrivilegeEventWrite.Action, PrivilegeSourcemapWrite.Action}
}
tc.builder = newApikeyBuilder(tc.client, tc.cache, tc.anyOfPrivileges)
}
4 changes: 3 additions & 1 deletion beater/authorization/bearer.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package authorization

import (
"crypto/subtle"

"github.com/elastic/apm-server/elasticsearch"
)

type bearerBuilder struct {
Expand All @@ -39,7 +41,7 @@ func (b bearerBuilder) forToken(token string) *bearerAuth {
configured: true}
}

func (b *bearerAuth) AuthorizedFor(_ string) (bool, error) {
func (b *bearerAuth) AuthorizedFor(_ elasticsearch.Resource) (bool, error) {
return b.authorized, nil
}

Expand Down
Loading