Skip to content

Return an error if an SDS client asks for resources that don't exist.#2020

Merged
azdagron merged 6 commits intospiffe:masterfrom
transferwise:disconnect-sds-when-no-identities
Jan 8, 2021
Merged

Return an error if an SDS client asks for resources that don't exist.#2020
azdagron merged 6 commits intospiffe:masterfrom
transferwise:disconnect-sds-when-no-identities

Conversation

@JonathanO
Copy link
Contributor

This will reject an SDS client if it requests resource names that don't exist.

This is a solution for SDS clients getting "stuck" if attestation fails temporarily due to some local problem (e.g. k8s attestation taking too long.)

This may cause problems if anyone is relying on a configuration where it is expected to request multiple identities/bundles in the same SDS session, some of which do not exist (yet.)

fixes #1230

@JonathanO
Copy link
Contributor Author

The alternative to this PR would be to simply hang up after attestation (or on first request) if no identities were issued. That would also solve the problem, and, unlike this PR, it matches the behaviour of the workload API. But I am concerned that there could be people depending on having workloads with no identities retrieving trust bundles via SDS, which such a change would break. If that isn't actually a supported use case then I'll close this PR, and raise a new one with the simpler fix.

@JonathanO JonathanO force-pushed the disconnect-sds-when-no-identities branch from 40970b7 to adcf2d4 Compare January 4, 2021 13:12
@azdagron
Copy link
Member

azdagron commented Jan 4, 2021

The alternative seems interesting and I'd probably be for it except the Workload API specification has recently gained an RPC that can be used to fetch X.509 bundles (optionally without requiring the workload to be attested) in order to support verifying SPIFFE IDs from a trust domain without having to actually belong to the trust domain (we have yet to implement this in SPIRE), so from a behavior parity standpoint, I think it makes most sense to continue allowing a bundle to be returned to an unauthenticated workload.

@evan2645
Copy link
Member

evan2645 commented Jan 4, 2021

TL;DR: I think this approach is alright for the time being

I'm still not in love with the whole unregistered workload api consumer thing... Putting that aside, this SDS thing needs to be fixed one way or another. I've considered this approach vs the alternative (which feels better on the surface), and ultimately I agree with @azdagron and @JonathanO - this approach seems unlikely to fail in strange ways, and also unlikely to introduce pain for existing consumers that may be using the agent in surprising ways. At the same time, it doesn't prevent us from moving to the alternative behavior in the future, so... 👍

azdagron
azdagron previously approved these changes Jan 8, 2021
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

This looks good to me. I left a few nitpicks to make the code a little more terse but don't feel obligated to fix them. Happy to merge without.

JonathanO and others added 3 commits January 8, 2021 14:02
This fixes spiffe#1230

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
Co-authored-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
@JonathanO JonathanO force-pushed the disconnect-sds-when-no-identities branch from 5d58f4a to ad14140 Compare January 8, 2021 14:03
Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
This is Circle's recommended image, and should contain a version of docker-compose that supports v3.5 compose files.

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

Thanks, @JonathanO !

@azdagron azdagron merged commit e6bbb5d into spiffe:master Jan 8, 2021
@JonathanO JonathanO deleted the disconnect-sds-when-no-identities branch January 8, 2021 17:34
@azdagron azdagron added this to the 0.12.2 milestone Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Agent's SDS server should hang up if no SVIDs are available

3 participants