Remove deprecated OmitX509SVIDUID option (spiffe#3794)

This change removes the deprecated OmitX509SVIDUID configuration option ahead of the 1.6.0 release. This option was added in 1.5.0 as an immediately deprecated option to disable the new behavior that made each X509-SVID unique through a random UID added to the subject. Signed-off-by: Andrew Harding <[email protected]>
grameshtwilio · Jan 24, 2023 · 0d6d64c · 0d6d64c
1 parent 62a8721
commit 0d6d64c
Show file tree

Hide file tree

Showing 9 changed files with 22 additions and 113 deletions.
diff --git a/cmd/spire-server/cli/run/run.go b/cmd/spire-server/cli/run/run.go
@@ -97,8 +97,7 @@ type serverConfig struct {
 	ProfilingNames   []string `hcl:"profiling_names"`
 
 	// Deprecated: remove in SPIRE 1.6.0
-	DefaultSVIDTTL  string `hcl:"default_svid_ttl"`
-	OmitX509SVIDUID *bool  `hcl:"omit_x509svid_uid"`
+	DefaultSVIDTTL string `hcl:"default_svid_ttl"`
 
 	UnusedKeys []string `hcl:",unusedKeys"`
 }
@@ -614,11 +613,6 @@ func NewServerConfig(c *Config, logOptions []log.Option, allowUnknownConfig bool
 		sc.CASubject = defaultCASubject
 	}
 
-	if c.Server.OmitX509SVIDUID != nil {
-		sc.Log.Warn("The omit_x509svid_uid flag is deprecated and will be removed from a future release")
-		sc.OmitX509SVIDUID = *c.Server.OmitX509SVIDUID
-	}
-
 	sc.PluginConfigs = *c.Plugins
 	sc.Telemetry = c.Telemetry
 	sc.HealthChecks = c.HealthChecks

diff --git a/cmd/spire-server/cli/run/run_test.go b/cmd/spire-server/cli/run/run_test.go
@@ -1007,47 +1007,6 @@ func TestNewServerConfig(t *testing.T) {
 				}, c.AdminIDs)
 			},
 		},
-		{
-			msg: "omit_x509svid_uid is unset",
-			input: func(c *Config) {
-				c.Server.OmitX509SVIDUID = nil
-			},
-			test: func(t *testing.T, c *server.Config) {
-				require.False(t, c.OmitX509SVIDUID)
-			},
-		},
-		{
-			msg: "omit_x509svid_uid is set to false",
-			input: func(c *Config) {
-				value := false
-				c.Server.OmitX509SVIDUID = &value
-			},
-			logOptions: assertLogsContainEntries([]spiretest.LogEntry{
-				{
-					Level:   logrus.WarnLevel,
-					Message: "The omit_x509svid_uid flag is deprecated and will be removed from a future release",
-				},
-			}),
-			test: func(t *testing.T, c *server.Config) {
-				require.False(t, c.OmitX509SVIDUID)
-			},
-		},
-		{
-			msg: "omit_x509svid_uid is set to true",
-			input: func(c *Config) {
-				value := true
-				c.Server.OmitX509SVIDUID = &value
-			},
-			logOptions: assertLogsContainEntries([]spiretest.LogEntry{
-				{
-					Level:   logrus.WarnLevel,
-					Message: "The omit_x509svid_uid flag is deprecated and will be removed from a future release",
-				},
-			}),
-			test: func(t *testing.T, c *server.Config) {
-				require.True(t, c.OmitX509SVIDUID)
-			},
-		},
 	}
 	cases = append(cases, newServerConfigCasesOS()...)
 

