You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2023/01/12 21:12:15 UTC
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9301: Updated certifier plugin to generate certificate with SAN extension
bneradt commented on code in PR #9301:
URL: https://github.com/apache/trafficserver/pull/9301#discussion_r1068643538
##########
plugins/certifier/certifier.cc:
##########
@@ -316,45 +316,28 @@ static TSMutex serial_mutex; ///< serial number mutex
static std::unique_ptr<SslLRUList> ssl_list = nullptr;
static std::string store_path;
-/// Local helper function that generates a CSR based on common name
-static scoped_X509_REQ
-mkcsr(const char *cn)
+/// Local helper function that adds a Subject Alternative Name field into a
+/// certificate
+static void
+addSANExtToCert(X509 *cert, const std::string &dnsName)
Review Comment:
std::string_view
##########
plugins/certifier/certifier.cc:
##########
@@ -316,45 +316,28 @@ static TSMutex serial_mutex; ///< serial number mutex
static std::unique_ptr<SslLRUList> ssl_list = nullptr;
static std::string store_path;
-/// Local helper function that generates a CSR based on common name
-static scoped_X509_REQ
-mkcsr(const char *cn)
+/// Local helper function that adds a Subject Alternative Name field into a
+/// certificate
+static void
+addSANExtToCert(X509 *cert, const std::string &dnsName)
{
- TSDebug(PLUGIN_NAME, "Entering mkcsr()...");
- X509_NAME *n;
- scoped_X509_REQ req;
- req.reset(X509_REQ_new());
-
- /// Set X509 version
- X509_REQ_set_version(req.get(), 1);
-
- /// Get handle to subject name
- n = X509_REQ_get_subject_name(req.get());
-
- /// Set common name field
- if (X509_NAME_add_entry_by_txt(n, "CN", MBSTRING_ASC, (unsigned char *)cn, -1, -1, 0) != 1) {
- TSError("[%s] mkcsr(): Failed to add entry.", PLUGIN_NAME);
- return nullptr;
- }
- /// Set Traffic Server public key
- if (X509_REQ_set_pubkey(req.get(), ca_pkey_scoped.get()) != 1) {
- TSError("[%s] mkcsr(): Failed to set pubkey.", PLUGIN_NAME);
- return nullptr;
- }
- /// Sign with Traffic Server private key
- if (X509_REQ_sign(req.get(), ca_pkey_scoped.get(), EVP_sha256()) <= 0) {
- TSError("[%s] mkcsr(): Failed to Sign.", PLUGIN_NAME);
- return nullptr;
- }
- return req;
+ TSDebug(PLUGIN_NAME, "Adding SAN extension to the cert");
+ GENERAL_NAMES *generalNames = sk_GENERAL_NAME_new_null();
+ GENERAL_NAME *generalName = GENERAL_NAME_new();
+ ASN1_IA5STRING *ia5 = ASN1_IA5STRING_new();
+ ASN1_STRING_set(ia5, dnsName.data(), dnsName.length());
+ // generalName owns ia5 after this call
+ GENERAL_NAME_set0_value(generalName, GEN_DNS, ia5);
+ sk_GENERAL_NAME_push(generalNames, generalName);
+ X509_add1_ext_i2d(cert, NID_subject_alt_name, generalNames, 0, X509V3_ADD_DEFAULT);
+ sk_GENERAL_NAME_pop_free(generalNames, GENERAL_NAME_free);
}
-/// Local helper function that generates a X509 certificate based on CSR
+/// Local helper function that generates a X509 certificate based on SNI
static scoped_X509
-mkcrt(X509_REQ *req, int serial)
+mkcrt(const std::string &commonName, int serial)
Review Comment:
We generally make use of `std::string_view` instead of `const std::string&`.
##########
plugins/certifier/certifier.cc:
##########
@@ -316,45 +316,28 @@ static TSMutex serial_mutex; ///< serial number mutex
static std::unique_ptr<SslLRUList> ssl_list = nullptr;
static std::string store_path;
-/// Local helper function that generates a CSR based on common name
-static scoped_X509_REQ
-mkcsr(const char *cn)
+/// Local helper function that adds a Subject Alternative Name field into a
+/// certificate
Review Comment:
Let's add `@param[out] cert` and `@param[in] dnsName` docs. While adding those, probably best to use `/**` style docs.
##########
tests/gold_tests/pluginTest/certifier/certifier.test.py:
##########
@@ -0,0 +1,170 @@
+'''
+Test certifier plugin behaviors
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the # "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+import hashlib
+import os
+import re
+
+Test.Summary = '''
+Test certifier plugin behaviors
+'''
+# **testname is required**
+testName = ""
Review Comment:
What requires testName?
##########
tests/gold_tests/pluginTest/certifier/certifier.test.py:
##########
@@ -0,0 +1,170 @@
+'''
+Test certifier plugin behaviors
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the # "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+import hashlib
+import os
+import re
+
+Test.Summary = '''
+Test certifier plugin behaviors
+'''
+# **testname is required**
+testName = ""
+
+Test.SkipUnless(
+ Condition.PluginExists('certifier.so')
+)
+
+
+class DynamicCertTest:
+ httpsReplayFile = "replays/https.replay.yaml"
+ certPathSrc = os.path.join(Test.TestDirectory, "certs")
+ host = "www.tls.com"
+ certPathDest = ""
+
+ def __init__(self):
+ self.setupOriginServer()
+ self.setupTS()
+
+ def setupOriginServer(self):
+ self.server = Test.MakeVerifierServerProcess("verifier-server1", self.httpsReplayFile)
+
+ def setupTS(self):
+ self.ts = Test.MakeATSProcess("ts1", enable_tls=True)
+ self.ts.addDefaultSSLFiles()
+ # copy over the cert store in which the certs will be generated/stored
+ self.certPathDest = os.path.join(self.ts.Variables.CONFIGDIR, "certifier-certs")
+ Setup.Copy(self.certPathSrc, self.certPathDest)
+ self.ts.Disk.records_config.update({
+ "proxy.config.diags.debug.enabled": 1,
+ "proxy.config.diags.debug.tags": "http|certifier|ssl",
+ "proxy.config.ssl.server.cert.path": f'{self.ts.Variables.SSLDir}',
+ "proxy.config.ssl.server.private_key.path": f'{self.ts.Variables.SSLDir}',
+ })
+ self.ts.Disk.ssl_multicert_config.AddLine(
+ 'dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key'
+ )
+ self.ts.Disk.remap_config.AddLine(
+ f"map / http://127.0.0.1:{self.server.Variables.http_port}/",
+ )
+ self.ts.Disk.plugin_config.AddLine(
+ f'certifier.so -s {os.path.join(self.certPathDest, "store")} -m 1000 -c {os.path.join(self.certPathDest, "ca.cert")} -k {os.path.join(self.certPathDest, "ca.key")} -r {os.path.join(self.certPathDest, "ca-serial.txt")}')
+ # Verify logs for dynamic generation of certs
+ self.ts.Disk.traffic_out.Content += Testers.ContainsExpression(
+ "Creating shadow certs",
+ "Verify the certifier plugin generates the certificate dynamically.")
+
+ def runHTTPSTraffic(self):
+ tr = Test.AddTestRun("Test dynamic generation of certs")
+ tr.AddVerifierClientProcess(
+ "client1",
+ self.httpsReplayFile,
+ http_ports=[self.ts.Variables.port],
+ https_ports=[self.ts.Variables.ssl_port],
+ other_args='--thread-limit 1')
+ tr.Processes.Default.StartBefore(self.server)
+ tr.Processes.Default.StartBefore(self.ts)
+ tr.StillRunningAfter = self.server
+ tr.StillRunningAfter = self.ts
+
+ def verifyCert(self, certPath):
+ tr = Test.AddTestRun("Verify the content of the generated cert")
+ tr.Processes.Default.Command = f'openssl x509 -in {certPath} -text -noout'
+ tr.Processes.Default.ReturnCode = 0
+ # Verifiy certificate content
+ tr.Processes.Default.Streams.All += Testers.ContainsExpression(
+ "Subject: CN = www.tls.com", "Subject should match the host in the request")
+ tr.Processes.Default.Streams.All += Testers.ContainsExpression(
+ r"X509v3 extensions:\n.*X509v3 Subject Alternative Name:.*\n.*DNS:www.tls.com",
+ "Should contain the SAN extension",
+ reflags=re.MULTILINE)
+
+ def run(self):
+ # the certifier plugin generates the cert and store it in a directory
+ # named with the first three character of the md5 hash of the hostname
+ genCertDirPath = os.path.join(self.certPathDest,
+ 'store',
+ str(hashlib.md5(self.host.encode('utf-8')).hexdigest()[:3]))
+ genCertPath = os.path.join(genCertDirPath, self.host + ".crt")
+ genCertDir = Test.Disk.Directory(genCertDirPath)
+ genCertFile = Test.Disk.File(genCertPath)
+ genCertDir.Exists = False
+ genCertFile.Exists = False
+ self.runHTTPSTraffic()
+ genCertDir.Exists = True
+ genCertFile.Exists = True
Review Comment:
I believe the latter Exists calls are simply overriding the former Exists ones. This can be a bit confusing, but AuTest is declarative rather than procedural in nature. Thus `genCertDir.Exists = True` isn't saying, "At this point in the code, check that the file exists. Rather it is saying, "At the end of the test run, check that `genCertDir` exists.
--
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