Skip to content

Commit 82e76ca

Browse files
Andrew Hardingstevend-uber
Andrew Harding
authored and
stevend-uber
committed
Change X509-SVID subject values (spiffe#3367)
* Change X509-SVID subject values As outlined in spiffe#3341, this change behavior that controls the subject of X509-SVIDs signed by the SPIRE Server CA. Instead of a fixed subject (i.e. "O=SPIRE,C=US"), the subject now also has a UniqueID attribute. The Unique ID is per SPIFFE ID. This brings us into RFC 5280 conformance. The new behavior can be disabled with an immediately deprecated flag, "omit_x509svid_uid". This flag will be removed in a future release.
1 parent 5cebee6 commit 82e76ca

File tree

13 files changed

+240
-72
lines changed

13 files changed

+240
-72
lines changed

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,11 @@ type serverConfig struct {
8282
LogFile string `hcl:"log_file"`
8383
LogLevel string `hcl:"log_level"`
8484
LogFormat string `hcl:"log_format"`
85-
RateLimit rateLimitConfig `hcl:"ratelimit"`
86-
SocketPath string `hcl:"socket_path"`
87-
TrustDomain string `hcl:"trust_domain"`
85+
// Deprecated: remove in SPIRE 1.6.0
86+
OmitX509SVIDUID *bool `hcl:"omit_x509svid_uid"`
87+
RateLimit rateLimitConfig `hcl:"ratelimit"`
88+
SocketPath string `hcl:"socket_path"`
89+
TrustDomain string `hcl:"trust_domain"`
8890

8991
ConfigPath string
9092
ExpandEnv bool
@@ -563,6 +565,11 @@ func NewServerConfig(c *Config, logOptions []log.Option, allowUnknownConfig bool
563565
sc.CASubject = defaultCASubject
564566
}
565567

568+
if c.Server.OmitX509SVIDUID != nil {
569+
sc.Log.Warn("The omit_x509svid_uid flag is deprecated and will be removed from a future release")
570+
sc.OmitX509SVIDUID = *c.Server.OmitX509SVIDUID
571+
}
572+
566573
sc.PluginConfigs = *c.Plugins
567574
sc.Telemetry = c.Telemetry
568575
sc.HealthChecks = c.HealthChecks

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

Lines changed: 64 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,21 @@ func TestMergeInput(t *testing.T) {
430430
}
431431

432432
func TestNewServerConfig(t *testing.T) {
433+
assertLogsContainEntries := func(expectedEntries []spiretest.LogEntry) func(t *testing.T) []log.Option {
434+
return func(t *testing.T) []log.Option {
435+
return []log.Option{
436+
func(logger *log.Logger) error {
437+
logger.SetOutput(io.Discard)
438+
hook := test.NewLocal(logger.Logger)
439+
t.Cleanup(func() {
440+
spiretest.AssertLogsContainEntries(t, hook.AllEntries(), expectedEntries)
441+
})
442+
return nil
443+
},
444+
}
445+
}
446+
}
447+
433448
cases := []newServerConfigCase{
434449
{
435450
msg: "bind_address and bind_port should be correctly parsed",
@@ -831,25 +846,14 @@ func TestNewServerConfig(t *testing.T) {
831846
input: func(c *Config) {
832847
c.Server.TrustDomain = strings.Repeat("a", 256)
833848
},
834-
logOptions: func(t *testing.T) []log.Option {
835-
return []log.Option{
836-
func(logger *log.Logger) error {
837-
logger.SetOutput(io.Discard)
838-
hook := test.NewLocal(logger.Logger)
839-
t.Cleanup(func() {
840-
spiretest.AssertLogsContainEntries(t, hook.AllEntries(), []spiretest.LogEntry{
841-
{
842-
Data: map[string]interface{}{"trust_domain": strings.Repeat("a", 256)},
843-
Level: logrus.WarnLevel,
844-
Message: "Configured trust domain name should be less than 255 characters to be " +
845-
"SPIFFE compliant; a longer trust domain name may impact interoperability",
846-
},
847-
})
848-
})
849-
return nil
850-
},
851-
}
852-
},
849+
logOptions: assertLogsContainEntries([]spiretest.LogEntry{
850+
{
851+
Data: map[string]interface{}{"trust_domain": strings.Repeat("a", 256)},
852+
Level: logrus.WarnLevel,
853+
Message: "Configured trust domain name should be less than 255 characters to be " +
854+
"SPIFFE compliant; a longer trust domain name may impact interoperability",
855+
},
856+
}),
853857
test: func(t *testing.T, c *server.Config) {
854858
assert.NotNil(t, c)
855859
},
@@ -918,6 +922,47 @@ func TestNewServerConfig(t *testing.T) {
918922
require.Nil(t, c)
919923
},
920924
},
925+
{
926+
msg: "omit_x509svid_uid is unset",
927+
input: func(c *Config) {
928+
c.Server.OmitX509SVIDUID = nil
929+
},
930+
test: func(t *testing.T, c *server.Config) {
931+
require.False(t, c.OmitX509SVIDUID)
932+
},
933+
},
934+
{
935+
msg: "omit_x509svid_uid is set to false",
936+
input: func(c *Config) {
937+
value := false
938+
c.Server.OmitX509SVIDUID = &value
939+
},
940+
logOptions: assertLogsContainEntries([]spiretest.LogEntry{
941+
{
942+
Level: logrus.WarnLevel,
943+
Message: "The omit_x509svid_uid flag is deprecated and will be removed from a future release",
944+
},
945+
}),
946+
test: func(t *testing.T, c *server.Config) {
947+
require.False(t, c.OmitX509SVIDUID)
948+
},
949+
},
950+
{
951+
msg: "omit_x509svid_uid is set to true",
952+
input: func(c *Config) {
953+
value := true
954+
c.Server.OmitX509SVIDUID = &value
955+
},
956+
logOptions: assertLogsContainEntries([]spiretest.LogEntry{
957+
{
958+
Level: logrus.WarnLevel,
959+
Message: "The omit_x509svid_uid flag is deprecated and will be removed from a future release",
960+
},
961+
}),
962+
test: func(t *testing.T, c *server.Config) {
963+
require.True(t, c.OmitX509SVIDUID)
964+
},
965+
},
921966
}
922967
cases = append(cases, newServerConfigCasesOS()...)
923968

‎conf/server/server_full.conf‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,11 @@ server {
145145

146146
# default_svid_ttl: The default SVID TTL. Default: 1h.
147147
# default_svid_ttl = "1h"
148+
149+
# omit_x509svid_uid: If true, the subject on X509-SVIDs will not contain
150+
# the unique ID attribute. This configurable is deprecated and will be
151+
# removed from a future release.
152+
# omit_x509svid_uid = false
148153

149154
# trust_domain: The trust domain that this server belongs to.
150155
trust_domain = "example.org"

‎doc/spire_server.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ This may be useful for templating configuration files, for example across differ
6969
| `log_file` | File to write logs to | |
7070
| `log_level` | Sets the logging level \<DEBUG\|INFO\|WARN\|ERROR\> | INFO |
7171
| `log_format` | Format of logs, \<text\|json\> | text |
72+
| `omit_x509svid_uid` | If true, the subject on X509-SVIDs will not contain the unique ID attribute (deprecated) | false |
7273
| `profiling_enabled` | If true, enables a [net/http/pprof](https://pkg.go.dev/net/http/pprof) endpoint | false |
7374
| `profiling_freq` | Frequency of dumping profiling data to disk. Only enabled when `profiling_enabled` is `true` and `profiling_freq` > 0. | |
7475
| `profiling_names` | List of profile names that will be dumped to disk on each profiling tick, see [Profiling Names](#profiling-names) | |

‎pkg/common/x509svid/uniqueid.go‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
package x509svid
2+
3+
import (
4+
"crypto/sha256"
5+
"crypto/x509/pkix"
6+
"encoding/asn1"
7+
"encoding/hex"
8+
"io"
9+
10+
"github.com/spiffe/go-spiffe/v2/spiffeid"
11+
)
12+
13+
var (
14+
uniqueIDOID = asn1.ObjectIdentifier{2, 5, 4, 45}
15+
)
16+
17+
// UniqueIDAttribute returns a X.500 Unique ID attribute (OID 2.5.4.45) for the
18+
// given SPIFFE ID for inclusion in an X509-SVID to satisfy RFC 5280
19+
// requirements that the subject "DN MUST be unique for each subject entity
20+
// certified by the one CA as defined by the issuer field" (see issue #3110 for
21+
// the discussion on this).
22+
//
23+
// The unique ID is composed of a SHA256 hash of the SPIFFE ID, truncated to
24+
// 128-bits (16 bytes), and then hex encoded. This *SHOULD* be large enough to
25+
// provide collision resistance on the input domain (i.e. registration entry
26+
// SPIFFE IDs registered with this server), which ranges from very- to
27+
// somewhat-restricted depending on the registration scheme and how much
28+
// influence an attacker can have on workload registration.
29+
func UniqueIDAttribute(id spiffeid.ID) pkix.AttributeTypeAndValue {
30+
return pkix.AttributeTypeAndValue{
31+
Type: uniqueIDOID,
32+
Value: calculateUniqueIDValue(id),
33+
}
34+
}
35+
36+
func calculateUniqueIDValue(id spiffeid.ID) string {
37+
h := sha256.New()
38+
_, _ = io.WriteString(h, id.String())
39+
sum := h.Sum(nil)
40+
return hex.EncodeToString(sum[:len(sum)/2])
41+
}

‎pkg/common/x509svid/uniqueid_test.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
package x509svid
2+
3+
import (
4+
"crypto/x509/pkix"
5+
"testing"
6+
7+
"github.com/spiffe/go-spiffe/v2/spiffeid"
8+
"github.com/stretchr/testify/require"
9+
)
10+
11+
func TestUniqueIDAttribute(t *testing.T) {
12+
name := pkix.Name{
13+
Names: []pkix.AttributeTypeAndValue{
14+
UniqueIDAttribute(spiffeid.RequireFromString("spiffe://example.org/foo")),
15+
},
16+
}
17+
require.Equal(t,
18+
"2.5.4.45=#13206333343036663962313263656234663963393438333138633537396239303562",
19+
name.String())
20+
}

‎pkg/server/api/svid/v1/service_test.go‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"github.com/spiffe/spire-api-sdk/proto/spire/api/types"
2121
"github.com/spiffe/spire/pkg/common/idutil"
2222
"github.com/spiffe/spire/pkg/common/telemetry"
23+
"github.com/spiffe/spire/pkg/common/x509svid"
2324
"github.com/spiffe/spire/pkg/common/x509util"
2425
"github.com/spiffe/spire/pkg/server/api"
2526
"github.com/spiffe/spire/pkg/server/api/middleware"
@@ -76,7 +77,7 @@ func TestServiceMintX509SVID(t *testing.T) {
7677
URIs: []*url.URL{workloadID.URL()},
7778
},
7879
expiredAt: expiredAt,
79-
subject: "O=SPIRE,C=US",
80+
subject: "O=SPIRE,C=US,2.5.4.45=#13203835323763353230323837636461376436323561613834373664386538336561",
8081
expectLogs: func(csr []byte) []spiretest.LogEntry {
8182
return []spiretest.LogEntry{
8283
{
@@ -102,7 +103,7 @@ func TestServiceMintX509SVID(t *testing.T) {
102103
URIs: []*url.URL{workloadID.URL()},
103104
},
104105
expiredAt: customExpiresAt,
105-
subject: "O=SPIRE,C=US",
106+
subject: "O=SPIRE,C=US,2.5.4.45=#13203835323763353230323837636461376436323561613834373664386538336561",
106107
ttl: 10 * time.Second,
107108
expectLogs: func(csr []byte) []spiretest.LogEntry {
108109
return []spiretest.LogEntry{
@@ -131,7 +132,7 @@ func TestServiceMintX509SVID(t *testing.T) {
131132
},
132133
dns: []string{"dns1", "dns2"},
133134
expiredAt: expiredAt,
134-
subject: "CN=dns1,O=SPIRE,C=US",
135+
subject: "CN=dns1,O=SPIRE,C=US,2.5.4.45=#13203835323763353230323837636461376436323561613834373664386538336561",
135136
expectLogs: func(csr []byte) []spiretest.LogEntry {
136137
return []spiretest.LogEntry{
137138
{
@@ -161,7 +162,7 @@ func TestServiceMintX509SVID(t *testing.T) {
161162
},
162163
},
163164
expiredAt: expiredAt,
164-
subject: "O=ORG,C=EN+C=US",
165+
subject: "O=ORG,C=EN+C=US,2.5.4.45=#13203835323763353230323837636461376436323561613834373664386538336561",
165166
expectLogs: func(csr []byte) []spiretest.LogEntry {
166167
return []spiretest.LogEntry{
167168
{
@@ -193,7 +194,7 @@ func TestServiceMintX509SVID(t *testing.T) {
193194
},
194195
dns: []string{"dns1", "dns2"},
195196
expiredAt: expiredAt,
196-
subject: "CN=dns1,O=ORG,C=EN+C=US",
197+
subject: "CN=dns1,O=ORG,C=EN+C=US,2.5.4.45=#13203835323763353230323837636461376436323561613834373664386538336561",
197198
expectLogs: func(csr []byte) []spiretest.LogEntry {
198199
return []spiretest.LogEntry{
199200
{
@@ -1719,8 +1720,8 @@ func TestServiceBatchNewX509SVID(t *testing.T) {
17191720
require.NotEmpty(t, certChain)
17201721
svid := certChain[0]
17211722

1722-
entryID := idutil.RequireIDFromProto(entry.SpiffeId)
1723-
require.Equal(t, []*url.URL{entryID.URL()}, svid.URIs)
1723+
entrySPIFFEID := idutil.RequireIDFromProto(entry.SpiffeId)
1724+
require.Equal(t, []*url.URL{entrySPIFFEID.URL()}, svid.URIs)
17241725

17251726
// Use entry ttl when defined
17261727
ttl := test.ca.X509SVIDTTL()
@@ -1734,14 +1735,16 @@ func TestServiceBatchNewX509SVID(t *testing.T) {
17341735

17351736
require.Equal(t, entry.DnsNames, svid.DNSNames)
17361737

1737-
expectedSubject := &pkix.Name{Country: []string{"US"}, Organization: []string{"SPIRE"}}
1738+
expectedSubject := &pkix.Name{
1739+
Organization: []string{"SPIRE"},
1740+
Country: []string{"US"},
1741+
Names: []pkix.AttributeTypeAndValue{
1742+
x509svid.UniqueIDAttribute(entrySPIFFEID),
1743+
},
1744+
}
17381745
if len(entry.DnsNames) > 0 {
1739-
name := entry.DnsNames[0]
1740-
1741-
expectedSubject.CommonName = name
1742-
require.Equal(t, name, svid.Subject.CommonName)
1746+
expectedSubject.CommonName = entry.DnsNames[0]
17431747
}
1744-
17451748
require.Equal(t, expectedSubject.String(), svid.Subject.String())
17461749
}
17471750
})

‎pkg/server/ca/ca.go‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/spiffe/spire/pkg/common/jwtsvid"
1818
"github.com/spiffe/spire/pkg/common/telemetry"
1919
telemetry_server "github.com/spiffe/spire/pkg/common/telemetry/server"
20+
"github.com/spiffe/spire/pkg/common/x509svid"
2021
"github.com/spiffe/spire/pkg/common/x509util"
2122
"github.com/spiffe/spire/pkg/server/api"
2223
"github.com/zeebo/errs"
@@ -110,15 +111,16 @@ type JWTKey struct {
110111
}
111112

112113
type Config struct {
113-
Log logrus.FieldLogger
114-
Metrics telemetry.Metrics
115-
TrustDomain spiffeid.TrustDomain
116-
X509SVIDTTL time.Duration
117-
JWTSVIDTTL time.Duration
118-
JWTIssuer string
119-
Clock clock.Clock
120-
CASubject pkix.Name
121-
HealthChecker health.Checker
114+
Log logrus.FieldLogger
115+
Metrics telemetry.Metrics
116+
TrustDomain spiffeid.TrustDomain
117+
X509SVIDTTL time.Duration
118+
JWTSVIDTTL time.Duration
119+
JWTIssuer string
120+
Clock clock.Clock
121+
CASubject pkix.Name
122+
HealthChecker health.Checker
123+
OmitX509SVIDUID bool
122124
}
123125

124126
type CA struct {
@@ -194,7 +196,7 @@ func (ca *CA) SignX509SVID(ctx context.Context, params X509SVIDParams) ([]*x509.
194196

195197
notBefore, notAfter := ca.capLifetime(params.TTL, x509CA.Certificate.NotAfter)
196198

197-
x509SVID, err := signX509SVID(ca.c.TrustDomain, x509CA, params, notBefore, notAfter)
199+
x509SVID, err := signX509SVID(ca.c.TrustDomain, x509CA, params, notBefore, notAfter, ca.c.OmitX509SVIDUID)
198200
if err != nil {
199201
return nil, err
200202
}
@@ -279,7 +281,7 @@ func (ca *CA) capLifetime(ttl time.Duration, expirationCap time.Time) (notBefore
279281
return notBefore, notAfter
280282
}
281283

282-
func signX509SVID(td spiffeid.TrustDomain, x509CA *X509CA, params X509SVIDParams, notBefore, notAfter time.Time) ([]*x509.Certificate, error) {
284+
func signX509SVID(td spiffeid.TrustDomain, x509CA *X509CA, params X509SVIDParams, notBefore, notAfter time.Time, omitUID bool) ([]*x509.Certificate, error) {
283285
if x509CA == nil {
284286
return nil, errs.New("X509 CA is not available for signing")
285287
}
@@ -294,9 +296,18 @@ func signX509SVID(td spiffeid.TrustDomain, x509CA *X509CA, params X509SVIDParams
294296
return nil, err
295297
}
296298

297-
// In case subject is provided use it
298299
if params.Subject.String() != "" {
299300
template.Subject = params.Subject
301+
} else {
302+
template.Subject = pkix.Name{
303+
Country: []string{"US"},
304+
Organization: []string{"SPIRE"},
305+
}
306+
}
307+
308+
// Append the unique ID to the subject, unless disabled
309+
if !omitUID {
310+
template.Subject.ExtraNames = append(template.Subject.ExtraNames, x509svid.UniqueIDAttribute(params.SpiffeID))
300311
}
301312

302313
// Explicitly set the AKI on the signed certificate, otherwise it won't be

0 commit comments

Comments
 (0)