Skip to content

Commit e1ecfeb

Browse files
authored
fix: generate unique SSL IDs to prevent certificate conflicts across different hosts (#2592) (#305)
1 parent 936b2e7 commit e1ecfeb

File tree

8 files changed

+174
-77
lines changed

8 files changed

+174
-77
lines changed

api/adc/types.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,19 @@ func (n Upstream) MarshalJSON() ([]byte, error) {
472472
return json.Marshal((Alias)(n))
473473
}
474474

475+
func ComposeSSLName(kind, namespace, name string) string {
476+
p := make([]byte, 0, len(kind)+len(namespace)+len(name)+2)
477+
buf := bytes.NewBuffer(p)
478+
479+
buf.WriteString(kind)
480+
buf.WriteByte('_')
481+
buf.WriteString(namespace)
482+
buf.WriteByte('_')
483+
buf.WriteString(name)
484+
485+
return buf.String()
486+
}
487+
475488
// ComposeRouteName uses namespace, name and rule name to compose
476489
// the route name.
477490
func ComposeRouteName(namespace, name string, rule string) string {

internal/adc/translator/apisixtls.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"github.com/apache/apisix-ingress-controller/internal/controller/label"
2828
"github.com/apache/apisix-ingress-controller/internal/id"
2929
"github.com/apache/apisix-ingress-controller/internal/provider"
30+
internaltypes "github.com/apache/apisix-ingress-controller/internal/types"
3031
)
3132

3233
func (t *Translator) TranslateApisixTls(tctx *provider.TranslateContext, tls *apiv2.ApisixTls) (*TranslateResult, error) {
@@ -57,7 +58,7 @@ func (t *Translator) TranslateApisixTls(tctx *provider.TranslateContext, tls *ap
5758
// Create SSL object
5859
ssl := &adctypes.SSL{
5960
Metadata: adctypes.Metadata{
60-
ID: id.GenID(tls.Namespace + "_" + tls.Name),
61+
ID: id.GenID(adctypes.ComposeSSLName(internaltypes.KindApisixTls, tls.Namespace, tls.Name)),
6162
Labels: label.GenLabel(tls),
6263
},
6364
Certificates: []adctypes.Certificate{

internal/adc/translator/gateway.go

Lines changed: 2 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import (
2222
"encoding/json"
2323
"encoding/pem"
2424
"fmt"
25-
"slices"
2625

2726
"github.com/pkg/errors"
2827
corev1 "k8s.io/api/core/v1"
@@ -50,7 +49,6 @@ func (t *Translator) TranslateGateway(tctx *provider.TranslateContext, obj *gate
5049
result.SSL = append(result.SSL, ssl...)
5150
}
5251
}
53-
result.SSL = mergeSSLWithSameID(result.SSL)
5452

5553
rk := utils.NamespacedNameKind(obj)
5654
gatewayProxy, ok := tctx.GatewayProxies[rk]
@@ -80,7 +78,7 @@ func (t *Translator) translateSecret(tctx *provider.TranslateContext, listener g
8078
sslObjs := make([]*adctypes.SSL, 0)
8179
switch *listener.TLS.Mode {
8280
case gatewayv1.TLSModeTerminate:
83-
for _, ref := range listener.TLS.CertificateRefs {
81+
for refIndex, ref := range listener.TLS.CertificateRefs {
8482
ns := obj.GetNamespace()
8583
if ref.Namespace != nil {
8684
ns = string(*ref.Namespace)
@@ -122,8 +120,7 @@ func (t *Translator) translateSecret(tctx *provider.TranslateContext, listener g
122120
}
123121
sslObj.Snis = append(sslObj.Snis, hosts...)
124122
}
125-
// Note: use cert as id to avoid duplicate certificate across ssl objects
126-
sslObj.ID = id.GenID(string(cert))
123+
sslObj.ID = id.GenID(fmt.Sprintf("%s_%s_%d", adctypes.ComposeSSLName(internaltypes.KindGateway, obj.Namespace, obj.Name), listener.Name, refIndex))
127124
t.Log.V(1).Info("generated ssl id", "ssl id", sslObj.ID, "secret", secretNN.String())
128125
sslObj.Labels = label.GenLabel(obj)
129126
sslObjs = append(sslObjs, sslObj)
@@ -241,47 +238,3 @@ func (t *Translator) fillPluginMetadataFromGatewayProxy(pluginMetadata adctypes.
241238
pluginMetadata[pluginName] = pluginConfig
242239
}
243240
}
244-
245-
// mergeSSLWithSameID merge ssl with same id
246-
func mergeSSLWithSameID(sslList []*adctypes.SSL) []*adctypes.SSL {
247-
if len(sslList) <= 1 {
248-
return sslList
249-
}
250-
251-
// create a map to store ssl with same id
252-
sslMap := make(map[string]*adctypes.SSL)
253-
for _, ssl := range sslList {
254-
if existing, exists := sslMap[ssl.ID]; exists {
255-
// if ssl with same id exists, merge their snis
256-
// use map to deduplicate
257-
sniMap := make(map[string]struct{})
258-
// add existing snis
259-
for _, sni := range existing.Snis {
260-
sniMap[sni] = struct{}{}
261-
}
262-
// add new snis
263-
for _, sni := range ssl.Snis {
264-
sniMap[sni] = struct{}{}
265-
}
266-
// rebuild deduplicated snis list
267-
newSnis := make([]string, 0, len(sniMap))
268-
for sni := range sniMap {
269-
newSnis = append(newSnis, sni)
270-
}
271-
272-
slices.Sort(newSnis)
273-
// update existing ssl object
274-
existing.Snis = newSnis
275-
} else {
276-
slices.Sort(ssl.Snis)
277-
// if new ssl id, add to map
278-
sslMap[ssl.ID] = ssl
279-
}
280-
}
281-
282-
mergedSSL := make([]*adctypes.SSL, 0, len(sslMap))
283-
for _, ssl := range sslMap {
284-
mergedSSL = append(mergedSSL, ssl)
285-
}
286-
return mergedSSL
287-
}

internal/adc/translator/ingress.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ import (
3333
internaltypes "github.com/apache/apisix-ingress-controller/internal/types"
3434
)
3535

36-
func (t *Translator) translateIngressTLS(ingressTLS *networkingv1.IngressTLS, secret *corev1.Secret, labels map[string]string) (*adctypes.SSL, error) {
36+
func (t *Translator) translateIngressTLS(namespace, name string, tlsIndex int, ingressTLS *networkingv1.IngressTLS, secret *corev1.Secret, labels map[string]string) (*adctypes.SSL, error) {
3737
// extract the key pair from the secret
3838
cert, key, err := extractKeyPair(secret, true)
3939
if err != nil {
@@ -64,7 +64,7 @@ func (t *Translator) translateIngressTLS(ingressTLS *networkingv1.IngressTLS, se
6464
},
6565
Snis: hosts,
6666
}
67-
ssl.ID = id.GenID(string(cert))
67+
ssl.ID = id.GenID(fmt.Sprintf("%s_%d", adctypes.ComposeSSLName(internaltypes.KindIngress, namespace, name), tlsIndex))
6868

6969
return ssl, nil
7070
}
@@ -75,7 +75,7 @@ func (t *Translator) TranslateIngress(tctx *provider.TranslateContext, obj *netw
7575
labels := label.GenLabel(obj)
7676

7777
// handle TLS configuration, convert to SSL objects
78-
for _, tls := range obj.Spec.TLS {
78+
for tlsIndex, tls := range obj.Spec.TLS {
7979
if tls.SecretName == "" {
8080
continue
8181
}
@@ -86,7 +86,7 @@ func (t *Translator) TranslateIngress(tctx *provider.TranslateContext, obj *netw
8686
if secret == nil {
8787
continue
8888
}
89-
ssl, err := t.translateIngressTLS(&tls, secret, labels)
89+
ssl, err := t.translateIngressTLS(obj.Namespace, obj.Name, tlsIndex, &tls, secret, labels)
9090
if err != nil {
9191
return nil, err
9292
}

test/e2e/crds/v2/tls.go

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,5 +310,111 @@ spec:
310310
assert.Equal(GinkgoT(), int64(10), *tls[0].Client.Depth, "client depth should be 10")
311311
assert.Contains(GinkgoT(), tls[0].Client.SkipMtlsURIRegex, skipMtlsUriRegex, "skip_mtls_uri_regex should be set")
312312
})
313+
314+
It("ApisixTls and Ingress with same certificate but different hosts", func() {
315+
if framework.IngressVersion != "v1" {
316+
Skip("skipping test in non-v1 ingress version")
317+
}
318+
By("create shared TLS secret")
319+
err := s.NewKubeTlsSecret("shared-tls-secret", Cert, Key)
320+
Expect(err).NotTo(HaveOccurred(), "creating shared TLS secret")
321+
322+
const apisixTlsSpec = `
323+
apiVersion: apisix.apache.org/v2
324+
kind: ApisixTls
325+
metadata:
326+
name: test-apisixtls-shared
327+
spec:
328+
ingressClassName: %s
329+
hosts:
330+
- api6.com
331+
secret:
332+
name: shared-tls-secret
333+
namespace: %s
334+
`
335+
336+
By("apply ApisixTls with api6.com")
337+
var apisixTls apiv2.ApisixTls
338+
tlsSpec := fmt.Sprintf(apisixTlsSpec, s.Namespace(), s.Namespace())
339+
applier.MustApplyAPIv2(types.NamespacedName{Namespace: s.Namespace(), Name: "test-apisixtls-shared"}, &apisixTls, tlsSpec)
340+
341+
const ingressYamlWithTLS = `
342+
apiVersion: networking.k8s.io/v1
343+
kind: Ingress
344+
metadata:
345+
name: test-ingress-tls-shared
346+
spec:
347+
ingressClassName: %s
348+
tls:
349+
- hosts:
350+
- api7.com
351+
secretName: shared-tls-secret
352+
rules:
353+
- host: api7.com
354+
http:
355+
paths:
356+
- path: /
357+
pathType: Prefix
358+
backend:
359+
service:
360+
name: httpbin-service-e2e-test
361+
port:
362+
number: 80
363+
`
364+
365+
By("apply Ingress with api7.com using same certificate")
366+
err = s.CreateResourceFromString(fmt.Sprintf(ingressYamlWithTLS, s.Namespace()))
367+
Expect(err).NotTo(HaveOccurred(), "creating Ingress")
368+
369+
By("verify two SSL objects exist in control plane")
370+
Eventually(func() bool {
371+
tls, err := s.DefaultDataplaneResource().SSL().List(context.Background())
372+
if err != nil {
373+
return false
374+
}
375+
return len(tls) == 2
376+
}).WithTimeout(30 * time.Second).ProbeEvery(1 * time.Second).Should(BeTrue())
377+
378+
tls, err := s.DefaultDataplaneResource().SSL().List(context.Background())
379+
assert.Nil(GinkgoT(), err, "list tls error")
380+
assert.Len(GinkgoT(), tls, 2, "should have exactly 2 SSL objects")
381+
382+
By("verify SSL objects have different IDs and SNIs")
383+
sniFound := make(map[string]bool)
384+
385+
for i := range tls {
386+
// Check certificate content is the same
387+
assert.Len(GinkgoT(), tls[i].Certificates, 1, "each SSL should have 1 certificate")
388+
assert.Equal(GinkgoT(), Cert, tls[i].Certificates[0].Certificate, "certificate should match")
389+
390+
// Track SNIs
391+
for _, sni := range tls[i].Snis {
392+
sniFound[sni] = true
393+
}
394+
}
395+
396+
By("verify both hosts are covered")
397+
assert.True(GinkgoT(), sniFound["api6.com"], "api6.com should be in SNIs")
398+
assert.True(GinkgoT(), sniFound["api7.com"], "api7.com should be in SNIs")
399+
400+
By("test HTTPS request to api6.com")
401+
Eventually(func() int {
402+
return s.NewAPISIXHttpsClient("api6.com").
403+
GET("/get").
404+
WithHost("api6.com").
405+
Expect().
406+
Raw().StatusCode
407+
}).WithTimeout(30 * time.Second).ProbeEvery(1 * time.Second).Should(Equal(http.StatusOK))
408+
409+
By("test HTTPS request to api7.com")
410+
Eventually(func() int {
411+
return s.NewAPISIXHttpsClient("api7.com").
412+
GET("/get").
413+
WithHost("api7.com").
414+
Expect().
415+
Raw().StatusCode
416+
}).WithTimeout(30 * time.Second).ProbeEvery(1 * time.Second).Should(Equal(http.StatusOK))
417+
})
418+
313419
})
314420
})

test/e2e/gatewayapi/gateway.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ spec:
256256
name: %s
257257
- name: https-with-hostname
258258
port: 443
259-
hostname: api6.com
259+
hostname: test.com
260260
protocol: HTTPS
261261
allowedRoutes:
262262
namespaces:
@@ -284,8 +284,8 @@ spec:
284284
Eventually(func() error {
285285
tls, err := s.DefaultDataplaneResource().SSL().List(context.Background())
286286
Expect(err).NotTo(HaveOccurred(), "list ssl")
287-
if len(tls) != 1 {
288-
return fmt.Errorf("expect 1 ssl, got %d", len(tls))
287+
if len(tls) != 2 {
288+
return fmt.Errorf("expect 2 ssl, got %d", len(tls))
289289
}
290290
if len(tls[0].Certificates) != 1 {
291291
return fmt.Errorf("expect 1 certificate, got %d", len(tls[0].Certificates))
@@ -305,7 +305,7 @@ spec:
305305
Eventually(func() string {
306306
tls, err := s.DefaultDataplaneResource().SSL().List(context.Background())
307307
Expect(err).NotTo(HaveOccurred(), "list ssl")
308-
if len(tls) < 1 {
308+
if len(tls) != 2 {
309309
return ""
310310
}
311311
if len(tls[0].Certificates) < 1 {

test/e2e/gatewayapi/tcproute.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,21 +72,22 @@ spec:
7272

7373
// Create GatewayClass
7474
Expect(s.CreateResourceFromString(s.GetGatewayClassYaml())).NotTo(HaveOccurred(), "creating GatewayClass")
75+
Expect(s.CreateResourceFromString(s.GetGatewayClassYaml())).
76+
NotTo(HaveOccurred(), "creating GatewayClass")
7577

7678
// Create Gateway with TCP listener
7779
Expect(s.CreateResourceFromString(fmt.Sprintf(tcpGateway, s.Namespace(), s.Namespace()))).
7880
NotTo(HaveOccurred(), "creating Gateway")
7981
})
8082

8183
It("should route TCP traffic to backend service", func() {
82-
gatewayName := s.Namespace()
8384
By("creating TCPRoute")
84-
Expect(s.CreateResourceFromString(fmt.Sprintf(tcpRoute, gatewayName))).
85+
Expect(s.CreateResourceFromString(fmt.Sprintf(tcpRoute, s.Namespace()))).
8586
NotTo(HaveOccurred(), "creating TCPRoute")
8687
time.Sleep(2 * time.Second)
8788

8889
By("verifying TCPRoute is functional")
89-
s.HTTPOverTCPConnectAssert(true, time.Minute*5) // should be able to connect
90+
s.HTTPOverTCPConnectAssert(true, time.Minute*3) // should be able to connect
9091
By("sending TCP traffic to verify routing")
9192
s.RequestAssert(&scaffold.RequestAssert{
9293
Client: s.NewAPISIXClientOnTCPPort(),
@@ -101,7 +102,7 @@ spec:
101102
Expect(s.DeleteResource("TCPRoute", "tcp-app-1")).
102103
NotTo(HaveOccurred(), "deleting TCPRoute")
103104

104-
s.HTTPOverTCPConnectAssert(false, time.Minute*5)
105+
s.HTTPOverTCPConnectAssert(false, time.Minute*3)
105106
})
106107
})
107108
})

0 commit comments

Comments
 (0)