-
Notifications
You must be signed in to change notification settings - Fork 32
fix: [Connectivity-ZTIS] Deterministic SVID selection #1246
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
4d82889
61c94c1
add323f
6419fb8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.util.Date; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
|
|
||
| import javax.annotation.Nonnull; | ||
|
|
@@ -23,6 +24,7 @@ | |
|
|
||
| import io.spiffe.bundle.x509bundle.X509Bundle; | ||
| import io.spiffe.exception.X509SvidException; | ||
| import io.spiffe.spiffeid.SpiffeId; | ||
| import io.spiffe.spiffeid.TrustDomain; | ||
| import io.spiffe.svid.x509svid.X509Svid; | ||
| import io.spiffe.workloadapi.DefaultX509Source; | ||
|
|
@@ -109,16 +111,57 @@ X509Source initX509Source() | |
| final String socketPath = Option.of(System.getenv(SOCKET_ENVIRONMENT_VARIABLE)).getOrElse(DEFAULT_SOCKET_PATH); | ||
| log.info("Using socket path {} for ZTIS agent.", socketPath); | ||
|
|
||
| final X509SourceOptions x509SourceOptions = | ||
| X509SourceOptions.builder().spiffeSocketPath(socketPath).initTimeout(DEFAULT_SOCKET_TIMEOUT).build(); | ||
| // The SPIRE agent may return multiple SVIDs when overlapping registration selectors exist | ||
| // (e.g. a second service key on the same ZTIS instance, or a co-located workload). We must pick | ||
| // the one whose SPIFFE ID matches our binding — see pickSvid. | ||
| final SpiffeId expectedSpiffeId; | ||
| try { | ||
| return DefaultX509Source.newSource(x509SourceOptions); | ||
| expectedSpiffeId = SpiffeId.parse(mapView.getMapView("workload").getString("spiffeID")); | ||
| } | ||
| catch( Exception e ) { | ||
| throw new CloudPlatformException("Invalid SPIFFE ID in Zero Trust Identity Service binding.", e); | ||
| } | ||
| try { | ||
| return DefaultX509Source.newSource(buildX509SourceOptions(socketPath, expectedSpiffeId)); | ||
| } | ||
| catch( final Exception e ) { | ||
| throw new CloudPlatformException("Failed to load the certificate from the unix socket: " + socketPath, e); | ||
| } | ||
| } | ||
|
|
||
| X509SourceOptions | ||
| buildX509SourceOptions( @Nonnull final String socketPath, @Nonnull final SpiffeId expectedSpiffeId ) | ||
| { | ||
| return X509SourceOptions | ||
| .builder() | ||
| .spiffeSocketPath(socketPath) | ||
| .initTimeout(DEFAULT_SOCKET_TIMEOUT) | ||
| .svidPicker(svids -> pickSvid(svids, expectedSpiffeId)) | ||
| .build(); | ||
| } | ||
|
|
||
| /** | ||
| * Selects the {@link X509Svid} whose SPIFFE ID equals {@code expectedSpiffeId} from the given list. Used as the | ||
| * {@code svidPicker} for {@link DefaultX509Source} to avoid non-deterministic selection when the SPIRE agent | ||
| * returns multiple SVIDs (e.g. after a second service key is created for the same ZTIS instance). | ||
| */ | ||
| @Nonnull | ||
| static X509Svid pickSvid( @Nonnull final List<X509Svid> svids, @Nonnull final SpiffeId expectedSpiffeId ) | ||
| { | ||
| log.debug("SPIRE agent returned {} SVID(s); selecting the one matching '{}'.", svids.size(), expectedSpiffeId); | ||
| return svids | ||
| .stream() | ||
| .filter(svid -> expectedSpiffeId.equals(svid.getSpiffeId())) | ||
| .findFirst() | ||
| .orElseThrow( | ||
| () -> new CloudPlatformException( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why throw? If not found we could use the old behaviour of not setting a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Falling back to the old behavior would reproduce the bug in a smaller scope — it would only behave correctly when there's exactly one SVID, or when the library happens to pick the right one from an unordered list. That's the non-determinism we're fixing.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It only throws when it doesn’t find the svid from the list right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, just improved the tests to address the other concern regarding removing the svidPicker |
||
| String | ||
| .format( | ||
| "No SVID matching SPIFFE ID '%s' among %d returned by the SPIRE agent.", | ||
| expectedSpiffeId, | ||
| svids.size()))); | ||
| } | ||
|
|
||
| @Nonnull | ||
| X509Svid getX509Svid() | ||
| { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remove the
svidPicker, the tests are still green apart fromassertThat(picker).isNotNull();I don't think any test checks the actual change of this PR, the
svidPickerUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is the same issue discussed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then wdym it's a compromise, if you revert the production code the tests are still green