Skip to content

Add flag guarding SPIFFE Bundle provider #8343

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

Merged
merged 14 commits into from
Jun 4, 2025
+90 −5
Merged

Add flag guarding SPIFFE Bundle provider #8343

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1b6e905
Add flag guarding SPIFFE Bundle provider
gtcooke94 May 19, 2025
dd60f60
remove the log
gtcooke94 May 19, 2025
7c30683
vet
gtcooke94 May 19, 2025
fb77e9d
address PR comments
gtcooke94 May 21, 2025
a96c031
add comment
gtcooke94 May 21, 2025
94cae8f
fix typo
gtcooke94 May 21, 2025
1cb982a
rename flag
gtcooke94 May 28, 2025
2762209
add test
gtcooke94 May 28, 2025
7007700
vet
gtcooke94 May 28, 2025
29e82a3
add other flag check
gtcooke94 May 28, 2025
b78b892
Merge branch 'master' into spiffe_flag
gtcooke94 Jun 4, 2025
2de3163
remove check from watcher
gtcooke94 Jun 4, 2025
b3e092d
add tests for new section where the spiffe bundle map file is set to …
gtcooke94 Jun 4, 2025
b2a6bdb
vet
gtcooke94 Jun 4, 2025
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
5 changes: 5 additions & 0 deletions credentials/tls/certprovider/pemfile/watcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
"google.golang.org/grpc/credentials/tls/certprovider"
"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/internal/credentials/spiffe"
"google.golang.org/grpc/internal/envconfig"
)

const defaultCertRefreshDuration = 1 * time.Hour
Expand Down Expand Up @@ -78,6 +79,10 @@ func (o Options) canonical() []byte {
}

func (o Options) validate() error {
// Guard against SPIFFE bundle map usage
Copy link
Contributor

Choose a reason for hiding this comment

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

My reading of this sentence from the gRFC is that we don't even read the new field spiffe_trust_bundle_map_file from the bootstrap. So, this might need to be handled in certprovider/pemfile/builder.go in pluginConfigFromJSON instead of here. Because by the time we get here, we have already read the field from the bootstrap JSON.

"""
The xDS functionality will be guarded via the GRPC_EXPERIMENTAL_XDS_MTLS_SPIFFE environment variable. The new bootstrap field will not be read if this env var is unset. This env var guard will be removed when the feature has passed interop tests.
"""

if !envconfig.XDSSpiffeEnabled {
o.SPIFFEBundleMapFile = ""
}
if o.CertFile == "" && o.KeyFile == "" && o.RootFile == "" && o.SPIFFEBundleMapFile == "" {
return fmt.Errorf("pemfile: at least one credential file needs to be specified")
}
Expand Down
2 changes: 2 additions & 0 deletions credentials/tls/certprovider/pemfile/watcher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (

"github.com/google/go-cmp/cmp"
"google.golang.org/grpc/credentials/tls/certprovider"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/grpctest"
"google.golang.org/grpc/internal/testutils"
"google.golang.org/grpc/testdata"
Expand Down Expand Up @@ -76,6 +77,7 @@ func compareKeyMaterial(got, want *certprovider.KeyMaterial) error {

// TestNewProvider tests the NewProvider() function with different inputs.
func (s) TestNewProvider(t *testing.T) {
testutils.SetEnvConfig(t, &envconfig.XDSSpiffeEnabled, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need a test case where the env var is set to false, but the bootstrap field is set, and we verify that the SPIFFE bundle is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tests := []struct {
desc string
options Options
Expand Down
2 changes: 2 additions & 0 deletions internal/envconfig/xds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,6 @@
// For more details, see:
// https://github.com/grpc/proposal/blob/master/A82-xds-system-root-certs.md.
XDSSystemRootCertsEnabled = boolFromEnv("GRPC_EXPERIMENTAL_XDS_SYSTEM_ROOT_CERTS", false)

XDSSpiffeEnabled = boolFromEnv("GRPC_EXPERIMENTAL_XDS_MTLS_SPIFFE", false)

Check failure on line 67 in internal/envconfig/xds.go

View workflow job for this annotation

GitHub Actions / tests (vet, 1.23) 

exported var XDSSpiffeEnabled should have comment or be unexported https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md#exported
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be XDSSPIFFEEnabled as per go/go-style/decisions#initialisms. I know this option is not more readable compared to what you have, but this is what the style guide recommends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)
4 changes: 4 additions & 0 deletions internal/xds/bootstrap/tlscreds/bundle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"google.golang.org/grpc/credentials/tls/certprovider"
"google.golang.org/grpc/credentials/tls/certprovider/pemfile"
"google.golang.org/grpc/internal/credentials/spiffe"
"google.golang.org/grpc/internal/envconfig"
)

// bundle is an implementation of credentials.Bundle which implements mTLS
Expand Down Expand Up @@ -64,6 +65,9 @@ func NewBundle(jd json.RawMessage) (credentials.Bundle, func(), error) {
}
} // Else the config field is absent. Treat it as an empty config.

if !envconfig.XDSSpiffeEnabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

This functionality was added as part of A65, right? https://github.com/grpc/proposal/blob/master/A65-xds-mtls-creds-in-bootstrap.md

And that gRFC says that we will not guard this with an environment variable. Am I missing something?

Even if we think we need to guard here with the same env variable, we probably need to do the same thing that I mentioned in the other comment. i.e., not even read the field if the env var is set to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This support for SPIFFE Bundles is added as part of A87 - https://github.com/grpc/proposal/blob/master/A87-mtls-spiffe-support.md

cfg.SPIFFETrustBundleMapFile = ""
}
if cfg.CACertificateFile == "" && cfg.CertificateFile == "" && cfg.PrivateKeyFile == "" && cfg.SPIFFETrustBundleMapFile == "" {
// We cannot use (and do not need) a file_watcher provider in this case,
// and can simply directly use the TLS transport credentials.
Expand Down
2 changes: 2 additions & 0 deletions internal/xds/bootstrap/tlscreds/bundle_ext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (

"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/grpctest"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/internal/testutils"
Expand Down Expand Up @@ -237,6 +238,7 @@ func (s) TestCaReloading(t *testing.T) {
// is performed and checked for failure, ensuring that gRPC is correctly using
// the changed-on-disk bundle map.
func (s) Test_SPIFFE_Reloading(t *testing.T) {
testutils.SetEnvConfig(t, &envconfig.XDSSpiffeEnabled, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

And same here. If we are going to guard this with the env var, we need to have a test case where the env var is set to false.

clientSPIFFEBundle, err := os.ReadFile(testdata.Path("spiffe_end2end/client_spiffebundle.json"))
if err != nil {
t.Fatalf("Failed to read test SPIFFE bundle: %v", err)
Expand Down
Loading