rawlinp commented on a change in pull request #6024:
URL: https://github.com/apache/trafficcontrol/pull/6024#discussion_r671519385



##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go
##########
@@ -115,3 +118,70 @@ func generatePutRiakKeys(req 
tc.DeliveryServiceGenSSLKeysReq, tx *sql.Tx, tv tra
        }
        return nil
 }
+
+// GeneratePlaceholderSelfSignedCert generates a self-signed SSL certificate 
as a placeholder when a new HTTPS
+// delivery service is created or an HTTP delivery service is updated to use 
HTTPS.
+func GeneratePlaceholderSelfSignedCert(ds tc.DeliveryServiceV40, inf 
*api.APIInfo, context context.Context) (error, int) {

Review comment:
       Per one of my prior comments, we would make this use the 
`DeliveryServiceV4` alias instead of `v40` if the check and generation are 
moved into the `createV40` and `updateV40` functions. That alias is meant to 
make things future-proof, i.e. as we add minor API versions, e.g. 
`DeliveryServiceV41`, the alias would be re-aliased to the latest version so 
basically all the functions still work without needing updated.

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go
##########
@@ -115,3 +118,70 @@ func generatePutRiakKeys(req 
tc.DeliveryServiceGenSSLKeysReq, tx *sql.Tx, tv tra
        }
        return nil
 }
+
+// GeneratePlaceholderSelfSignedCert generates a self-signed SSL certificate 
as a placeholder when a new HTTPS
+// delivery service is created or an HTTP delivery service is updated to use 
HTTPS.
+func GeneratePlaceholderSelfSignedCert(ds tc.DeliveryServiceV40, inf 
*api.APIInfo, context context.Context) (error, int) {
+       version := util.JSONIntStr(1)
+
+       db, err := api.GetDB(context)
+       if err != nil {
+               return err, http.StatusInternalServerError
+       }
+       tx, err := db.Begin()

Review comment:
       Does using `inf.Tx.Tx` not work? 

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go
##########
@@ -115,3 +118,70 @@ func generatePutRiakKeys(req 
tc.DeliveryServiceGenSSLKeysReq, tx *sql.Tx, tv tra
        }
        return nil
 }