diff --git a/conf/server/server_full.conf b/conf/server/server_full.conf
@@ -146,11 +146,6 @@ server {
     # default_svid_ttl: The default SVID TTL. Default: 1h.
     # default_svid_ttl = "1h"
 
-    # omit_x509svid_uid: If true, the subject on X509-SVIDs will not contain
-    # the unique ID attribute. This configurable is deprecated and will be
-    # removed from a future release.
-    # omit_x509svid_uid = false
-
     # trust_domain: The trust domain that this server belongs to.
     trust_domain = "example.org"
 

diff --git a/doc/spire_server.md b/doc/spire_server.md
@@ -69,7 +69,6 @@ This may be useful for templating configuration files, for example across differ
 | `log_file`              | File to write logs to                                                                                                                                                                                                                               |                                                                |
 | `log_level`             | Sets the logging level &lt;DEBUG&vert;INFO&vert;WARN&vert;ERROR&gt;                                                                                                                                                                                 | INFO                                                           |
 | `log_format`            | Format of logs, &lt;text&vert;json&gt;                                                                                                                                                                                                              | text                                                           |
-| `omit_x509svid_uid`     | If true, the subject on X509-SVIDs will not contain the unique ID attribute (deprecated)                                                                                                                                                            | false                                                          |
 | `profiling_enabled`     | If true, enables a [net/http/pprof](https://pkg.go.dev/net/http/pprof) endpoint                                                                                                                                                                     | false                                                          |
 | `profiling_freq`        | Frequency of dumping profiling data to disk. Only enabled when `profiling_enabled` is `true` and `profiling_freq` > 0.                                                                                                                              |                                                                |
 | `profiling_names`       | List of profile names that will be dumped to disk on each profiling tick, see [Profiling Names](#profiling-names)                                                                                                                                   |                                                                |

diff --git a/pkg/server/ca/ca.go b/pkg/server/ca/ca.go
@@ -111,16 +111,15 @@ type JWTKey struct {
 }
 
 type Config struct {
-	Log             logrus.FieldLogger
-	Metrics         telemetry.Metrics
-	TrustDomain     spiffeid.TrustDomain
-	X509SVIDTTL     time.Duration
-	JWTSVIDTTL      time.Duration
-	JWTIssuer       string
-	Clock           clock.Clock
-	CASubject       pkix.Name
-	HealthChecker   health.Checker
-	OmitX509SVIDUID bool
+	Log           logrus.FieldLogger
+	Metrics       telemetry.Metrics
+	TrustDomain   spiffeid.TrustDomain
+	X509SVIDTTL   time.Duration
+	JWTSVIDTTL    time.Duration
+	JWTIssuer     string
+	Clock         clock.Clock
+	CASubject     pkix.Name
+	HealthChecker health.Checker
 }
 
 type CA struct {
@@ -196,7 +195,7 @@ func (ca *CA) SignX509SVID(ctx context.Context, params X509SVIDParams) ([]*x509.
 
 	notBefore, notAfter := ca.capLifetime(params.TTL, x509CA.Certificate.NotAfter)
 
-	x509SVID, err := signX509SVID(ca.c.TrustDomain, x509CA, params, notBefore, notAfter, ca.c.OmitX509SVIDUID)
+	x509SVID, err := signX509SVID(ca.c.TrustDomain, x509CA, params, notBefore, notAfter)
 	if err != nil {
 		return nil, err
 	}
@@ -281,7 +280,7 @@ func (ca *CA) capLifetime(ttl time.Duration, expirationCap time.Time) (notBefore
 	return notBefore, notAfter
 }
 
-func signX509SVID(td spiffeid.TrustDomain, x509CA *X509CA, params X509SVIDParams, notBefore, notAfter time.Time, omitUID bool) ([]*x509.Certificate, error) {
+func signX509SVID(td spiffeid.TrustDomain, x509CA *X509CA, params X509SVIDParams, notBefore, notAfter time.Time) ([]*x509.Certificate, error) {
 	if x509CA == nil {
 		return nil, errs.New("X509 CA is not available for signing")
 	}
@@ -305,10 +304,8 @@ func signX509SVID(td spiffeid.TrustDomain, x509CA *X509CA, params X509SVIDParams
 		}
 	}
 
-	// Append the unique ID to the subject, unless disabled
-	if !omitUID {
-		template.Subject.ExtraNames = append(template.Subject.ExtraNames, x509svid.UniqueIDAttribute(params.SpiffeID))
-	}
+	// Append the unique ID to the subject.
+	template.Subject.ExtraNames = append(template.Subject.ExtraNames, x509svid.UniqueIDAttribute(params.SpiffeID))
 
 	// Explicitly set the AKI on the signed certificate, otherwise it won't be
 	// added if the subject and issuer match name match (however unlikely).

diff --git a/pkg/server/ca/ca_test.go b/pkg/server/ca/ca_test.go
@@ -427,36 +427,6 @@ func (s *CATestSuite) createCACertificate(cn string, parent *x509.Certificate) *
 	return createCACertificate(s.T(), s.clock, cn, parent)
 }
 
-func TestOmitX509SVIDUID(t *testing.T) {
-	clk := clock.NewMock(t)
-	log, _ := test.NewNullLogger()
-
-	ca := NewCA(Config{
-		Log:         log,
-		Metrics:     telemetry.Blackhole{},
-		TrustDomain: trustDomainExample,
-		X509SVIDTTL: time.Minute,
-		Clock:       clk,
-		CASubject: pkix.Name{
-			CommonName: "TESTCA",
-		},
-		HealthChecker:   fakehealthchecker.New(),
-		OmitX509SVIDUID: true,
-	})
-	ca.SetX509CA(&X509CA{
-		Signer:      testSigner,
-		Certificate: createCACertificate(t, clk, "CA", nil),
-	})
-
-	certs, err := ca.SignX509SVID(context.Background(), X509SVIDParams{
-		SpiffeID:  spiffeid.RequireFromString("spiffe://example.org/workload"),
-		PublicKey: testSigner.Public(),
-	})
-	require.NoError(t, err)
-	require.Len(t, certs, 1)
-	require.Equal(t, "O=SPIRE,C=US", certs[0].Subject.String())
-}
-
 func createCACertificate(t *testing.T, clk clock.Clock, cn string, parent *x509.Certificate) *x509.Certificate {
 	keyID, err := x509util.GetSubjectKeyID(testSigner.Public())
 	require.NoError(t, err)

diff --git a/pkg/server/ca/validation.go b/pkg/server/ca/validation.go
@@ -46,7 +46,7 @@ func (v X509CAValidator) validateX509CA(x509CA *x509.Certificate, x509Roots, ups
 		Signer:        v.Signer,
 		Certificate:   x509CA,
 		UpstreamChain: upstreamChain,
-	}, params, x509CA.NotBefore, x509CA.NotAfter, false)
+	}, params, x509CA.NotBefore, x509CA.NotAfter)
 	if err != nil {
 		return fmt.Errorf("unable to sign throwaway SVID for X509 CA validation: %w", err)
 	}

diff --git a/pkg/server/config.go b/pkg/server/config.go
@@ -104,10 +104,6 @@ type Config struct {
 	// AdminIDs are a list of fixed IDs that when presented by a caller in an
 	// X509-SVID, are granted admin rights.
 	AdminIDs []spiffeid.ID
-
-	// OmitX509SVIDUID, if true, omits the X.500 Unique Identifier from being
-	// calculated and added to the Subject DN on X509-SVIDs.
-	OmitX509SVIDUID bool
 }
 
 type ExperimentalConfig struct {

diff --git a/pkg/server/server.go b/pkg/server/server.go
@@ -264,14 +264,13 @@ func (s *Server) loadCatalog(ctx context.Context, metrics telemetry.Metrics, ide
 
 func (s *Server) newCA(metrics telemetry.Metrics, healthChecker health.Checker) *ca.CA {
 	return ca.NewCA(ca.Config{
-		Metrics:         metrics,
-		X509SVIDTTL:     s.config.X509SVIDTTL,
-		JWTSVIDTTL:      s.config.JWTSVIDTTL,
-		JWTIssuer:       s.config.JWTIssuer,
-		TrustDomain:     s.config.TrustDomain,
-		CASubject:       s.config.CASubject,
-		HealthChecker:   healthChecker,
-		OmitX509SVIDUID: s.config.OmitX509SVIDUID,
+		Metrics:       metrics,
+		X509SVIDTTL:   s.config.X509SVIDTTL,
+		JWTSVIDTTL:    s.config.JWTSVIDTTL,
+		JWTIssuer:     s.config.JWTIssuer,
+		TrustDomain:   s.config.TrustDomain,
+		CASubject:     s.config.CASubject,
+		HealthChecker: healthChecker,
 	})
 }