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

Don't send POLICY_CHANGE actions retrieved from index to agent. #1963

Merged
merged 4 commits into from
Oct 17, 2022
Merged

Don't send POLICY_CHANGE actions retrieved from index to agent. #1963

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
09fc555
Don't send POLICY_CHANGE actions retrieved from index to agent.
michel-laterman Oct 6, 2022
d8712bf
move filtering to its own method
michel-laterman Oct 17, 2022
100a4eb
Fix linter, tests, fix file name
michel-laterman Oct 17, 2022
19745de
Merge branch 'main' into remove-policy-change-actions-checkin
michel-laterman Oct 17, 2022
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
3 changes: 2 additions & 1 deletion CHANGELOG.next.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
- LoadServerLimits will not overwrite specified limits when loading default/agent number specified values. {issue}1841[1841] {pull}1912[1912]
- Use seperate rate limiters for internal and external API listeners. {issue}1859[1859] {pull}1904[1904]
- Fix fleet.migration.total log key overlap {pull}1951[1951]
- Remove POLICY_CHANGE actions from list retrieved from actions index before sending actions to agent on Checkin. {issue}1773[1773] {pull}1963[1963]

==== New Features

Expand All @@ -25,4 +26,4 @@
- Fleet Server now allows setting global labels on APM instrumentation. {pull}1649[1649]
- Fleet Server now allows setting transaction sample rate on APM instrumentation {pull}1681[1681]
- Log redacted config when config updates. {issue}1626[1626] {pull}1668[1668]
- Storing checkin message in last_checkin_message {pull}1932[1932]
- Storing checkin message in last_checkin_message {pull}1932[1932]
4 changes: 4 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 @@ -376,6 +376,10 @@ func convertActions(agentID string, actions []model.Action) ([]ActionResp, strin

respList := make([]ActionResp, 0, sz)
for _, action := range actions {
if action.Type == "POLICY_CHANGE" {
michel-laterman marked this conversation as resolved.
Show resolved Hide resolved
michel-laterman marked this conversation as resolved.
Show resolved Hide resolved
michel-laterman marked this conversation as resolved.
Show resolved Hide resolved
log.Warn().Str("agent_id", agentID).Str("action_id", action.ActionID).Msg("Removing POLICY_CHANGE action found in index from check in response")
michel-laterman marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@jsoriano jsoriano Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Question: When are these policy change actions an issue on check in?

Looking at the code, it seems that TypePolicyChange actions are only created on checkin code, in the processPolicy method, so it doesn't seem to be always an issue on checkin.

Here a filter is being added to convertAction, that is used when:

  • There are pending actions.
  • There are not pending actions, but something is received from the actions dispatcher.

Are these policy change actions an issue on both cases?
Are they generated in some other place too?

If this checkin handler is the only source of these actions, could there be some way to avoid generating them instead of having to filter them later?

Copy link
Contributor Author

@michel-laterman michel-laterman Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The fleet-server creates policy change actions dynamically when it detects a difference in the config an agent is running and what is defined. This occurs in the processRequest method:

fleet-server/internal/pkg/api/handleCheckin.go

Lines 241 to 247 in a516f03

case policy := <-sub.Output():
actionResp, err := processPolicy(ctx, zlog, ct.bulker, agent.Id, policy)
if err != nil {
return errors.Wrap(err, "processPolicy")
}
actions = append(actions, *actionResp)
break LOOP

We want to consider this as the source of truth.

However the processRequest method also passes actions retrieved from the actions index (pendingActions also retrieves from this index):

fleet-server/internal/pkg/api/handleCheckin.go

Lines 236 to 240 in a516f03

case acdocs := <-actCh:
var acs []ActionResp
acs, ackToken = convertActions(agent.Id, acdocs)
actions = append(actions, acs...)
break LOOP

It's possible for a user to insert a policy change action this way. These actions can make it difficult to determine what policy an agent should be running, it is also very easy to insert a malformed action by mistake and cause the agent to get stuck.
Filtering policy change actions received from the index out prevents these issues.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanations!

the actions index

It's possible for a user to insert a policy change action this way.

How can actions be added to the actions index? Or how can users insert policy changes?

continue
}
respList = append(respList, ActionResp{
AgentID: agentID,
CreatedAt: action.Timestamp,
Expand Down
65 changes: 50 additions & 15 deletions internal/pkg/api/handleChecking_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,25 +15,60 @@ import (
"github.com/stretchr/testify/assert"
)

func TestConvertActionsEmpty(t *testing.T) {
resp, token := convertActions("1234", nil)
assert.Equal(t, resp, []ActionResp{})
assert.Equal(t, token, "")
}

func TestConvertActions(t *testing.T) {
actions := []model.Action{
{
ActionID: "1234",
tests := []struct {
name string
actions []model.Action
resp []ActionResp
token string
}{{
name: "empty actions",
actions: nil,
resp: []ActionResp{},
token: "",
}, {
name: "single action",
actions: []model.Action{{ActionID: "1234"}},
resp: []ActionResp{{
AgentID: "agent-id",
ID: "1234",
Data: json.RawMessage(nil),
}},
token: "",
}, {
name: "multiple actions",
actions: []model.Action{
{ActionID: "1234"},
{ActionID: "5678"},
},
}
resp, token := convertActions("agent-id", actions)
assert.Equal(t, resp, []ActionResp{
{
resp: []ActionResp{{
AgentID: "agent-id",
ID: "1234",
Data: json.RawMessage(nil),
}, {
AgentID: "agent-id",
ID: "5678",
Data: json.RawMessage(nil),
}},
token: "",
}, {
name: "remove POLICY_CHANGE action",
actions: []model.Action{
{ActionID: "1234", Type: "POLICY_CHANGE"},
{ActionID: "5678"},
},
})
assert.Equal(t, token, "")
resp: []ActionResp{{
AgentID: "agent-id",
ID: "5678",
Data: json.RawMessage(nil),
}},
token: "",
}}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
resp, token := convertActions("agent-id", tc.actions)
assert.Equal(t, tc.resp, resp)
assert.Equal(t, tc.token, token)
})
}
}