Skip to content
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

Remove deprecated OmitX509SVIDUID option #3794

Merged
merged 2 commits into from
Jan 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 1 addition & 7 deletions cmd/spire-server/cli/run/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}
Expand Down Expand Up @@ -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
Expand Down
41 changes: 0 additions & 41 deletions cmd/spire-server/cli/run/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()...)

Expand Down
5 changes: 0 additions & 5 deletions conf/server/server_full.conf
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
1 change: 0 additions & 1 deletion doc/spire_server.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <DEBUG|INFO|WARN|ERROR> | INFO |
| `log_format` | Format of logs, <text|json> | 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) | |
Expand Down
29 changes: 13 additions & 16 deletions pkg/server/ca/ca.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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")
}
Expand All @@ -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).
Expand Down
30 changes: 0 additions & 30 deletions pkg/server/ca/ca_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/ca/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
4 changes: 0 additions & 4 deletions pkg/server/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
15 changes: 7 additions & 8 deletions pkg/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
}

Expand Down