Changeset 105136 in vbox

Timestamp:

Jul 4, 2024 9:02:33 AM (5 months ago)

Author:

vboxsync

Message:

Main/VRDEServerImpl.cpp: Consider the autogenerated certificate 'invalid' if it expires within the next year. LogRel the auto certificate repair actions. Use RTPathFilename+RTStrICmp to match filenames, rather than strstr or RTCString::contains. Use #defines for the filenames to avoid accidental typos. bugref:10310

File:

• trunk/src/VBox/Main/src-server/VRDEServerImpl.cpp (modified) (8 diffs)

trunk/src/VBox/Main/src-server/VRDEServerImpl.cpp

@@ -55 +55 @@
 #define VRDP_DEFAULT_PORT_STR "3389"
 
+#define VRDE_AUTO_GENENERATED_CERT_FILENAME "VRDEAutoGeneratedCert.pem"
+#define VRDE_AUTO_GENENERATED_PKEY_FILENAME "VRDEAutoGeneratedPrivateKey.pem"
+
+
 // constructor / destructor
 /////////////////////////////////////////////////////////////////////////////
@@ -238 +242 @@
  *
  * @note Locks this object for writing.
-*/
+ */
 int VRDEServer::i_generateServerCertificate()
 {
-    Utf8Str strServerCertificate("VRDEAutoGeneratedCert.pem");
+    Utf8Str strServerCertificate(VRDE_AUTO_GENENERATED_CERT_FILENAME);
     int vrc = mParent->i_calculateFullPath(strServerCertificate, strServerCertificate);
     AssertRCReturn(vrc, vrc);
 
-    Utf8Str strServerPrivateKey("VRDEAutoGeneratedPrivateKey.pem");
+    Utf8Str strServerPrivateKey(VRDE_AUTO_GENENERATED_PKEY_FILENAME);
     vrc = mParent->i_calculateFullPath(strServerPrivateKey, strServerPrivateKey);
     AssertRCReturn(vrc, vrc);
 
     AutoReadLock mlock(mParent COMMA_LOCKVAL_SRC_POS);
-    Utf8Str strVMName = mParent->i_getName();
+    Utf8Str const strVMName = mParent->i_getName();
     mlock.release();
 
@@ -286 +290 @@
 
 /**
- * Checks validity of auto-generated certificates, sets VRDE properties, and deletes files if necessary.
+ * Checks validity of auto-generated certificates, sets VRDE properties, and
+ * regenerates obsolete or missing files as necessary.
  *
  * @note Locks this object for writing.
@@ -292 +297 @@
 HRESULT VRDEServer::i_certificateRepair(BOOL &certificateGenerated)
 {
-    if ( (mData->mapProperties["Security/Method"] != "RDP" || mData->mapProperties["Security/Method"] != "None"))
-    {
-        Utf8Str strServerCertificate("VRDEAutoGeneratedCert.pem");
+    if (mData->mapProperties["Security/Method"] != "RDP" || mData->mapProperties["Security/Method"] != "None")
+    {
+        Utf8Str strServerCertificate(VRDE_AUTO_GENENERATED_CERT_FILENAME);
         int vrc = mParent->i_calculateFullPath(strServerCertificate, strServerCertificate);
         AssertRCReturn(vrc, VBOX_E_IPRT_ERROR);
 
-        Utf8Str strServerPrivateKey("VRDEAutoGeneratedPrivateKey.pem");
+        Utf8Str strServerPrivateKey(VRDE_AUTO_GENENERATED_PKEY_FILENAME);
         vrc = mParent->i_calculateFullPath(strServerPrivateKey, strServerPrivateKey);
         AssertRCReturn(vrc, VBOX_E_IPRT_ERROR);
 
-        if ( RTFileExists(strServerPrivateKey.c_str()) && RTFileExists(strServerCertificate.c_str()) )
-        {
-            /* Check validity of certificate */
+        bool const fServerPrivateKeyExists = RTFileExists(strServerPrivateKey.c_str());
+        bool const fServerCertificate      = RTFileExists(strServerCertificate.c_str());
+        if (fServerPrivateKeyExists && fServerCertificate)
+        {
+            /*
+             * Check that the certificate is valid right now and for the next 365 days.
+             *
+             * The ASSUMPTIONS here are that the automatically generated certificates
+             * are valid for at least two years (currently ~10 years) and that VMs
+             * doesn't typically stay up more than a year before being completely
+             * restarted.  The latter assumption is of course a big one, as we've no
+             * control what users do here, but a year seems reasonable while not being
+             * too aggressive.
+             */
+            RTERRINFOSTATIC ErrInfo;
             RTCRX509CERTIFICATE certificate;
             vrc = RTCrX509Certificate_ReadFromFile(&certificate, strServerCertificate.c_str(), RTCRX509CERT_READ_F_PEM_ONLY,
-                                                   &g_RTAsn1DefaultAllocator, NULL);
-            if(RT_FAILURE(vrc))
+                                                   &g_RTAsn1DefaultAllocator, RTErrInfoInitStatic(&ErrInfo));
+            if (RT_FAILURE(vrc))
             {
                 RTCrX509Certificate_Delete(&certificate);
-                return setError(VBOX_E_IPRT_ERROR, tr("Failed to read server certificate: (%Rrc)\n"), vrc);
+                return setError(VBOX_E_IPRT_ERROR, tr("Failed to read server certificate '%s': %Rrc%#RTeim\n"),
+                                strServerCertificate.c_str(), vrc, &ErrInfo.Core);
             }
 
             RTTIMESPEC Now;
-            bool const validCert = RTCrX509Validity_IsValidAtTimeSpec(&certificate.TbsCertificate.Validity, RTTimeNow(&Now));
+            bool const validCert = RTCrX509Validity_IsValidAtTimeSpec(&certificate.TbsCertificate.Validity, RTTimeNow(&Now))
+                                && RTCrX509Validity_IsValidAtTimeSpec(&certificate.TbsCertificate.Validity,
+                                                                      RTTimeSpecAddSeconds(&Now, 365 * RT_SEC_1DAY_64));
 
             RTCrX509Certificate_Delete(&certificate);
 
-            Utf8Str strPath = mData->mapProperties["Security/ServerCertificate"];
-            if ( strPath.isEmpty() && validCert )
+            Utf8Str const strPath = mData->mapProperties["Security/ServerCertificate"];
+            if (validCert && strPath.isEmpty())
             {
                 /*
@@ -333 +353 @@
                 alock.release();
                 certificateGenerated = true;
+                LogRel(("VRDE: Reconfigured using existing '%s' and '%s' files.\n",
+                        strServerCertificate.c_str(), strServerPrivateKey.c_str()));
             }
-            else if ( (strPath.isEmpty() || strstr(strPath.c_str(),"VRDEAutoGeneratedCert.pem")) && !validCert)
+            else if (   !validCert
+                     && (strPath.isEmpty() || RTStrICmp(RTPathFilename(strPath.c_str()), VRDE_AUTO_GENENERATED_CERT_FILENAME)))
             {
-                /* Certificate is not valid so delete the files and create new ones */
+                /*
+                 * Certificate is not valid so delete the files and create new ones
+                 */
+                LogRel(("VRDE: Regenerating expired or expiring certificate files '%s' and '%s'...\n",
+                        strServerCertificate.c_str(), strServerPrivateKey.c_str()));
                 RTFileDelete(strServerPrivateKey.c_str());
                 RTFileDelete(strServerCertificate.c_str());
@@ -348 +375 @@
             }
         }
