Add apikey subcommand

[jalvz]

Still a couple tests failing

• Integration tests pending

Usage:

Manage API Keys for communication between APM agents and server. 
Most operations require the "manage_security" cluster privilege. Ensure to configure "apm-server.api_key.*" or 
"output.elasticsearch.*" appropriately. APM Server will create security privileges for the "apm" application; 
you can freely query them. If you modify or delete apm privileges, APM Server might reject all requests.
If an invalid argument is passed, nothing will be printed.
Check the Elastic Security API documentation for details.

Usage:
  apm-server apikey [command]

Available Commands:
  create      Create an API Key with the specified privilege(s)
  info        Query API Key(s) by Id or Name
  invalidate  Invalidate API Key(s) by Id or Name
  verify      Check if a "credentials" string has the given privilege(s)

Create an API Key with the specified privilege(s).
If no privilege(s) are specified, the API Key will be valid for all.
Requires the "manage_security" cluster privilege in Elasticsearch.

Usage:
  apm-server apikey create [flags]

Flags:
      --name string         API Key name (default "apm-key")
      --expiration string   expiration for the key, eg. "1d" (default never)
      --ingest              give the {event event:write} privilege to this key, required for ingesting events
      --sourcemap           give the {sourcemap sourcemap:write} privilege to this key, required for uploading sourcemaps
      --agent-config        give the {agentConfig config_agent:read} privilege to this key, required for agents to read configuration remotely
      --json                prints the output of this command as JSON
  -h, --help                help for create

Invalidate API Key(s) by Id or Name.
If both "id" and "name" are supplied, only "id" will be used.
If neither of them are, an error will be returned.
Requires the "manage_security" cluster privilege in Elasticsearch.

Usage:
  apm-server apikey invalidate [flags]

Flags:
      --id string     id of the API Key to delete
      --name string   name of the API Key(s) to delete (several might match)
      --purge         also remove all privileges created and used by APM Server
      --json          prints the output of this command as JSON
  -h, --help          help for invalidate

Query API Key(s) by Id or Name.
If both "id" and "name" are supplied, only "id" will be used.
If neither of them are, an error will be returned.
Requires the "manage_security" cluster privilege in Elasticsearch.

Usage:
  apm-server apikey info [flags]

Flags:
      --id string     id of the API Key to query
      --name string   name of the API Key(s) to query (several might match)
      --valid-only    only return valid API Keys (not expired or invalidated)
      --json          prints the output of this command as JSON
  -h, --help          help for info

Check if a "credentials" string has the given privilege(s).
If no privilege(s) are specified, the credentials will be queried for all.

Usage:
  apm-server apikey verify [flags]

Flags:
      --credentials string   credentials for which check privileges
      --ingest               ask for the {event event:write} privilege, required for ingesting events
      --sourcemap            ask for the {sourcemap sourcemap:write} privilege, required for uploading sourcemaps
      --agent-config         ask for the {agentConfig config_agent:read} privilege, required for agents to read configuration remotely
      --json                 prints the output of this command as JSON
  -h, --help                 help for verify

Closes #2987

[simitt]

A couple of remarks after playing around with the commands:

• expiration information misleading: when returning an invalidated API Key Expiration ..... never is misleading
• output consistency: there is if no expiration date is set - no expiration key in json output but Expiration...... never in text output
• require parameter for invalidate: ./apm-server apikey invalidate throws an ES error. Require id or name as parameter and throw more user friendly error if missing
• require parameter for verify: ./apm-server apikey verify --credentials make credentials a required parameter instead of a flag as it needs to be passed in for a successful request
• consistency when creating privileges: Why does ./apm-server apikey create --ingest create all security privileges and not only for ingest?
• verifying non-existing privileges: ./apm-server apikey verify --credentials XXX --ingest --something does not print anything. Can you add some output along the lines of non-existing privilege, or if this information is not available an output that the key is invalid for it?