+
+// GeneratePlaceholderSelfSignedCert generates a self-signed SSL certificate 
as a placeholder when a new HTTPS
+// delivery service is created or an HTTP delivery service is updated to use 
HTTPS.
+func GeneratePlaceholderSelfSignedCert(ds tc.DeliveryServiceV40, inf 
*api.APIInfo, context context.Context) (error, int) {
+       version := util.JSONIntStr(1)
+
+       db, err := api.GetDB(context)
+       if err != nil {
+               return err, http.StatusInternalServerError
+       }
+       tx, err := db.Begin()
+       if err != nil {
+               return err, http.StatusInternalServerError
+       }
+       defer tx.Commit()
+
+       cdnName, cdnDomain, err := getCDNNameDomain(*ds.CDNID, tx)
+       if err != nil {
+               return err, http.StatusInternalServerError
+       }
+
+       cdnNameStr := string(cdnName)
+
+       if ds.ExampleURLs == nil {
+               ds.ExampleURLs = MakeExampleURLs(ds.Protocol, *ds.Type, 
*ds.RoutingName, *ds.MatchList, cdnDomain)
+       }
+
+       hostname := strings.Split(ds.ExampleURLs[0], "://")[1]
+       if strings.Contains(ds.Type.String(), "HTTP") {
+               parts := strings.Split(hostname, ".")
+               parts[0] = "*"
+               hostname = strings.Join(parts, ".")
+       }
+
+       req := tc.DeliveryServiceGenSSLKeysReq{
+               DeliveryServiceSSLKeysReq: tc.DeliveryServiceSSLKeysReq{
+                       CDN:             &cdnNameStr,
+                       DeliveryService: ds.XMLID,
+                       HostName:        &hostname,
+                       Key:             ds.XMLID,
+                       Version:         &version,
+                       BusinessUnit:    util.StrPtr("Placeholder"),
+                       City:            util.StrPtr("Placeholder"),
+                       Organization:    util.StrPtr("Placeholder"),
+                       Country:         util.StrPtr("United States (US)"),

Review comment:
       Does "Placeholder" not work for country and state? If not, disregard :) 

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go
##########
@@ -115,3 +118,70 @@ func generatePutRiakKeys(req 
tc.DeliveryServiceGenSSLKeysReq, tx *sql.Tx, tv tra
        }
        return nil
 }
+
+// GeneratePlaceholderSelfSignedCert generates a self-signed SSL certificate 
as a placeholder when a new HTTPS
+// delivery service is created or an HTTP delivery service is updated to use 
HTTPS.
+func GeneratePlaceholderSelfSignedCert(ds tc.DeliveryServiceV40, inf 
*api.APIInfo, context context.Context) (error, int) {
+       version := util.JSONIntStr(1)
+
+       db, err := api.GetDB(context)
+       if err != nil {
+               return err, http.StatusInternalServerError
+       }
+       tx, err := db.Begin()
+       if err != nil {
+               return err, http.StatusInternalServerError
+       }
+       defer tx.Commit()
+
+       cdnName, cdnDomain, err := getCDNNameDomain(*ds.CDNID, tx)
+       if err != nil {
+               return err, http.StatusInternalServerError
+       }
+
+       cdnNameStr := string(cdnName)
+
+       if ds.ExampleURLs == nil {
+               ds.ExampleURLs = MakeExampleURLs(ds.Protocol, *ds.Type, 
*ds.RoutingName, *ds.MatchList, cdnDomain)
+       }
+
+       hostname := strings.Split(ds.ExampleURLs[0], "://")[1]
+       if strings.Contains(ds.Type.String(), "HTTP") {
+               parts := strings.Split(hostname, ".")
+               parts[0] = "*"
+               hostname = strings.Join(parts, ".")
+       }
+
+       req := tc.DeliveryServiceGenSSLKeysReq{
+               DeliveryServiceSSLKeysReq: tc.DeliveryServiceSSLKeysReq{
+                       CDN:             &cdnNameStr,
+                       DeliveryService: ds.XMLID,
+                       HostName:        &hostname,
+                       Key:             ds.XMLID,
+                       Version:         &version,
+                       BusinessUnit:    util.StrPtr("Placeholder"),
+                       City:            util.StrPtr("Placeholder"),
+                       Organization:    util.StrPtr("Placeholder"),
+                       Country:         util.StrPtr("United States (US)"),
+                       State:           util.StrPtr("CO"),
+               },
+       }
+       if err := GeneratePutRiakKeys(req, tx, inf.Vault, context); err != nil {
+               return errors.New("generating and putting SSL keys: " + 
err.Error()), http.StatusInternalServerError
+       }
+       if err := updateSSLKeyVersion(*req.DeliveryService, 
req.Version.ToInt64(), tx); err != nil {
+               return errors.New("generating SSL keys for delivery service '" 
+ *req.DeliveryService + "': " + err.Error()), http.StatusInternalServerError
+       }
+
+       return nil, http.StatusOK
+}
+
+func getCDNNameDomain(cdnID int, tx *sql.Tx) (string, string, error) {

Review comment:
       This function should probably go in db_helpers.go since it might be 
useful to other packages as well.

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -878,6 +888,16 @@ func UpdateV40(w http.ResponseWriter, r *http.Request) {
        }
        alerts := res.TLSVersionsAlerts()
        alerts.AddNewAlert(tc.SuccessLevel, "Delivery Service update was 
successful")
+
+       if inf.Config.TrafficVaultEnabled && (*ds.Protocol == 
tc.DSProtocolHTTPS || *ds.Protocol == tc.DSProtocolHTTPAndHTTPS) && 
(ds.SSLKeyVersion == nil || *ds.SSLKeyVersion == 0) {

Review comment:
       ditto my prior comment, except `updateV40` instead of `createv40` on the 
last point

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -277,6 +277,16 @@ func CreateV40(w http.ResponseWriter, r *http.Request) {
        }
        alerts := res.TLSVersionsAlerts()
        alerts.AddNewAlert(tc.SuccessLevel, "Delivery Service creation was 
successful")
+
+       if inf.Config.TrafficVaultEnabled && (*ds.Protocol == 
tc.DSProtocolHTTPS || *ds.Protocol == tc.DSProtocolHTTPAndHTTPS) && 
(ds.SSLKeyVersion == nil || *ds.SSLKeyVersion == 0) {

Review comment:
       I wonder if we should also include `DSProtocolHTTPToHTTPS` even though 
this is about preventing TR from spinning. That way, we can say any DS that 
uses HTTPS will have a default cert generated for it.
   
   Also, I'm not sure checking `ds.SSLKeyVersion == nil || *ds.SSLKeyVersion == 
0` is really necessary, since this DS can't possibly have certs assigned yet 
because the DS doesn't exist yet. However, I suppose if it were deleted and 
recreated (which needs to happen from time to time), it might have certs 
already. Should we attempt to pull the `latest` cert from Traffic Vault, and if 
it doesn't exist, we can generate the default one?
   
   Also, this check and cert generation should probably go into `createV40`, 
because we'd want this to be done for all API versions, right?

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go
##########
@@ -87,9 +90,9 @@ func GenerateSSLKeys(w http.ResponseWriter, r *http.Request) {
        api.WriteResp(w, r, "Successfully created ssl keys for 
"+*req.DeliveryService)
 }
 
-// generatePutRiakKeys generates a certificate, csr, and key from the given 
request, and insert it into the Riak key database.
+// GeneratePutRiakKeys generates a certificate, csr, and key from the given 
request, and insert it into the Riak key database.
 // The req MUST be validated, ensuring required fields exist.
-func generatePutRiakKeys(req tc.DeliveryServiceGenSSLKeysReq, tx *sql.Tx, tv 
trafficvault.TrafficVault, ctx context.Context) error {
+func GeneratePutRiakKeys(req tc.DeliveryServiceGenSSLKeysReq, tx *sql.Tx, tv 
trafficvault.TrafficVault, ctx context.Context) error {

Review comment:
       Why make this public? Since `GeneratePlaceholderSelfSignedCert` is 
public and in the same package, it could still make use of 
`GeneratePutRiakKeys` if it were private.
   
   Also, we should probably rename this to `generatePutTrafficVaultSSLKeys` and 
update the comment since this isn't Riak-specific anymore.

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go
##########
@@ -115,3 +118,70 @@ func generatePutRiakKeys(req 
tc.DeliveryServiceGenSSLKeysReq, tx *sql.Tx, tv tra
        }
        return nil
 }
+
+// GeneratePlaceholderSelfSignedCert generates a self-signed SSL certificate 
as a placeholder when a new HTTPS
+// delivery service is created or an HTTP delivery service is updated to use 
HTTPS.
+func GeneratePlaceholderSelfSignedCert(ds tc.DeliveryServiceV40, inf 
*api.APIInfo, context context.Context) (error, int) {
+       version := util.JSONIntStr(1)
+
+       db, err := api.GetDB(context)
+       if err != nil {
+               return err, http.StatusInternalServerError
+       }
+       tx, err := db.Begin()
+       if err != nil {
+               return err, http.StatusInternalServerError
+       }
+       defer tx.Commit()
+
+       cdnName, cdnDomain, err := getCDNNameDomain(*ds.CDNID, tx)
+       if err != nil {
+               return err, http.StatusInternalServerError
+       }
+
+       cdnNameStr := string(cdnName)
+
+       if ds.ExampleURLs == nil {
+               ds.ExampleURLs = MakeExampleURLs(ds.Protocol, *ds.Type, 
*ds.RoutingName, *ds.MatchList, cdnDomain)
+       }
+
+       hostname := strings.Split(ds.ExampleURLs[0], "://")[1]
+       if strings.Contains(ds.Type.String(), "HTTP") {

Review comment:
       I think we may be able to change this to `if ds.Type.IsHTTP()`




-- 
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]


Reply via email to