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.

Sign up for GitHub

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

invalidate remote api keys on unenroll #3154

Merged
merged 11 commits into from
Dec 12, 2023
Merged

invalidate remote api keys on unenroll #3154

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3e06b0f
invalidate remote api keys on unenroll
juliaElastic Dec 7, 2023
e2679f8
fix test
juliaElastic Dec 11, 2023
55803dc
using latest es snapshot in tests
juliaElastic Dec 11, 2023
2ec7678
try with latest 8.12 snapshot
juliaElastic Dec 11, 2023
88d1cb8
use new 8.13 snapshot
juliaElastic Dec 12, 2023
a6efc9b
try without build id
juliaElastic Dec 12, 2023
35f3539
invalidate remote api key after force unenroll
juliaElastic Dec 12, 2023
b183e55
fix lint
juliaElastic Dec 12, 2023
853745b
added integration test to force unenroll
juliaElastic Dec 12, 2023
26dd618
added integration test on unenroll
juliaElastic Dec 12, 2023
3585b69
fix lint
juliaElastic Dec 12, 2023
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
2 changes: 1 addition & 1 deletion dev-tools/integration/.env
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
ELASTICSEARCH_VERSION=8.12.0-9d443b17-SNAPSHOT
ELASTICSEARCH_VERSION=8.13.0-SNAPSHOT
ELASTICSEARCH_USERNAME=elastic
Copy link
Member

Choose a reason for hiding this comment

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

This change broke the automation with updatecli, see https://github.com/elastic/fleet-server/actions/runs/7197121542

is that the expected behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, I changed it to fix e2e tests, it was not working with a specific 8.13 build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how can we fix this?

Copy link
Member

Choose a reason for hiding this comment

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

Using 8.13.0-jqmw70gd in main and 8.12.0-8db20ea5 in 8.12

I took those values from https://github.com/elastic/fleet-server/actions/runs/7197121542/job/19603532844#step:3:213

https://storage.googleapis.com/artifacts-api/snapshots/8.12.json
https://storage.googleapis.com/artifacts-api/snapshots/main.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mind raising prs to fix this or do you want me to?

ELASTICSEARCH_PASSWORD=changeme
TEST_ELASTICSEARCH_HOSTS=localhost:9200
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/api/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ func authAgent(r *http.Request, id *string, bulker bulk.Bulk, c cache.Cache) (*m

// Update the cache to mark the api key id associated with this agent as not enabled
c.SetAPIKey(*key, false)
return nil, ErrAgentInactive
return agent, ErrAgentInactive
}

return agent, nil
Expand Down
100 changes: 50 additions & 50 deletions internal/pkg/api/handleAck.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,62 +567,16 @@ func cleanRoles(roles json.RawMessage) (json.RawMessage, int, error) {
}

func (ack *AckT) invalidateAPIKeys(ctx context.Context, zlog zerolog.Logger, toRetireAPIKeyIDs []model.ToRetireAPIKeyIdsItems, skip string) {
ids := make([]string, 0, len(toRetireAPIKeyIDs))
remoteIds := make(map[string][]string)
for _, k := range toRetireAPIKeyIDs {
if k.ID == skip || k.ID == "" {
continue
}
if k.Output != "" {
if remoteIds[k.Output] == nil {
remoteIds[k.Output] = make([]string, 0)
}
remoteIds[k.Output] = append(remoteIds[k.Output], k.ID)
} else {
ids = append(ids, k.ID)
}
}
if len(ids) > 0 {
zlog.Info().Strs("fleet.policy.apiKeyIDsToRetire", ids).Msg("Invalidate old API keys")
if err := ack.bulk.APIKeyInvalidate(ctx, ids...); err != nil {
zlog.Info().Err(err).Strs("ids", ids).Msg("Failed to invalidate API keys")
}
}
// using remote es bulker to invalidate api key
for outputName, outputIds := range remoteIds {
outputBulk := ack.bulk.GetBulker(outputName)

if outputBulk == nil {
// read output config from .fleet-policies, not filtering by policy id as agent could be reassigned
policy, err := dl.QueryOutputFromPolicy(ctx, ack.bulk, outputName)
if err != nil || policy == nil {
zlog.Warn().Str("outputName", outputName).Any("ids", outputIds).Msg("Output policy not found, API keys will be orphaned")
} else {
outputBulk, _, err = ack.bulk.CreateAndGetBulker(ctx, zlog, outputName, policy.Data.Outputs)
if err != nil {
zlog.Warn().Str("outputName", outputName).Any("ids", outputIds).Msg("Failed to recreate output bulker, API keys will be orphaned")
}
}
}
if outputBulk != nil {
if err := outputBulk.APIKeyInvalidate(ctx, outputIds...); err != nil {
zlog.Info().Err(err).Strs("ids", outputIds).Str("outputName", outputName).Msg("Failed to invalidate API keys")
}
}
}
invalidateAPIKeys(ctx, zlog, ack.bulk, toRetireAPIKeyIDs, skip)
}

