Skip to content
This repository was archived by the owner on Jul 11, 2023. It is now read-only.

Commit 5ab34a3

Browse files
authored
Leverage trust domain in issuing certs; remove TD from identity (#4782)
* This PR finalizes the steps to allow for custom trust domains. It accomplishes this by: 1. Remove trust domain from service identity 2. Modify IssueCertificate to accept a CN prefix instead of a full CN, and apply the trust domain dynamically. 3. Change IssueCertificate to accept variadic options when the full CN is actually provided Signed-off-by: Sean Teeling <[email protected]> * address comments Signed-off-by: Sean Teeling <[email protected]> * address comment Signed-off-by: Sean Teeling <[email protected]> * address lint Signed-off-by: Sean Teeling <[email protected]>
1 parent 8b1c3cc commit 5ab34a3

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

51 files changed

+312
-328
lines changed

cmd/osm-controller/gateway.go

+2-3
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import (
1515
"github.com/openservicemesh/osm/pkg/certificate"
1616
"github.com/openservicemesh/osm/pkg/constants"
1717
"github.com/openservicemesh/osm/pkg/envoy/bootstrap"
18-
"github.com/openservicemesh/osm/pkg/identity"
1918
"github.com/openservicemesh/osm/pkg/multicluster"
2019
"github.com/openservicemesh/osm/pkg/utils"
2120
)
@@ -38,7 +37,7 @@ func bootstrapOSMMulticlusterGateway(kubeClient kubernetes.Interface, certManage
3837
}
3938

4039
gatewayCN := multicluster.GetMulticlusterGatewaySubjectCommonName(osmServiceAccount, osmNamespace)
41-
bootstrapCert, err := certManager.IssueCertificate(gatewayCN, constants.XDSCertificateValidityPeriod)
40+
bootstrapCert, err := certManager.IssueCertificate(gatewayCN, certificate.WithValidityPeriod(constants.XDSCertificateValidityPeriod))
4241
if err != nil {
4342
return errors.Errorf("Error issuing bootstrap certificate for OSM gateway: %s", err)
4443
}
@@ -47,7 +46,7 @@ func bootstrapOSMMulticlusterGateway(kubeClient kubernetes.Interface, certManage
4746
NodeID: bootstrapCert.GetCommonName().String(),
4847
AdminPort: constants.EnvoyAdminPort,
4948
XDSClusterName: constants.OSMControllerName,
50-
XDSHost: fmt.Sprintf("%s.%s.svc.%s", constants.OSMControllerName, osmNamespace, identity.ClusterLocalTrustDomain),
49+
XDSHost: fmt.Sprintf("%s.%s.svc.cluster.local", constants.OSMControllerName, osmNamespace),
5150
XDSPort: constants.ADSServerPort,
5251
})
5352
if err != nil {

cmd/osm-controller/osm-controller.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
2929
"k8s.io/client-go/tools/clientcmd"
3030

31+
"github.com/openservicemesh/osm/pkg/certificate"
3132
configClientset "github.com/openservicemesh/osm/pkg/gen/client/config/clientset/versioned"
3233
policyClientset "github.com/openservicemesh/osm/pkg/gen/client/policy/clientset/versioned"
3334

@@ -262,7 +263,7 @@ func main() {
262263
proxyRegistry := registry.NewProxyRegistry(proxyMapper, msgBroker)
263264
go proxyRegistry.ReleaseCertificateHandler(certManager, stop)
264265

265-
adsCert, err := certManager.IssueCertificate(xdsServerCertificateCommonName, constants.XDSCertificateValidityPeriod)
266+
adsCert, err := certManager.IssueCertificate(xdsServerCertificateCommonName, certificate.WithValidityPeriod(constants.XDSCertificateValidityPeriod))
266267
if err != nil {
267268
events.GenericEventRecorder().FatalEvent(err, events.CertificateIssuanceFailure, "Error issuing XDS certificate to ADS server")
268269
}

pkg/catalog/egress_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,7 @@ func TestGetEgressTrafficPolicy(t *testing.T) {
433433
},
434434
}
435435

436-
testSourceIdentity := identity.ServiceIdentity("foo.bar.cluster.local")
436+
testSourceIdentity := identity.ServiceIdentity("foo.bar")
437437

438438
for i, tc := range testCases {
439439
t.Run(fmt.Sprintf("Running test case %d: %s", i, tc.name), func(t *testing.T) {

pkg/catalog/outbound_traffic_policies_test.go

+5-5
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,12 @@ func TestGetOutboundMeshTrafficPolicy(t *testing.T) {
7676
}
7777

7878
svcIdentityToSvcMapping := map[string][]service.MeshService{
79-
"sa1.ns1.cluster.local": {meshSvc1P1, meshSvc1P2},
80-
"sa2.ns2.cluster.local": {meshSvc2}, // Client `downstreamIdentity` cannot access this upstream
81-
"sa3.ns3.cluster.local": {meshSvc3, meshSvc3V1, meshSvc3V2, meshSvc4, meshSvc5},
79+
"sa1.ns1": {meshSvc1P1, meshSvc1P2},
80+
"sa2.ns2": {meshSvc2}, // Client `downstreamIdentity` cannot access this upstream
81+
"sa3.ns3": {meshSvc3, meshSvc3V1, meshSvc3V2, meshSvc4, meshSvc5},
8282
}
8383

84-
downstreamIdentity := identity.ServiceIdentity("sa-x.ns1.cluster.local")
84+
downstreamIdentity := identity.ServiceIdentity("sa-x.ns1")
8585

8686
// TrafficTargets that allow: sa-x.ns1 -> sa1.ns1, sa3.ns3
8787
// No TrafficTarget that allows sa-x.ns1 -> sa2.ns2 (this should be allowed in permissive mode)
@@ -691,7 +691,7 @@ func TestListOutboundServicesForIdentity(t *testing.T) {
691691
},
692692
{
693693
name: "gateway",
694-
svcIdentity: "gateway.osm-system.cluster.local",
694+
svcIdentity: "gateway.osm-system",
695695
expectedList: []service.MeshService{tests.BookstoreV1Service, tests.BookstoreV2Service, tests.BookstoreApexService, tests.BookbuyerService},
696696
permissiveMode: true,
697697
},

pkg/catalog/retry_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ func TestGetRetryPolicy(t *testing.T) {
3838
policyController: mockPolicyController,
3939
kubeController: mockKubeController,
4040
}
41-
retrySrc := identity.ServiceIdentity("sa1.ns.cluster.local")
41+
retrySrc := identity.ServiceIdentity("sa1.ns")
4242

4343
testcases := []struct {
4444
name string

pkg/catalog/traffictarget_test.go

+10-10
Original file line numberDiff line numberDiff line change
@@ -501,9 +501,9 @@ func TestListInboundTrafficTargetsWithRoutes(t *testing.T) {
501501
expectedTrafficTargets: []trafficpolicy.TrafficTargetWithRoutes{
502502
{
503503
Name: "ns-1/test-1",
504-
Destination: identity.ServiceIdentity("sa-1.ns-1.cluster.local"),
504+
Destination: identity.ServiceIdentity("sa-1.ns-1"),
505505
Sources: []identity.ServiceIdentity{
506-
identity.ServiceIdentity("sa-2.ns-2.cluster.local"),
506+
identity.ServiceIdentity("sa-2.ns-2"),
507507
},
508508
TCPRouteMatches: []trafficpolicy.TCPRouteMatch{
509509
{
@@ -588,9 +588,9 @@ func TestListInboundTrafficTargetsWithRoutes(t *testing.T) {
588588
expectedTrafficTargets: []trafficpolicy.TrafficTargetWithRoutes{
589589
{
590590
Name: "ns-1/test-1",
591-
Destination: identity.ServiceIdentity("sa-1.ns-1.cluster.local"),
591+
Destination: identity.ServiceIdentity("sa-1.ns-1"),
592592
Sources: []identity.ServiceIdentity{
593-
identity.ServiceIdentity("sa-2.ns-2.cluster.local"),
593+
identity.ServiceIdentity("sa-2.ns-2"),
594594
},
595595
TCPRouteMatches: []trafficpolicy.TCPRouteMatch{
596596
{
@@ -736,9 +736,9 @@ func TestListInboundTrafficTargetsWithRoutes(t *testing.T) {
736736
expectedTrafficTargets: []trafficpolicy.TrafficTargetWithRoutes{
737737
{
738738
Name: "ns-1/test-1",
739-
Destination: identity.ServiceIdentity("sa-1.ns-1.cluster.local"),
739+
Destination: identity.ServiceIdentity("sa-1.ns-1"),
740740
Sources: []identity.ServiceIdentity{
741-
identity.ServiceIdentity("sa-2.ns-2.cluster.local"),
741+
identity.ServiceIdentity("sa-2.ns-2"),
742742
},
743743
TCPRouteMatches: []trafficpolicy.TCPRouteMatch{
744744
{
@@ -753,9 +753,9 @@ func TestListInboundTrafficTargetsWithRoutes(t *testing.T) {
753753
},
754754
{
755755
Name: "ns-1/test-2",
756-
Destination: identity.ServiceIdentity("sa-1.ns-1.cluster.local"),
756+
Destination: identity.ServiceIdentity("sa-1.ns-1"),
757757
Sources: []identity.ServiceIdentity{
758-
identity.ServiceIdentity("sa-3.ns-3.cluster.local"),
758+
identity.ServiceIdentity("sa-3.ns-3"),
759759
},
760760
TCPRouteMatches: []trafficpolicy.TCPRouteMatch{
761761
{
@@ -823,9 +823,9 @@ func TestListInboundTrafficTargetsWithRoutes(t *testing.T) {
823823
expectedTrafficTargets: []trafficpolicy.TrafficTargetWithRoutes{
824824
{
825825
Name: "ns-1/test-1",
826-
Destination: identity.ServiceIdentity("sa-1.ns-1.cluster.local"),
826+
Destination: identity.ServiceIdentity("sa-1.ns-1"),
827827
Sources: []identity.ServiceIdentity{
828-
identity.ServiceIdentity("sa-2.ns-2.cluster.local"),
828+
identity.ServiceIdentity("sa-2.ns-2"),
829829
},
830830
TCPRouteMatches: []trafficpolicy.TCPRouteMatch{
831831
{

pkg/certificate/fake_manager.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,11 @@ func (c *fakeMRCClient) GetCertIssuerForMRC(mrc *v1alpha2.MeshRootCertificate) (
2121
// List returns the single, pre-generated MRC. It is intended to implement the certificate.MRCClient interface.
2222
func (c *fakeMRCClient) List() ([]*v1alpha2.MeshRootCertificate, error) {
2323
// return single empty object in the list.
24-
return []*v1alpha2.MeshRootCertificate{{}}, nil
24+
return []*v1alpha2.MeshRootCertificate{{
25+
Spec: v1alpha2.MeshRootCertificateSpec{
26+
TrustDomain: "fake.domain.com",
27+
},
28+
}}, nil
2529
}
2630

2731
type fakeIssuer struct {

pkg/certificate/manager.go

+37-16
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ func NewManager(mrcClient MRCClient, serviceCertValidityDuration time.Duration,
2424
return nil, err
2525
}
2626

27-
c := &issuer{Issuer: client, ID: clientID, CertificateAuthority: ca}
27+
c := &issuer{Issuer: client, ID: clientID, CertificateAuthority: ca, TrustDomain: mrcs[0].Spec.TrustDomain}
2828

2929
m := &Manager{
3030
// The signingIssuer is responsible for signing all newly issued certificates
@@ -59,15 +59,23 @@ func (m *Manager) Start(checkInterval time.Duration, stop <-chan struct{}) {
5959
// GetTrustDomain returns the trust domain from the configured signingkey issuer.
6060
// Note that the CRD uses a default, so this value will always be set.
6161
func (m *Manager) GetTrustDomain() string {
62-
// TODO(4754): implement
63-
return ""
62+
m.mu.Lock()
63+
defer m.mu.Unlock()
64+
return m.signingIssuer.TrustDomain
6465
}
6566

6667
func (m *Manager) checkAndRotate() {
6768
// NOTE: checkAndRotate can reintroduce a certificate that has been released, thereby creating an unbounded cache.
6869
// A certificate can also have been rotated already, leaving the list of issued certs stale, and we re-rotate.
6970
// the latter is not a bug, but a source of inefficiency.
70-
for _, cert := range m.ListIssuedCertificates() {
71+
72+
certs := map[string]*Certificate{}
73+
m.cache.Range(func(keyIface interface{}, certInterface interface{}) bool {
74+
key := keyIface.(string)
75+
certs[key] = certInterface.(*Certificate)
76+
return true // continue the iteration
77+
})
78+
for key, cert := range certs {
7179
shouldRotate := cert.ShouldRotate()
7280

7381
word := map[bool]string{true: "will", false: "will not"}[shouldRotate]
@@ -78,7 +86,14 @@ func (m *Manager) checkAndRotate() {
7886
RenewBeforeCertExpires)
7987

8088
if shouldRotate {
81-
newCert, err := m.IssueCertificate(cert.GetCommonName(), m.serviceCertValidityDuration)
89+
opts := []IssueOption{WithValidityPeriod(m.serviceCertValidityDuration)}
90+
// if the key is equal to the common name, then it was issued with FullCNProvided(). This will prevent
91+
// an additional trust domain from being appended. We don't do this in every case, in case the trust domain
92+
// has changed since the last issue.
93+
if key == cert.CommonName.String() {
94+
opts = append(opts, FullCNProvided())
95+
}
96+
newCert, err := m.IssueCertificate(key, opts...)
8297
if err != nil {
8398
// TODO(#3962): metric might not be scraped before process restart resulting from this error
8499
log.Error().Err(err).Str(errcode.Kind, errcode.GetErrCodeWithMetric(errcode.ErrRotatingCert)).
@@ -97,8 +112,8 @@ func (m *Manager) checkAndRotate() {
97112
}
98113
}
99114

100-
func (m *Manager) getFromCache(cn CommonName) *Certificate {
101-
certInterface, exists := m.cache.Load(cn)
115+
func (m *Manager) getFromCache(key string) *Certificate {
116+
certInterface, exists := m.cache.Load(key)
102117
if !exists {
103118
return nil
104119
}
@@ -112,18 +127,24 @@ func (m *Manager) getFromCache(cn CommonName) *Certificate {
112127
}
113128

114129
// IssueCertificate implements Manager and returns a newly issued certificate from the given client.
115-
func (m *Manager) IssueCertificate(cn CommonName, validityPeriod time.Duration) (*Certificate, error) {
130+
func (m *Manager) IssueCertificate(prefix string, opts ...IssueOption) (*Certificate, error) {
116131
var err error
117-
cert := m.getFromCache(cn) // Don't call this while holding the lock
132+
cert := m.getFromCache(prefix) // Don't call this while holding the lock
133+
134+
options := defaultOptions(m.serviceCertValidityDuration)
135+
136+
for _, o := range opts {
137+
o(options)
138+
}
118139

119-
m.mu.RLock()
140+
m.mu.Lock()
120141
validatingIssuer := m.validatingIssuer
121142
signingIssuer := m.signingIssuer
122-
m.mu.RUnlock()
143+
m.mu.Unlock()
123144

124145
start := time.Now()
125146
if cert == nil || cert.signingIssuerID != signingIssuer.ID || cert.validatingIssuerID != validatingIssuer.ID {
126-
cert, err = signingIssuer.IssueCertificate(cn, validityPeriod)
147+
cert, err = signingIssuer.IssueCertificate(options.formatCN(prefix, signingIssuer.TrustDomain), options.validityPeriod)
127148
if err != nil {
128149
return nil, err
129150
}
@@ -138,17 +159,17 @@ func (m *Manager) IssueCertificate(cn CommonName, validityPeriod time.Duration)
138159
cert.validatingIssuerID = validatingIssuer.ID
139160
}
140161

141-
m.cache.Store(cn, cert)
162+
m.cache.Store(prefix, cert)
142163

143164
log.Trace().Msgf("It took %s to issue certificate with SerialNumber=%s", time.Since(start), cert.GetSerialNumber())
144165

145166
return cert, nil
146167
}
147168

148169
// ReleaseCertificate is called when a cert will no longer be needed and should be removed from the system.
149-
func (m *Manager) ReleaseCertificate(cn CommonName) {
150-
log.Trace().Msgf("Releasing certificate %s", cn)
151-
m.cache.Delete(cn)
170+
func (m *Manager) ReleaseCertificate(key string) {
171+
log.Trace().Msgf("Releasing certificate %s", key)
172+
m.cache.Delete(key)
152173
}
153174

154175
// ListIssuedCertificates implements CertificateDebugger interface and returns the list of issued certificates.

0 commit comments

Comments
 (0)