Unexpected default X509 SVID TTL

[dennisgove]

PR #3445 added support for X509 and JWT specific SVID TTLs. As part of this, the spire-server binary config arguments were updated to include all of

spire/cmd/spire-server/cli/run/run.go

Lines 77 to 79 in d98577f

	DefaultSVIDTTL string `hcl:"default_svid_ttl"`
	DefaultX509SVIDTTL string `hcl:"default_x509_svid_ttl"`
	DefaultJWTSVIDTTL string `hcl:"default_jwt_svid_ttl"`

The following lines set the config defaults for the X509 and JWT SVIDs

spire/cmd/spire-server/cli/run/run.go

Lines 839 to 840 in d98577f

	DefaultX509SVIDTTL: ca.DefaultX509SVIDTTL.String(),
	DefaultJWTSVIDTTL: ca.DefaultJWTSVIDTTL.String(),

The result is that the values for DefaultX509SVIDTTL and DefaultJWTSVIDTTL are always set to some value even if the startup config file does not set those.

On face this seems reasonable, but an issue appears if the startup config instead sets DefaultSVIDTTL and neither of the new X509/JWT specific values.

In such a case the following blocks will always be executed and the desired DefaultSVIDTTL will be ignored.

spire/cmd/spire-server/cli/run/run.go

Lines 472 to 481 in d98577f

	if c.Server.DefaultX509SVIDTTL != "" {
	ttl, err := time.ParseDuration(c.Server.DefaultX509SVIDTTL)
	if err != nil {
	return nil, fmt.Errorf("could not parse default X509 SVID ttl %q: %w", c.Server.DefaultX509SVIDTTL, err)
	}
	sc.X509SVIDTTL = ttl
	if sc.X509SVIDTTL != 0 && c.Server.DefaultSVIDTTL != "" {
	logger.Warnf("both default_x509_svid_ttl and default_svid_ttl are configured; default_x509_svid_ttl (%s) will be used for X509-SVIDs", c.Server.DefaultX509SVIDTTL)
	}

spire/cmd/spire-server/cli/run/run.go

Lines 492 to 501 in d98577f

	if c.Server.DefaultJWTSVIDTTL != "" {
	ttl, err := time.ParseDuration(c.Server.DefaultJWTSVIDTTL)
	if err != nil {
	return nil, fmt.Errorf("could not parse default JWT SVID ttl %q: %w", c.Server.DefaultJWTSVIDTTL, err)
	}
	sc.JWTSVIDTTL = ttl
	if sc.JWTSVIDTTL != 0 && c.Server.DefaultSVIDTTL != "" {
	logger.Warnf("both default_jwt_svid_ttl and default_svid_ttl are configured; default_jwt_svid_ttl (%s) will be used for JWT-SVIDs", c.Server.DefaultJWTSVIDTTL)
	}

The result is a breaking change if someone has set the DefaultSVIDTTL value and has now updated to version 1.5.0.

For example, with this config file

server {
	...
	default_svid_ttl = "15m"
	...
}

the following will appear in the logs on startup

time="2022-11-07T19:46:50Z" level=warning msg="both default_x509_svid_ttl and default_svid_ttl are configured; default_x509_svid_ttl (1h0m0s) will be used for X509-SVIDs"
time="2022-11-07T19:46:50Z" level=warning msg="both default_jwt_svid_ttl and default_svid_ttl are configured; default_jwt_svid_ttl (5m0s) will be used for JWT-SVIDs"

[dennisgove]

@azdagron for visibility

[dennisgove]

These two lines in the test appear to be why this wasn't caught during testing. It was an issue I ran into during initial testing and resolved it incorrectly.

spire/cmd/spire-server/cli/run/run_test.go

Lines 642 to 643 in d98577f

	c.Server.DefaultX509SVIDTTL = ""
	c.Server.DefaultJWTSVIDTTL = ""

Removing those two lines shows the test now failing as it should.

time="2022-11-07T15:08:10-05:00" level=warning msg="both default_x509_svid_ttl and default_svid_ttl are configured; default_x509_svid_ttl (1h0m0s) will be used for X509-SVIDs"
time="2022-11-07T15:08:10-05:00" level=warning msg="both default_jwt_svid_ttl and default_svid_ttl are configured; default_jwt_svid_ttl (5m0s) will be used for JWT-SVIDs"
.....

--- FAIL: TestNewServerConfig (0.01s)
    --- FAIL: TestNewServerConfig/default_svid_ttl_is_correctly_parsed (0.00s)
        run_test.go:646:
            	Error Trace:	/Users/dgove1/dev/spire/spire/cmd/spire-server/cli/run/run_test.go:646
            	            				/Users/dgove1/dev/spire/spire/cmd/spire-server/cli/run/run_test.go:1074
            	Error:      	Not equal:
            	            	expected: 1m0s
            	            	actual  : 1h0m0s
            	Test:       	TestNewServerConfig/default_svid_ttl_is_correctly_parsed

[azdagron]

Yikes. We should fix this.