func (ack *AckT) handleUnenroll(ctx context.Context, zlog zerolog.Logger, agent *model.Agent) error {
span, ctx := apm.StartSpan(ctx, "ackUnenroll", "process")
defer span.End()
apiKeys := agent.APIKeyIDs()
if len(apiKeys) > 0 {
zlog = zlog.With().Strs(LogAPIKeyID, apiKeys).Logger()

if err := ack.bulk.APIKeyInvalidate(ctx, apiKeys...); err != nil {
return fmt.Errorf("handleUnenroll invalidate apikey: %w", err)
}
}
apiKeys := agent.APIKeyIDs()
zlog.Info().Any("fleet.policy.apiKeyIDsToRetire", apiKeys).Msg("handleUnenroll invalidate API keys")
ack.invalidateAPIKeys(ctx, zlog, apiKeys, "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

building []ToRetireAPIKeyIdsItems to reuse the logic in invalidateAPIKeys to invalidate against remote es


now := time.Now().UTC().Format(time.RFC3339)
doc := bulk.UpdateFields{
Expand Down Expand Up @@ -740,3 +694,49 @@ func makeUpdatePolicyBody(policyID string, newRev, coordIdx int64) []byte {

return buf.Bytes()
}

func invalidateAPIKeys(ctx context.Context, zlog zerolog.Logger, bulk bulk.Bulk, toRetireAPIKeyIDs []model.ToRetireAPIKeyIdsItems, skip string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this function outside of AckT so it is accessible from handleCheckin too

ids := make([]string, 0, len(toRetireAPIKeyIDs))
remoteIds := make(map[string][]string)
for _, k := range toRetireAPIKeyIDs {
if k.ID == skip || k.ID == "" {
continue
}
if k.Output != "" {
if remoteIds[k.Output] == nil {
remoteIds[k.Output] = make([]string, 0)
}
remoteIds[k.Output] = append(remoteIds[k.Output], k.ID)
} else {
ids = append(ids, k.ID)
}
}
if len(ids) > 0 {
zlog.Info().Strs("fleet.policy.apiKeyIDsToRetire", ids).Msg("Invalidate old API keys")
if err := bulk.APIKeyInvalidate(ctx, ids...); err != nil {
zlog.Info().Err(err).Strs("ids", ids).Msg("Failed to invalidate API keys")
}
}
// using remote es bulker to invalidate api key
for outputName, outputIds := range remoteIds {
outputBulk := bulk.GetBulker(outputName)

if outputBulk == nil {
// read output config from .fleet-policies, not filtering by policy id as agent could be reassigned
policy, err := dl.QueryOutputFromPolicy(ctx, bulk, outputName)
if err != nil || policy == nil {
zlog.Warn().Str("outputName", outputName).Any("ids", outputIds).Msg("Output policy not found, API keys will be orphaned")
} else {
outputBulk, _, err = bulk.CreateAndGetBulker(ctx, zlog, outputName, policy.Data.Outputs)
if err != nil {
zlog.Warn().Str("outputName", outputName).Any("ids", outputIds).Msg("Failed to recreate output bulker, API keys will be orphaned")
}
}
}
if outputBulk != nil {
if err := outputBulk.APIKeyInvalidate(ctx, outputIds...); err != nil {
zlog.Info().Err(err).Strs("ids", outputIds).Str("outputName", outputName).Msg("Failed to invalidate API keys")
}
}
}
}
17 changes: 17 additions & 0 deletions internal/pkg/api/handleCheckin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,11 @@

agent, err := authAgent(r, &id, ct.bulker, ct.cache)
if err != nil {
// invalidate remote API keys of force unenrolled agents
if err == ErrAgentInactive && agent != nil {

Check failure on line 123 in internal/pkg/api/handleCheckin.go

View workflow job for this annotation

GitHub Actions / lint (linux) 

comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added testing steps of the force unenroll scenario to the pr description.

Decided to invalidate remote API keys on checkin here, when finding that the agent is inactive, because the agent becomes inactive after force unenroll. It's not guaranteed that the agent is going to check-in after force unenroll, but we can't do the invalidation in kibana, as kibana doesn't have access to the remote es service token as it is a secret.
Fleet-server also doesn't handle force unenroll actions, and even if it would, fleet server only reads actions on agent checkin. So I didn't find a better way to invalidate api keys after force unenroll.

ctx := zlog.WithContext(r.Context())
invalidateAPIKeysOfInactiveAgent(ctx, zlog, ct.bulker, agent)
}
return err
}

Expand All @@ -136,6 +141,18 @@
return ct.ProcessRequest(zlog, w, r, start, agent, newVer)
}

func invalidateAPIKeysOfInactiveAgent(ctx context.Context, zlog zerolog.Logger, bulker bulk.Bulk, agent *model.Agent) {
remoteAPIKeys := make([]model.ToRetireAPIKeyIdsItems, 0)
apiKeys := agent.APIKeyIDs()
for _, key := range apiKeys {
if key.Output != "" {
remoteAPIKeys = append(remoteAPIKeys, key)
}
}
zlog.Info().Any("fleet.policy.apiKeyIDsToRetire", remoteAPIKeys).Msg("handleCheckin invalidate remote API keys")
invalidateAPIKeys(ctx, zlog, bulker, remoteAPIKeys, "")
}

// validatedCheckin is a struct to wrap all the things that validateRequest returns.
type validatedCheckin struct {
req *CheckinRequest
Expand Down
24 changes: 18 additions & 6 deletions internal/pkg/model/ext.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,22 +44,34 @@ func (a *Agent) CheckDifferentVersion(ver string) string {

// APIKeyIDs returns all the API keys, the valid, in-use as well as the one
// marked to be retired.
func (a *Agent) APIKeyIDs() []string {
func (a *Agent) APIKeyIDs() []ToRetireAPIKeyIdsItems {
if a == nil {
return nil
}
keys := make([]string, 0, len(a.Outputs)+1)
keys := make([]ToRetireAPIKeyIdsItems, 0, len(a.Outputs)+1)
if a.AccessAPIKeyID != "" {
keys = append(keys, a.AccessAPIKeyID)
keys = append(keys, ToRetireAPIKeyIdsItems{
ID: a.AccessAPIKeyID,
Output: "",
RetiredAt: "",
})
}

for _, output := range a.Outputs {
for outputName, output := range a.Outputs {
if output.APIKeyID != "" {
keys = append(keys, output.APIKeyID)
name := ""
if outputName != "default" {
name = outputName
}
keys = append(keys, ToRetireAPIKeyIdsItems{
ID: output.APIKeyID,
Output: name,
RetiredAt: "",
})
}
for _, key := range output.ToRetireAPIKeyIds {
if key.ID != "" {
keys = append(keys, key.ID)
keys = append(keys, key)
}
}
}
Expand Down
26 changes: 16 additions & 10 deletions internal/pkg/model/ext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func TestAgentAPIKeyIDs(t *testing.T) {
tcs := []struct {
name string
agent Agent
want []string
want []ToRetireAPIKeyIdsItems
}{
{
name: "no API key marked to be retired",
Expand All @@ -99,7 +99,9 @@ func TestAgentAPIKeyIDs(t *testing.T) {
"p2": {APIKeyID: "p2_api_key_id"},
},
},
want: []string{"access_api_key_id", "p1_api_key_id", "p2_api_key_id"},
want: []ToRetireAPIKeyIdsItems{{ID: "access_api_key_id", Output: "", RetiredAt: ""},
{ID: "p1_api_key_id", Output: "p1", RetiredAt: ""},
{ID: "p2_api_key_id", Output: "p2", RetiredAt: ""}},
},
{
name: "with API key marked to be retired",
Expand All @@ -109,18 +111,22 @@ func TestAgentAPIKeyIDs(t *testing.T) {
"p1": {
APIKeyID: "p1_api_key_id",
ToRetireAPIKeyIds: []ToRetireAPIKeyIdsItems{{
ID: "p1_to_retire_key",
ID: "p1_to_retire_key",
Output: "remote",
}}},
"p2": {
APIKeyID: "p2_api_key_id",
ToRetireAPIKeyIds: []ToRetireAPIKeyIdsItems{{
ID: "p2_to_retire_key",
ID: "p2_to_retire_key",
Output: "remote",
}}},
},
},
want: []string{
"access_api_key_id", "p1_api_key_id", "p2_api_key_id",
"p1_to_retire_key", "p2_to_retire_key"},
want: []ToRetireAPIKeyIdsItems{{ID: "access_api_key_id", Output: "", RetiredAt: ""},
{ID: "p1_api_key_id", Output: "p1", RetiredAt: ""},
{ID: "p2_api_key_id", Output: "p2", RetiredAt: ""},
{ID: "p1_to_retire_key", Output: "remote", RetiredAt: ""},
{ID: "p2_to_retire_key", Output: "remote", RetiredAt: ""}},
},
{
name: "API key empty",
Expand All @@ -130,7 +136,7 @@ func TestAgentAPIKeyIDs(t *testing.T) {
"p1": {APIKeyID: ""},
},
},
want: []string{"access_api_key_id"},
want: []ToRetireAPIKeyIdsItems{{ID: "access_api_key_id", Output: "", RetiredAt: ""}},
},
{
name: "retired API key empty",
Expand All @@ -144,8 +150,8 @@ func TestAgentAPIKeyIDs(t *testing.T) {
}}},
},
},
want: []string{
"access_api_key_id", "p1_api_key_id"},
want: []ToRetireAPIKeyIdsItems{{ID: "access_api_key_id", Output: "", RetiredAt: ""},
{ID: "p1_api_key_id", Output: "p1", RetiredAt: ""}},
},
}

Expand Down
Loading