Some bugs:

• name flag does not seem to be picked up when invalidating keys: ./apm-server apikey invalidate --name apm-systemtest throws exception, while ./apm-server apikey invalidate --id 90Zm_m4BXuxExeOtJ1ki works just fine with the same seetings.
  Raised error:

Error invalidating API Key(s), do you have the "manage_cluster" security privilege?
{"error":{"root_cause":[{"type":"action_request_validation_exception","reason":"Validation Failed: 1: One of [api key id, api key name, username, realm name] must be specified if [owner] flag is false;"}],"type":"action_request_validation_exception","reason":"Validation Failed: 1: One of [api key id, api key name, username, realm name] must be specified if [owner] flag is false;"},"status":400}

• verify only verifies first privilege: ./apm-server apikey create --ingest --agent-config, followed by ./apm-server apikey verify --credentials XXX --ingest --sourcemap --agent-config returns Authorized for privilege "config_agent:read"...: No, but when only verifying agent-config the output returns Yes
  I suspect that only the first passed in flag (e.g. --ingest) is used for the query

• name flag does not seem to be picked up when info is provided: ./apm-server apikey info --name apm-key provided name is ignored and all keys returned

Will provide code review separately when done.

[jalvz]

Thanks. A few of those comments unfortunately are related to Cobra limitations:

I expect this subcommand to be used programmatically, so I think is important that it always honours the --json flag. If an error is returned, Cobra will print it, but not as JSON. Which means that we need to silence them and print them ourselves: https://github.com/elastic/apm-server/pull/3063/files#diff-ff7686b39bf90dc2520886fb874371a4R193.
I initially thought that SilenceErrors would only affect errors inside the *RunE implementations, but unfortunately that is not the case; and we don't have access to the Cobra machinery. Neither I can pass the json variable to it because it has been parsed yet, neither define the json variable in the parent command and pass it to the children (or at least I couldn't get it to work).
This all means that we can't mark flags as required, and if a flag is invalid nothing will be printed, which has been noted https://github.com/elastic/apm-server/pull/3063/files#diff-ff7686b39bf90dc2520886fb874371a4R157
Not great, but didn't have any better ideas. Let me know if you do.

I fixed however some pointers, which should help with the --name flag not picked and the "name or id required" error.

output consistency: there is if no expiration date is set - no expiration key in json output but Expiration...... never in text output

I think is OK that output is different data for human and machine consumption. A "never" string is going to be of no use for a program, so it mimics the Elasticsearch response (no expiration = never); however does not harm to be more explicit when replying a human. If if bothers you, it can be removed.

consistency when creating privileges: Why does ./apm-server apikey create --ingest create all security privileges and not only for ingest?

Because the user doesn't care, and the code is simpler that way (with less conditionals). Maintaining it is going to be a bit more complicated (https://github.com/elastic/apm-server/pull/3063/files#diff-2d9068aff6ea838c5c2463d1fd9bee36R40-R42), so I think is important to keep it simple.

verify only verifies first privilege: ./apm-server apikey create --ingest --agent-config, followed by ./apm-server apikey verify --credentials XXX --ingest --sourcemap --agent-config returns Authorized for privilege "config_agent:read"...: No, but when only verifying agent-config the output returns Yes

Good catch. It turns out that we can't reuse the same Builder for several checks, didn't know that. Moved it inside the for loop and now it works: 1d7f204#diff-2d9068aff6ea838c5c2463d1fd9bee36R220

expiration information misleading: when returning an invalidated API Key Expiration ..... never is misleading

This is an Elasticsearch thing, "expired" is not the same as "invalidated", and it can happen that either is true and the other one is not. I can add a conditional to not print the expiration in that case to avoid confusion.

[simitt]

What exactly should be deprecated and why?

[jalvz]

