[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #9572: Add logic to make the server.policy and server.properties settings reloadable

["brbzull0 (via GitHub)" <gi...@apache.org>]

brbzull0 commented on code in PR #9572:
URL: https://github.com/apache/trafficserver/pull/9572#discussion_r1155826324


##########
iocore/net/SSLConfig.cc:
##########
@@ -189,6 +189,65 @@ set_paths_helper(const char *path, const char *filename, char **final_path, char
   }
 }
 
+int
+UpdateServerPolicy(const char * /* name ATS_UNUSED */, RecDataT /* data_type ATS_UNUSED */, RecData data, void *cookie)
+{
+  SSLConfigParams *params = SSLConfig::acquire();
+  char *verify_server     = data.rec_string;
+  if (params != nullptr && verify_server != nullptr) {
+    Debug("ssl_load", "New Server Policy %s", verify_server);
+    params->SetServerPolicy(verify_server);
+  } else {
+    Debug("ssl_load", "Failed to load new Server Policy %p %p", verify_server, params);
+  }
+  return 0;
+}
+
+int
+UpdateServerPolicyProperties(const char * /* name ATS_UNUSED */, RecDataT /* data_type ATS_UNUSED */, RecData data, void *cookie)
+{
+  SSLConfigParams *params = SSLConfig::acquire();
+  char *verify_server     = data.rec_string;
+  if (params != nullptr && verify_server != nullptr) {
+    params->SetServerPolicyProperties(verify_server);
+  }
+  return 0;
+}
+
+void
+SSLConfigParams::SetServerPolicyProperties(const char *verify_server)
+{
+  if (strcmp(verify_server, "SIGNATURE") == 0) {
+    verifyServerProperties = YamlSNIConfig::Property::SIGNATURE_MASK;
+  } else if (strcmp(verify_server, "NAME") == 0) {
+    verifyServerProperties = YamlSNIConfig::Property::NAME_MASK;
+  } else if (strcmp(verify_server, "ALL") == 0) {
+    verifyServerProperties = YamlSNIConfig::Property::ALL_MASK;
+  } else if (strcmp(verify_server, "NONE") == 0) {
+    verifyServerProperties = YamlSNIConfig::Property::NONE;
+  } else {
+    Warning("%s is invalid for proxy.config.ssl.client.verify.server.properties.  Should be one of SIGNATURE, NAME, or ALL",
+            verify_server);
+    verifyServerProperties = YamlSNIConfig::Property::NONE;
+  }
+}
+
+void
+SSLConfigParams::SetServerPolicy(const char *verify_server)
+{
+  if (strcmp(verify_server, "DISABLED") == 0) {
+    verifyServerPolicy = YamlSNIConfig::Policy::DISABLED;
+  } else if (strcmp(verify_server, "PERMISSIVE") == 0) {
+    verifyServerPolicy = YamlSNIConfig::Policy::PERMISSIVE;
+  } else if (strcmp(verify_server, "ENFORCED") == 0) {
+    verifyServerPolicy = YamlSNIConfig::Policy::ENFORCED;
+  } else {
+    Warning("%s is invalid for proxy.config.ssl.client.verify.server.policy.  Should be one of DISABLED, PERMISSIVE, or ENFORCED",

Review Comment:
   Same here, I would mention that this is now `DISABLED`



##########
iocore/net/SSLConfig.cc:
##########
@@ -189,6 +189,65 @@ set_paths_helper(const char *path, const char *filename, char **final_path, char
   }
 }
 
+int
+UpdateServerPolicy(const char * /* name ATS_UNUSED */, RecDataT /* data_type ATS_UNUSED */, RecData data, void *cookie)
+{
+  SSLConfigParams *params = SSLConfig::acquire();
+  char *verify_server     = data.rec_string;
+  if (params != nullptr && verify_server != nullptr) {
+    Debug("ssl_load", "New Server Policy %s", verify_server);
+    params->SetServerPolicy(verify_server);
+  } else {
+    Debug("ssl_load", "Failed to load new Server Policy %p %p", verify_server, params);
+  }
+  return 0;
+}
+
+int
+UpdateServerPolicyProperties(const char * /* name ATS_UNUSED */, RecDataT /* data_type ATS_UNUSED */, RecData data, void *cookie)
+{
+  SSLConfigParams *params = SSLConfig::acquire();
+  char *verify_server     = data.rec_string;
+  if (params != nullptr && verify_server != nullptr) {
+    params->SetServerPolicyProperties(verify_server);
+  }
+  return 0;
+}
+
+void
+SSLConfigParams::SetServerPolicyProperties(const char *verify_server)
+{
+  if (strcmp(verify_server, "SIGNATURE") == 0) {
+    verifyServerProperties = YamlSNIConfig::Property::SIGNATURE_MASK;
+  } else if (strcmp(verify_server, "NAME") == 0) {
+    verifyServerProperties = YamlSNIConfig::Property::NAME_MASK;
+  } else if (strcmp(verify_server, "ALL") == 0) {
+    verifyServerProperties = YamlSNIConfig::Property::ALL_MASK;
+  } else if (strcmp(verify_server, "NONE") == 0) {
+    verifyServerProperties = YamlSNIConfig::Property::NONE;
+  } else {
+    Warning("%s is invalid for proxy.config.ssl.client.verify.server.properties.  Should be one of SIGNATURE, NAME, or ALL",

Review Comment:
   I know this was already in the code but(not big issue) I would probably mention that `NONE` will be set in this case as per the docs  `ALL` seems the default, it could be misleading and someone may think `ALL` will be used in this case.



-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org