SyncAuthorizedEntries: fix issue with entry id reuse and add integration test #5764
Conversation
sorindumitru
force-pushed
the
sync-authorized-entries-fix
branch
4 times, most recently
from
2c70eaa to
fdbf9c4
Compare
sorindumitru
requested review from
evan2645,
amartinezfayo,
azdagron,
MarcosDY and
rturner3
as code owners
sorindumitru
force-pushed
the
sync-authorized-entries-fix
branch
from
3c68348 to
ca3db54
Compare
| return true | ||
| } | ||
|
|
||
| // TODO: remove in SPIRE 1.14 |
There was a problem hiding this comment.
is there a bug to track this?
| return false | ||
| } | ||
|
|
||
| if entry.CreatedAt != revisionCreatedAt { |
There was a problem hiding this comment.
can you add comments about why this is required?
pkg/agent/client/client_test.go
Outdated
| @@ -1136,16 +1147,7 @@ type testServer struct { | |||
| } | |||
|
|
|||
| func checkAuthorizedEntryOutputMask(outputMask *types.EntryMask) error { | |||
| if diff := cmp.Diff(outputMask, &types.EntryMask{ | |||
There was a problem hiding this comment.
This is a personal preference, but could you keep the hardcoded expected mask? The reason is to prevent accidental updates to the default one. If we decide to update the default mask, we should intentionally update this one as well as a security check.
| for id in $(seq 1 512); do | ||
| ENTRIES=$(jq \ | ||
| --arg uidselector "uid:${id}"\ | ||
| --arg spiffeID "spiffe://domain.test/workload${id}"\ | ||
| '.entries[.entries| length] |= . + { | ||
| "parent_id": "spiffe://domain.test/node", | ||
| "spiffe_id": $spiffeID, | ||
| "selectors": [{"type": "unix", "value": $uidselector}] | ||
| }' <<< ${ENTRIES}) | ||
| done |
There was a problem hiding this comment.
all entry creation can be simplified with something like this:
ENTRIES=$(jq -n '{
entries: [
{
parent_id: "spiffe://domain.test/node",
spiffe_id: "spiffe://domain.test/theworkload",
selectors: [
{
type: "unix",
value: "uid:0"
}
]
},
(
range(1; 512) | {
parent_id: "spiffe://domain.test/node",
spiffe_id: ("spiffe://domain.test/workload" + (. | tostring)),
selectors: [
{
type: "unix",
value: ("uid:" + (. | tostring))
}
]
}
)
]
}')
There was a problem hiding this comment.
if you go for this, you will need to change the verified entry
| # successfully synced down the workload entry. | ||
| MAXCHECKS=30 | ||
| CHECKINTERVAL=1 | ||
| for ((i=1;i<=MAXCHECKS;i++)); do |
There was a problem hiding this comment.
you can use check-synced-entry than is doing the same
| @@ -0,0 +1,9 @@ | |||
| # Join Token Suite | |||
There was a problem hiding this comment.
looks like this readme is not updated?
|
|
||
| ## Description | ||
|
|
||
| This suite verifies that: |
There was a problem hiding this comment.
it looks tome than we must recreate an entry with the same reused id right?
and verify it is failing as expected, or recreated once we removed the old one,
maybe you can add more scripts verify that?
There was a problem hiding this comment.
I think that ends up being difficult to handle in the integration test because of timing. I've added some more checks to the unit tests to verify those conditions. Do you think those are enough or should I see about checking those in the integration test?
There was a problem hiding this comment.
yeah, it is going difficult to test the edge scenario within an IT
| bad-spire-agent: | ||
| image: spire-agent:latest-local | ||
| volumes: | ||
| - ./conf/bad-agent:/opt/spire/conf/agent | ||
| command: ["-config", "/opt/spire/conf/agent/agent.conf"] |
There was a problem hiding this comment.
Its not, just some leftovers from the test I started from. I thought I cleaned those up, but turns out i didn't
Signed-off-by: Sorin Dumitru <[email protected]>
Signed-off-by: Sorin Dumitru <[email protected]>
Signed-off-by: Sorin Dumitru <[email protected]>
Signed-off-by: Sorin Dumitru <[email protected]>
Signed-off-by: Sorin Dumitru <[email protected]>
sorindumitru
force-pushed
the
sync-authorized-entries-fix
branch
from
ca3db54 to
230cd6b
Compare
Pull Request check list
Affected functionality
Registration entry synchronization between agent and server.
Description of change
The agent will now include the created_at field when determining if a registration entry is stale or not.
Which issue this PR fixes
fixes #5522
fixes #5686
depends on spiffe/spire-api-sdk#69