Skip to content

Use UpstreamAuthority.SubscribeToLocalBundle RPC #6090

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sorindumitru
Copy link
Collaborator

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality
UpstreamAuthority plugins.

Description of change
If the UpstreamAuthority plugin implements the GetTrustBundle RPC use it to keep the trust bundle of the trust domain in sync. This allows us to get bundle updates before we create a new CA or key.

Which issue this PR fixes
fixes #6083

@sorindumitru sorindumitru force-pushed the nested-restart branch 5 times, most recently from 85660f5 to ad8c3f6 Compare May 30, 2025 11:51
@sorindumitru sorindumitru changed the title Use UpstreamAuthority.GetTrustBundle RPC Use UpstreamAuthority.SubscribeToLocalBundle RPC May 30, 2025
@sorindumitru sorindumitru force-pushed the nested-restart branch 2 times, most recently from 89f91df to 1379d0d Compare June 1, 2025 09:53
@sorindumitru sorindumitru marked this pull request as ready for review June 1, 2025 10:38
@MarcosDY MarcosDY added this to the 1.12.3 milestone Jun 3, 2025
err := util.RunTasks(ctx,
func(ctx context.Context) error {
return r.rotateEvery(ctx, rotateInterval)
},
func(ctx context.Context) error {
var lastError error
Copy link
Collaborator

Choose a reason for hiding this comment

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

may we move this to Subscribe?
and only start when we really need to subscribe

@@ -151,6 +151,43 @@ func TestUpstreamClientPublishJWTKey_NotImplemented(t *testing.T) {
require.Nil(t, jwtKeys)
}

func TestUpstreamClientSubscribeToLocalBundle(t *testing.T) {
client, updater, ua := setUpUpstreamClientTest(t, fakeupstreamauthority.Config{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
client, updater, ua := setUpUpstreamClientTest(t, fakeupstreamauthority.Config{
client, updater, ua := setupUpstreamClientTest(t, fakeupstreamauthority.Config{

// We should get an update with the initial CA and a list of empty JWT keys since
// the fakeupstreamauthority does not create one by default.
require.Equal(t, ua.X509Roots(), updater.WaitForAppendedX509Roots(t))
spiretest.RequireProtoListEqual(t, []*common.PublicKey{}, updater.WaitForAppendedJWTKeys(t))
Copy link
Collaborator

Choose a reason for hiding this comment

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

require.Empty?

@@ -32,6 +32,16 @@ type UpstreamAuthority interface {
// the upstream authority does not support streaming updates, the stream
// will return io.EOF when called.
PublishJWTKey(ctx context.Context, jwtKey *common.PublicKey) (jwtAuthorities []*common.PublicKey, stream UpstreamJWTAuthorityStream, err error)

// GetUpstreamAuthorities can be used to sync the local trust bundle with
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// GetUpstreamAuthorities can be used to sync the local trust bundle with
// SubscribeToLocalBundle can be used to sync the local trust bundle with

}

x509Authorities, err := s.v1.parseX509Authorities(resp.UpstreamX509Roots)
if len(resp.UpstreamX509Roots) != 0 && err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why to verify roots length? parse may return no error only when there was something to parse, right?

@@ -1,5 +1,7 @@
#!/bin/bash

pwd
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you remove this?

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.

Trust bundle missing active signing keys on some clusters
2 participants