ocket8888 commented on code in PR #7136:
URL: https://github.com/apache/trafficcontrol/pull/7136#discussion_r1014224466
##########
traffic_ops/testing/api/v5/deliveryservices_keys_test.go:
##########
@@ -333,6 +334,38 @@ func DeliveryServiceSSLKeys(t *testing.T) {
if dsSSLKey.Certificate.CSR == "" {
t.Errorf("expected a valid CSR, but got nothing")
}
+
+ var otherKey *tc.DeliveryServiceSSLKeys
+ for _, ds := range testData.DeliveryServices {
+ if !(*ds.Protocol == tc.DSProtocolHTTPS || *ds.Protocol ==
tc.DSProtocolHTTPAndHTTPS || *ds.Protocol == tc.DSProtocolHTTPToHTTPS) {
+ continue
+ }
+ var err error
+ dsSSLKey := new(tc.DeliveryServiceSSLKeys)
+ for tries := 0; tries < 5; tries++ {
+ time.Sleep(time.Second)
+ var sslKeysResp tc.DeliveryServiceSSLKeysResponse
+ sslKeysResp, _, err =
TOSession.GetDeliveryServiceSSLKeys(*ds.XMLID, client.RequestOptions{})
+ *dsSSLKey = sslKeysResp.Response
+ if err == nil && dsSSLKey != nil {
+ break
+ }
+ }
+ if dsSSLKey != nil {
+ otherKey = dsSSLKey
+ break
+ }
+ }
+ assert.RequireNotNil(t, otherKey, "Expected to find another DS with a
valid SSL certificate")
+ err = deliveryservice.Base64DecodeCertificate(&otherKey.Certificate)
+ assert.RequireNoError(t, err, "Couldn't decode certificate: %v", err)
+
+ dsSSLKeyReq.Certificate.Crt += otherKey.Certificate.Crt
+ _, _, err =
TOSession.AddSSLKeysForDS(tc.DeliveryServiceAddSSLKeysReq{DeliveryServiceSSLKeysReq:
dsSSLKeyReq}, client.RequestOptions{})
+ assert.RequireNotNil(t, err, "Expected inconsistent certificate chain
to be rejected")
+ if !strings.Contains(err.Error(), "intermediate chain contains
certificates that do not match") {
+ t.Fatalf("Expected failure with chain issue, instead got: %s",
err.Error())
Review Comment:
nit but Fatal is unnecessary when the statement is the last one in the test
##########
traffic_ops/testing/api/v5/deliveryservices_keys_test.go:
##########
@@ -333,6 +334,38 @@ func DeliveryServiceSSLKeys(t *testing.T) {
if dsSSLKey.Certificate.CSR == "" {
t.Errorf("expected a valid CSR, but got nothing")
}
+
+ var otherKey *tc.DeliveryServiceSSLKeys
+ for _, ds := range testData.DeliveryServices {
+ if !(*ds.Protocol == tc.DSProtocolHTTPS || *ds.Protocol ==
tc.DSProtocolHTTPAndHTTPS || *ds.Protocol == tc.DSProtocolHTTPToHTTPS) {
+ continue
+ }
+ var err error
+ dsSSLKey := new(tc.DeliveryServiceSSLKeys)
+ for tries := 0; tries < 5; tries++ {
+ time.Sleep(time.Second)
+ var sslKeysResp tc.DeliveryServiceSSLKeysResponse
+ sslKeysResp, _, err =
TOSession.GetDeliveryServiceSSLKeys(*ds.XMLID, client.RequestOptions{})
+ *dsSSLKey = sslKeysResp.Response
+ if err == nil && dsSSLKey != nil {
+ break
+ }
+ }
+ if dsSSLKey != nil {
+ otherKey = dsSSLKey
+ break
+ }
+ }
+ assert.RequireNotNil(t, otherKey, "Expected to find another DS with a
valid SSL certificate")
Review Comment:
`dsSSLKey` is never `nil` at any point, so the check after the inner `for`
loop will always be true, and this assertion can never fail. To make it work, I
think you'd need to only use `new` when setting the value of `dsSSLKey` in that
inner loop, and just declare it as a nil pointer above that. I'm sure the test
would fail anyway because parsing a blank cert should probably return an error,
but as-is this is an unused check.
##########
traffic_ops/traffic_ops_golang/deliveryservice/keys_test.go:
##########
@@ -2521,11 +2654,29 @@ func
TestVerifyAndEncodeCertificateECDSASelfSignedCertificateKeyPairWithoutParam
func
TestVerifyAndEncodeCertificateECDSASelfSignedCertificateKeyPairMisMatchedPrivateKey(t
*testing.T) {
// Should fail due to mismatched ECDSA cert/private key pair
- _, _, _, _, err := verifyCertKeyPair(SelfSignedECDSACertificate,
PrivateKeyECDSANISTPrime256V1, "", true)
+ _, _, _, _, _, err := verifyCertKeyPair(SelfSignedECDSACertificate,
PrivateKeyECDSANISTPrime256V1, "", true)
if err == nil {
t.Fatalf("unexpected Result: Mismatched ECDSA cert/key pair
should have failed verification")
} else {
t.Logf("expected error message: %s", err.Error())
}
}
+
+func TestVerifyInconsistentCertChain(t *testing.T) {
+ _, _, _, _, isInconsistent, err :=
verifyCertKeyPair(SelfSignedInconsistentCertChain,
SelfSignedRSAPrivateKeyInvalidChain, "", true)
+ if err != nil {
+ t.Fatalf("expected mismatched to return no error")
+ }
+ if !isInconsistent {
+ t.Fatalf("expected chain to be considered inconsistent")
+ }
+
+ _, _, _, _, isInconsistent, err =
verifyCertKeyPair(SelfSignedCertChain+ValidIntermediateCert,
SelfSignedRSAPrivateKeyInvalidChain, "", true)
+ if err != nil {
+ t.Fatalf("expected mismatched to return no error")
+ }
+ if !isInconsistent {
+ t.Fatalf("expected chain to be considered inconsistent")
+ }
Review Comment:
nit but I don't think any of these actually need to be fatal failures
##########
traffic_ops/testing/api/v5/deliveryservices_keys_test.go:
##########
@@ -333,6 +334,38 @@ func DeliveryServiceSSLKeys(t *testing.T) {
if dsSSLKey.Certificate.CSR == "" {
t.Errorf("expected a valid CSR, but got nothing")
}
+
+ var otherKey *tc.DeliveryServiceSSLKeys
+ for _, ds := range testData.DeliveryServices {
+ if !(*ds.Protocol == tc.DSProtocolHTTPS || *ds.Protocol ==
tc.DSProtocolHTTPAndHTTPS || *ds.Protocol == tc.DSProtocolHTTPToHTTPS) {
+ continue
+ }
+ var err error
+ dsSSLKey := new(tc.DeliveryServiceSSLKeys)
+ for tries := 0; tries < 5; tries++ {
+ time.Sleep(time.Second)
+ var sslKeysResp tc.DeliveryServiceSSLKeysResponse
+ sslKeysResp, _, err =
TOSession.GetDeliveryServiceSSLKeys(*ds.XMLID, client.RequestOptions{})
+ *dsSSLKey = sslKeysResp.Response
+ if err == nil && dsSSLKey != nil {
Review Comment:
Similar to above, `dsSSLKey` can never be nil, so checking it here is
pointless unless that changes. If that check were to be `true`, the preceding
line would segfault before it got that far.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