the line bellow it, the Search interface. Mostly because it is redundant now:
it can be implemented with the JsonRequest and richer data types (as functions in security_api.go, instead of requiring and returning low level readers.

It would also resolve colliding names, which would allow us to embed the whole client struct in the wrapper, avoiding the need for more (confusing) names: #3063 (comment)

[simitt]

Are you planning on doing this within this PR? If not, please create an issue with a small description so someone can pick it up and maybe link it here to the TODO.

[simitt]

This is an Elasticsearch thing, "expired" is not the same as "invalidated", and it can happen that either is true and the other one is not. I can add a conditional to not print the expiration in that case to avoid confusion.

I assumed that invalidated was automatically true when key was expired. In that case though, I agree with your original solution that the result should reflect ES response and show Invalidated and Expired.

Regarding the issues you are facing when using cobra and the --json flag:

• I am wondering what the usecase is to support two output formats?
• I was expecting the apikey commands to return a response in json format that can be redirected to any output file. This response can contain e.g. valid api keys, but also errors e.g. when ES is unreachable. The format of this response is in our hands, and you could print an output after json marshalling it.
  From what I understand all the issues with cobra not returning json formatted output arise from the goal of also returning invalid CLI usage errors as json. I do not see the advantage of doing that.

[simitt]

verify only verifies first privilege: ./apm-server apikey create --ingest --agent-config, followed by ./apm-server apikey verify --credentials XXX --ingest --sourcemap --agent-config returns Authorized for privilege "config_agent:read"...: No, but when only verifying agent-config the output returns Yes

Good catch. It turns out that we can't reuse the same Builder for several checks, didn't know that. Moved it inside the for loop and now it works: 1d7f204#diff-2d9068aff6ea838c5c2463d1fd9bee36R220

I am pretty sure the issue was somewhere else and resolved by one of the other fixes. Having the builder outside the loop works just fine, please change it again.

[simitt]

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".

[jalvz]

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

[simitt]

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.

[simitt]

The go-elasticsearch client offers an API for all the security related requests. We should leverage that and use it rather than re-implementing it.
Also, imagine the ES endpoint changes with the next major version from /_security/api_key to /_sec/api_key (or whatever), then one would need to maintain different versions of the go-elasticsearch client AND check the endpoints for all supported ES versions again in our code.

This comment also concerns all the following methods.

[jalvz]

We should leverage that and use it rather than re-implementing it
The only thing we are missing from the client is crafting a request. It doesn't buy us that much, and it would mean that we'd have to duplicate every single function.

If there is a breaking change, we need to take care of it either way and in a similar manner, so can we tackle that problem when it comes?

[simitt]

Why would we need to take care of a breaking change in the ES API if we use the provided go-elasticsearch client API? This API would abstract such ES API changes, right?

[jalvz]

Sure, I mean we would need 2 different implementations, one with each client.
That's the same with this approach, but at least like this we don't need 2 implementations for each function always, only when they diverge. Does that make sense?

[simitt]

We should optimize for correctness. Bringing ES details into the APM Server code, rather than using the abstraction the go-elasticsearch client provides is error prone.
If doing so, you would need to add integration tests for every single function in the elasticsearch package testing against v7 and v8 Elasticsearch instance to ensure breaking changes are recognized. That's duplicated effort, as already handled by the client.
The duplication of the clients API calls is ofc unfortunate, but they can be kept to a minimum, all the response decoding etc. you are doing doesn't need to be duplicated.

Maybe @karmi has some ideas on integrating with a v7 and v8 elasticsearch-go client at the same time, with minimum repetition while still leveraging the clients API.

[simitt]

I was thinking about something along the lines of https://github.com/jalvz/apm-server/compare/apikey-command...simitt:es-poc?expand=1

[axw]

I think there's a compromise available. Since we're currently assuming the requests are compatible between v7 and v8 (the only difference in the code is which client is used for calling JSONRequest), then we could instead use the esapi request types from v7 and execute them with either client's transport. This keeps the ES API details inside the ES client.

https://gist.github.com/axw/8d9e3bd234faec19fa8ed265c460101d

We'll eventually need to have some code to deal with differences in the request and response bodies, and possibly request parameters. The ES low-level client doesn't abstract away the differences between versions. We could do that by doing something like below, however I think that's a much bigger issue that we should deal with later.

type clientV8 struct {
    // embed clientV7 to get its methods
    clientV7
}

// Foo in v8 is incompatible with Foo in v7, hence we override it.
func (c clientV8) Foo(req FooRequest) FooResponse {
    reqV8 := fooV8Request{/*set fields from req*/}
    var resp fooV8Response
    ...
    return FooResponse{/*set fields from resp*/}
}

type clientV7 {
    estransport.Transport // initialised with the transport from either v7 or v8
}

// Foo ...
func (c clientV7) Foo(req FooRequest) FooResponse {
    reqV7 := fooV7Request{/*set fields from req*/}
    var resp fooV7Response
    ...
    return FooResponse{/*set fields from resp*/}
}

// Bar is compatible with v7 and v8, so we don't override it in clientV8.
func (c clientV7) Bar(req BarRequest) BarResponse {
    reqV7 := barV7Request{/*set fields from req*/}
    var resp barV7Response
    ...
    return BarResponse{/*set fields from resp*/}
}

[simitt]

Interesting idea @axw! As you pointed out, this only works if the actual requests for v7 and v8 can be assumed to be the same. Since you are using v7 and not v8 structs you ensure that the code runs fine with currently released ES versions.
+1 from me on moving forward with this.

[jalvz]

Ah, I see. Funny enough, I considered that but I thought it would be rejected because an implementation detail difference in one version could cause a bug when using the other version, and would be harder to detect than say a typo in a endpoint name.
I'm fine with that.

[karmi]

Hello, I'm surprised that you went back to calling the ES endpoints manually, @jalvz — I remember we did find couple of ways of solving it when using the Go client, in our conversations.

I have particularly liked the "injection" of the client in one of the sketches you had, do you still have it handy?

I like the idea @axw is proposing.

I still do think that using the client for abstracting the communication with ES makes a lot of sense: manually crafting requests is something like an antipattern. I understand the challenges coming from the types being in different packages, but that's something we should be able to solve.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@022ae2e). Click here to learn what that means.
The diff coverage is 72.72%.

@@            Coverage Diff            @@
##             master    #3063   +/-   ##
=========================================
  Coverage          ?   78.85%           
=========================================
  Files             ?      106           
  Lines             ?     5614           
  Branches          ?        0           
=========================================
  Hits              ?     4427           
  Misses            ?     1187           
  Partials          ?        0

Impacted Files	Coverage Δ
beater/authorization/apikey.go	93.18% <100%> (ø)
beater/middleware/log_middleware.go	83.33% <71.42%> (ø)

addresssed

comments have been addressed

[axw]

Some of these things can be deferred (e.g. renaming things), but please create TODOs and tracking issue(s). IMO the most important things to get fixed:

• revert vendor/github.com/elastic/go-ucfg/error.go
• revert beater/server_test.go? Or maybe it's related, I don't know
• update cmd/apikey.go to return errors to Cobra

[axw]

Why is the error being silently ignored?

[jalvz]

IIRC, because it would fail to delete privileges if they don't exist. But I did this more than 1 month ago, my memory is fuzzy. Should check again

[axw]

Ah, because we're returning an error on 404. Probably shouldn't do that, but treat it as an expected status code. A problem for another day.

[jalvz]

isn't that what this it is doing? or you mean inside DeletePrivileges impl? what difference would that make? 🤔

[axw]

I meant inside DeletePrivleges - we could also do it here, if we expose details of the error. This isn't checking for 404, it's ignoring all errors. The error could be because Elasticsearch is down, because the privilege doesn't exist, or because it's a Monday.

[graphaelli]

superseded by #3157