-        else if (RTFileExists(strServerPrivateKey.c_str())) /* If only one of cert/key pair exists */
-        {
+        /*
+         * If only one of cert/key pair exists, delete the file and generate a new matching.
+         */
+        else if (fServerPrivateKeyExists)
+        {
+            LogRel(("VRDE: Orphaned private key file found. Regenerating certificate files '%s' and '%s'...\n",
+                    strServerCertificate.c_str(), strServerPrivateKey.c_str()));
             RTFileDelete(strServerPrivateKey.c_str());
             vrc = i_generateServerCertificate();
@@ -359 +391 @@
             certificateGenerated = true;
         }
-        else if (RTFileExists(strServerCertificate.c_str()))
-        {
+        else if (fServerCertificate)
+        {
+            LogRel(("VRDE: Orphaned certificate file found. Regenerating certificate files '%s' and '%s'...\n",
+                    strServerCertificate.c_str(), strServerPrivateKey.c_str()));
             RTFileDelete(strServerCertificate.c_str());
             vrc = i_generateServerCertificate();
@@ -370 +404 @@
             certificateGenerated = true;
         }
+        /*
+         * Auto-generated certificate and key files do not exist
+         * If the server certificate property is not set
+         * or indicates an auto-generated certificate should exist, create one
+         */
         else
         {
-            /*
-             * Auto-generated certificate and key files do not exist
-             * If the server certificate property is not set
-             * or indicates an auto-generated certificate should exist, create one
-             */
-            Utf8Str strPath = mData->mapProperties["Security/ServerCertificate"];
-            if (strPath.isEmpty() || strstr(strPath.c_str(),"VRDEAutoGeneratedCert.pem"))
+            Utf8Str const strPath = mData->mapProperties["Security/ServerCertificate"];
+            if (strPath.isEmpty() || RTStrICmp(RTPathFilename(strPath.c_str()), VRDE_AUTO_GENENERATED_CERT_FILENAME) == 0)
             {
+                LogRel(("VRDE: Generating certificate files '%s' and '%s'...\n",
+                        strServerCertificate.c_str(), strServerPrivateKey.c_str()));
                 vrc = i_generateServerCertificate();
                 if (RT_FAILURE(vrc))