XRDDEV-824: do not allow generating a new CSR for a key that has an e…

…xisting certificate (#1821)

* feat: do not allow generating new CSR for keys with certificate

Refs: XRDDEV-824

* chore: comment fox in rhel packaging file (xroad-base.spec)

* chore: minor fixes

Refs: XRDDEV-824

* chore: method extracted

Refs: XRDDEV-824

* chore: sonar fix

Refs: XRDDEV-824

doc/Manuals/ug-syspar_x-road_v6_system_parameters.md

@@ -1,6 +1,6 @@
 # X-Road: System Parameters User Guide
 
-Version: 2.77
+Version: 2.78
 Doc. ID: UG-SYSPAR
 
 
@@ -88,6 +88,7 @@ Doc. ID: UG-SYSPAR
 | 13.06.2023 | 2.75    | Added new *complementary-user-role-mappings* parameters                                                                                                                                                                                                                                                                                                                                                        | Andres Rosenthal           |
 | 24.08.2023 | 2.76    | Added new *server-min-supported-client-version* parameter                                                                                                                                                                                                                                                                                                                                                      | Eneli Reimets              |
 | 02.10.2023 | 2.77    | Remove the separate section about changing the global configuration interval on the Central Server.                                                                                                                                                                                                                                                                                                            | Petteri Kivimäki           |
+| 16.10.2023 | 2.78    | Added new *allow-csr-for-key-with-certificate* parameter                                                                                                                                                                                                                                                                                                                                                       | Justas Samuolis            |
 
 ## Table of Contents
 
@@ -420,6 +421,7 @@ the message log.
 | cache-api-key-ttl                           | 60                        | Configures Api Key cache expiration in seconds. Can be used by various api services. Setting the value to -1 disables the cache.                                                                                                                                                                                                                                                                                                                                                                                     |
 | strict-identifier-checks                    | true*                     | Configures identifier input validation (member code, subsystem code etc).<br>Values:<br>`true`- identifiers must mach pattern `^[a-zA-Z0-9'()+,-.=?]*`.<br>`false`- identfiers must not contain colon (`:`), semicolon (`;`), slashes (`/\`), percent (`%`) or control characters.                                                                                                                                                                                                                                   |
 | complementary-user-role-mappings            |                           | Configures additional UNIX groups mapped to X-Road user roles. **Example:** *XROAD_SECURITY_OFFICER=group1,group2*<br/>**Note that following configurations are preconfigured and cannot be redefined:**<br/>*XROAD_SECURITY_OFFICER=xroad-security-officer*<br/>*XROAD_REGISTRATION_OFFICER=xroad-registration-officer*<br/>*XROAD_SERVICE_ADMINISTRATOR=xroad-service-administrator*<br/>*XROAD_SYSTEM_ADMINISTRATOR=xroad-system-administrator*<br/>*XROAD_SECURITYSERVER_OBSERVER=xroad-securityserver-observer* |
+| allow-csr-for-key-with-certificate          | false                     | Controls if generating the new CSR is allowed for a key that has an existing certificate.                                                                                                                                                                                                                                                                                                                                                                                                                            |
 
 > **NOTE**: `strict-identifier-checks` default value is true for new installations starting from version 7.3.0. It is 
 > set to `false` in `local.ini` during upgrade process if version installed before upgrade is less than 7.3.0.

src/common/common-util/src/main/java/ee/ria/xroad/common/SystemProperties.java

@@ -96,6 +96,10 @@ private SystemProperties() {
     public static final String PROXY_UI_API_AUTO_UPDATE_TIMESTAMP_SERVICE_URL =
             PREFIX + "proxy-ui-api.auto-update-timestamp-service-url";
 
+    /** property name of the flag to allow generating csr for key with certificates */
+    public static final String PROXY_UI_API_ALLOW_CSR_FOR_KEY_WITH_CERTIFICATE =
+            PREFIX + "proxy-ui-api.allow-csr-for-key-with-certificate";
+
     // Proxy ------------------------------------------------------------------
 
     /** Property name of controlling SSL support between Proxies. */
@@ -300,6 +304,8 @@ private SystemProperties() {
 
     private static final String DEFAULT_AUTO_UPDATE_TIMESTAMP_SERVICE_URL = "false";
 
+    private static final String DEFAULT_ALLOW_CSR_FOR_KEY_WITH_CERTIFICATE = "false";
+
     private static final String DEFAULT_SERVERPROXY_CONNECTOR_MAX_IDLE_TIME = "0";
 
     private static final String DEFAULT_PROXY_CONNECTOR_INITIAL_IDLE_TIME = "30000";
@@ -755,6 +761,14 @@ public static boolean geUpdateTimestampServiceUrlsAutomatically() {
                 DEFAULT_AUTO_UPDATE_TIMESTAMP_SERVICE_URL));
     }
 
+    /**
+     * @return whether generating CSR is allowed for with existing certificate, 'false' by default
+     */
+    public static boolean getAllowCsrForKeyWithCertificate() {
+        return Boolean.parseBoolean(System.getProperty(PROXY_UI_API_ALLOW_CSR_FOR_KEY_WITH_CERTIFICATE,
+                DEFAULT_ALLOW_CSR_FOR_KEY_WITH_CERTIFICATE));
+    }
+
     /**
      * @return path to the configuration anchor file, '/etc/xroad/configuration-anchor.xml' by default.
      */

src/packages/src/xroad/redhat/SPECS/xroad-base.spec

@@ -120,7 +120,7 @@ if [ $1 -gt 1 ] ; then
       fi
     fi
 
-    # 7.3.0 remove JAVA_HOME from local.conf if it points to java < 11
+    # 7.4.0 remove JAVA_HOME from local.conf if it points to java < 17
     if [ -f /etc/xroad/services/local.conf ]; then
       java_home=$(grep -oP '^\s*JAVA_HOME=\K(.*)' /etc/xroad/services/local.conf | tail -n 1)
       if [ -n "$java_home" ]; then

src/security-server/admin-service/application/src/main/java/org/niis/xroad/securityserver/restapi/service/PossibleActionsRuleEngine.java

@@ -25,6 +25,7 @@
  */
 package org.niis.xroad.securityserver.restapi.service;
 
+import ee.ria.xroad.common.SystemProperties;
 import ee.ria.xroad.common.util.CertUtils;
 import ee.ria.xroad.common.util.CryptoUtils;
 import ee.ria.xroad.signer.protocol.dto.CertRequestInfo;
@@ -39,6 +40,8 @@
 import java.security.cert.X509Certificate;
 import java.util.EnumSet;
 
+import static org.springframework.util.CollectionUtils.isEmpty;
+
 /**
  * Validation logic for possible actions done on tokens, keys, certs and csrs
  */
@@ -88,7 +91,7 @@ public boolean isKeyUnsupported(TokenInfo tokenInfo, KeyInfo keyInfo) {
      * Get possible actions for a key
      */
     public EnumSet<PossibleActionEnum> getPossibleKeyActions(TokenInfo tokenInfo,
-            KeyInfo keyInfo) {
+                                                             KeyInfo keyInfo) {
         EnumSet<PossibleActionEnum> actions = EnumSet.noneOf(PossibleActionEnum.class);
 
         // DELETE
@@ -107,12 +110,14 @@ public EnumSet<PossibleActionEnum> getPossibleKeyActions(TokenInfo tokenInfo,
         // keys.js#35
         if (SOFTWARE_TOKEN_ID.equals(tokenInfo.getId())
                 && (keyInfo.getUsage() == null || keyInfo.getUsage() == KeyUsageInfo.AUTHENTICATION)
-                && !(!keyInfo.isAvailable() || !tokenInfo.isActive() || keyNotSupported)) {
+                && !(!keyInfo.isAvailable() || !tokenInfo.isActive() || keyNotSupported)
+                && keyHasNoCertificatesOrGenerateCsrAllowedInProperties(keyInfo)) {
             actions.add(PossibleActionEnum.GENERATE_AUTH_CSR);
         }
         // GENERATE_SIGN_CSR
         if ((keyInfo.getUsage() == null || keyInfo.getUsage() == KeyUsageInfo.SIGNING)
-                && !(!keyInfo.isAvailable() || !tokenInfo.isActive() || keyNotSupported)) {
+                && !(!keyInfo.isAvailable() || !tokenInfo.isActive() || keyNotSupported)
+                && keyHasNoCertificatesOrGenerateCsrAllowedInProperties(keyInfo)) {
             actions.add(PossibleActionEnum.GENERATE_SIGN_CSR);
         }
         // EDIT_FRIENDLY_NAME
@@ -121,12 +126,16 @@ public EnumSet<PossibleActionEnum> getPossibleKeyActions(TokenInfo tokenInfo,
         return actions;
     }
 
+    private static boolean keyHasNoCertificatesOrGenerateCsrAllowedInProperties(KeyInfo keyInfo) {
+        return isEmpty(keyInfo.getCerts()) || SystemProperties.getAllowCsrForKeyWithCertificate();
+    }
+
     /**
      * get possible actions for a certificate
      */
     public EnumSet<PossibleActionEnum> getPossibleCertificateActions(TokenInfo tokenInfo,
-            KeyInfo keyInfo,
-            CertificateInfo certificateInfo) {
+                                                                     KeyInfo keyInfo,
+                                                                     CertificateInfo certificateInfo) {
         EnumSet<PossibleActionEnum> actions = EnumSet.noneOf(PossibleActionEnum.class);
         boolean canUnregister = keyInfo.getUsage() == KeyUsageInfo.AUTHENTICATION
                 && (CertificateInfo.STATUS_REGINPROG.equals(certificateInfo.getStatus())
@@ -191,8 +200,8 @@ public EnumSet<PossibleActionEnum> getPossibleCsrActions(TokenInfo tokenInfo) {
      * combined logic from keys.js and token_renderer.rb
      */
     private boolean canDeleteCertOrCsr(TokenInfo tokenInfo,
-            boolean savedToConfiguration,
-            boolean canUnregister) {
+                                       boolean savedToConfiguration,
+                                       boolean canUnregister) {
 
         // token_renderer.rb#230
         boolean canDeleteCertFromTokenRenderer;
@@ -214,6 +223,7 @@ private boolean canDeleteCertOrCsr(TokenInfo tokenInfo,
 
     /**
      * Shortcut helper method for verifying required action
+     *
      * @throws ActionNotPossibleException if given action is not in possibleActions
      */
     public void requirePossibleAction(PossibleActionEnum action, EnumSet<PossibleActionEnum> possibleActions)
@@ -225,6 +235,7 @@ public void requirePossibleAction(PossibleActionEnum action, EnumSet<PossibleAct
 
     /**
      * Shortcut helper method for verifying required action
+     *
      * @throws ActionNotPossibleException if given token action is not possible
      */
     public void requirePossibleTokenAction(PossibleActionEnum action, TokenInfo tokenInfo)
@@ -234,6 +245,7 @@ public void requirePossibleTokenAction(PossibleActionEnum action, TokenInfo toke
 
     /**
      * Shortcut helper method for verifying required action
+     *
      * @throws ActionNotPossibleException if given key action is not possible
      */
     public void requirePossibleKeyAction(PossibleActionEnum action, TokenInfo tokenInfo, KeyInfo keyInfo)
@@ -243,26 +255,29 @@ public void requirePossibleKeyAction(PossibleActionEnum action, TokenInfo tokenI
 
     /**
      * Shortcut helper method for verifying required action
+     *
      * @throws ActionNotPossibleException if given certificate action is not possible
      */
     public void requirePossibleCertificateAction(PossibleActionEnum action, TokenInfo tokenInfo, KeyInfo keyInfo,
-            CertificateInfo certificateInfo)
+                                                 CertificateInfo certificateInfo)
             throws ActionNotPossibleException {
         requirePossibleAction(action, ActionTargetType.CERTIFICATE, tokenInfo, keyInfo, certificateInfo, null);
     }
 
     /**
      * Shortcut helper method for verifying required action
+     *
      * @throws ActionNotPossibleException if given csr action is not possible
      */
     public void requirePossibleCsrAction(PossibleActionEnum action, TokenInfo tokenInfo, KeyInfo keyInfo,
-            CertRequestInfo certRequestInfo)
+                                         CertRequestInfo certRequestInfo)
             throws ActionNotPossibleException {
         requirePossibleAction(action, ActionTargetType.CSR, tokenInfo, keyInfo, null, certRequestInfo);
     }
 
     /**
      * Shortcut helper method for verifying required action
+     *
      * @throws ActionNotPossibleException if given action is not possible for give target type
      */
     public void requirePossibleAction(PossibleActionEnum action, ActionTargetType target,

src/security-server/admin-service/application/src/test/java/org/niis/xroad/securityserver/restapi/service/PossibleActionsRuleEngineTest.java

@@ -25,6 +25,7 @@
  */
 package org.niis.xroad.securityserver.restapi.service;
 
+import ee.ria.xroad.common.SystemProperties;
 import ee.ria.xroad.signer.protocol.dto.CertificateInfo;
 import ee.ria.xroad.signer.protocol.dto.KeyInfo;
 import ee.ria.xroad.signer.protocol.dto.KeyUsageInfo;
@@ -424,6 +425,33 @@ public void getPossibleKeyActionGenerateAuthCsr() {
         assertTrue(actions.contains(PossibleActionEnum.GENERATE_AUTH_CSR));
         assertTrue(actions.contains(PossibleActionEnum.GENERATE_SIGN_CSR));
 
+        // not possible if key has certificate and system property not set (using default)
+        tokenInfo = new TokenTestUtils.TokenInfoBuilder()
+                .id(PossibleActionsRuleEngine.SOFTWARE_TOKEN_ID)
+                .key(new TokenTestUtils.KeyInfoBuilder()
+                        .keyUsageInfo(null)
+                        .cert(new CertificateTestUtils.CertificateInfoBuilder().build())
+                        .build())
+                .build();
+        actions = getPossibleKeyActions(tokenInfo);
+        assertFalse(actions.contains(PossibleActionEnum.GENERATE_AUTH_CSR));
+        assertFalse(actions.contains(PossibleActionEnum.GENERATE_SIGN_CSR));
+
+        // possible if key has certificate and system property is set to allow csr
+        boolean currValue = SystemProperties.getAllowCsrForKeyWithCertificate();
+        System.setProperty(SystemProperties.PROXY_UI_API_ALLOW_CSR_FOR_KEY_WITH_CERTIFICATE, "true");
+        tokenInfo = new TokenTestUtils.TokenInfoBuilder()
+                .id(PossibleActionsRuleEngine.SOFTWARE_TOKEN_ID)
+                .key(new TokenTestUtils.KeyInfoBuilder()
+                        .keyUsageInfo(null)
+                        .cert(new CertificateTestUtils.CertificateInfoBuilder().build())
+                        .build())
+                .build();
+        actions = getPossibleKeyActions(tokenInfo);
+        assertTrue(actions.contains(PossibleActionEnum.GENERATE_AUTH_CSR));
+        assertTrue(actions.contains(PossibleActionEnum.GENERATE_SIGN_CSR));
+        System.setProperty(SystemProperties.PROXY_UI_API_ALLOW_CSR_FOR_KEY_WITH_CERTIFICATE, String.valueOf(currValue));
+
         // not possible if token is not softtoken
         tokenInfo = new TokenTestUtils.TokenInfoBuilder()
                 .id(PossibleActionsRuleEngine.SOFTWARE_TOKEN_ID + 1)
@@ -502,6 +530,31 @@ public void getPossibleKeyActionGenerateSignCsr() {
         actions = getPossibleKeyActions(tokenInfo);
         assertTrue(actions.contains(PossibleActionEnum.GENERATE_SIGN_CSR));
 
+        // not possible if key has certificate
+        tokenInfo = new TokenTestUtils.TokenInfoBuilder()
+                .key(new TokenTestUtils.KeyInfoBuilder()
+                        .keyUsageInfo(null)
+                        .cert(new CertificateTestUtils.CertificateInfoBuilder().build())
+                        .available(true)
+                        .build())
+                .build();
+        actions = getPossibleKeyActions(tokenInfo);
+        assertFalse(actions.contains(PossibleActionEnum.GENERATE_SIGN_CSR));
+
+        // possible if key has certificate and allowed in properties
+        boolean currVal = SystemProperties.getAllowCsrForKeyWithCertificate();
+        System.setProperty(SystemProperties.PROXY_UI_API_ALLOW_CSR_FOR_KEY_WITH_CERTIFICATE, "true");
+        tokenInfo = new TokenTestUtils.TokenInfoBuilder()
+                .key(new TokenTestUtils.KeyInfoBuilder()
+                        .keyUsageInfo(null)
+                        .cert(new CertificateTestUtils.CertificateInfoBuilder().build())
+                        .available(true)
+                        .build())
+                .build();
+        actions = getPossibleKeyActions(tokenInfo);
+        assertTrue(actions.contains(PossibleActionEnum.GENERATE_SIGN_CSR));
+        System.setProperty(SystemProperties.PROXY_UI_API_ALLOW_CSR_FOR_KEY_WITH_CERTIFICATE, String.valueOf(currVal));
+
         // not possible if usage = auth
         tokenInfo = new TokenTestUtils.TokenInfoBuilder()
                 .active(true)

src/security-server/admin-service/ui/src/views/KeysAndCertificates/SignAndAuthKeys/KeyRow.vue

@@ -24,7 +24,7 @@
    THE SOFTWARE.
  -->
 <template>
-  <tr>
+  <tr data-test="key-row">
     <td class="pl-8">
       <div class="name-wrap-top">
         <i class="icon-Key key-icon" />
@@ -44,6 +44,7 @@
         :outlined="false"
         text
         :disabled="disableGenerateCsr"
+        data-test="generate-csr-button"
         @click="generateCsr"
         >{{ $t('keys.generateCsr') }}</xrd-button
       >

src/security-server/system-test/src/intTest/java/org/niis/xroad/ss/test/ui/glue/KeyAndCertStepDefs.java

@@ -44,6 +44,7 @@
 import static com.codeborne.selenide.Condition.enabled;
 import static com.codeborne.selenide.Condition.text;
 import static com.codeborne.selenide.Condition.visible;
+import static org.apache.commons.lang3.RandomStringUtils.randomAlphabetic;
 
 @Slf4j
 public class KeyAndCertStepDefs extends BaseUiStepDefs {
@@ -163,6 +164,31 @@ public void setAuthCsrDetails(String usage, String client, String certificationS
         keyAndCertPageObj.addKeyWizardCsrDetails.continueButton().shouldBe(visible).click();
     }
 
+    @SuppressWarnings("checkstyle:MagicNumber")
+    @Step("CSR is generated for token {string}, key {string}, certification service {string}, format {string}")
+    public void csrIsGeneratedForKeyCertificationServiceFormat(String token, String key, String certService, String csrFormat) {
+        keyAndCertPageObj.section(token).tokenLabeledKeyGenerateCsrButton(key).shouldBe(enabled).click();
+
+        keyAndCertPageObj.addKeyWizardCsrDetails.csrService().click();
+        keyAndCertPageObj.addKeyWizardCsrDetails.selectorOptionOf(certService).click();
+
+        keyAndCertPageObj.addKeyWizardCsrDetails.csrFormat().click();
+        keyAndCertPageObj.addKeyWizardCsrDetails.selectorOptionOf(csrFormat).click();
+
+        keyAndCertPageObj.addKeyWizardCsrDetails.continueButton().shouldBe(visible).click();
+
+        keyAndCertPageObj.addKeyWizardGenerate.serverDNS().setValue("ss1");
+        keyAndCertPageObj.addKeyWizardGenerate.organizationName().setValue(randomAlphabetic(10));
+
+        keyAndCertPageObj.addKeyWizardGenerate.generateButton().click();
+        keyAndCertPageObj.addKeyWizardGenerate.doneButton().click();
+    }
+
+    @Step("Token {string}, key {string} has {int} certificate signing requests")
+    public void tokenKeyHasCertificateSigningRequests(String token, String keyLabel, int count) {
+        keyAndCertPageObj.section(token).labeledKeyCsrRows(keyLabel).shouldBe(CollectionCondition.size(count));
+    }
+
     @SneakyThrows
     @Step("Generate CSR is set to DNS {string}, Organization {string} and CSR successfully generated")
     public void setGenerateCsr(String dns, String organization) {
@@ -179,7 +205,6 @@ public void setGenerateCsr(String dns, String organization) {
         putStepData(StepDataKey.DOWNLOADED_FILE, certReq);
 
         keyAndCertPageObj.addKeyWizardGenerate.doneButton().click();
-
     }
 
     @Step("Token: {} - has key with label {string}")
@@ -225,4 +250,10 @@ public void deleteCsr(String tokenKey, String type, int pos) {
         btnDelete.click();
         commonPageObj.dialog.btnSave().click();
     }
+
+    @Step("Token: {}, key {string} generate CSR button is disabled")
+    public void tokenKeyGenerateCSRButtonIsDisabled(String token, String keyLabel) {
+        keyAndCertPageObj.section(token).tokenLabeledKeyGenerateCsrButton(keyLabel).shouldBe(disabled);
+    }
+
 }