Merge pull request #2199 from nordic-institute/XRDDEV-2651

fix: ensure that manual key creation works in case ACME failures
nordic-institute · Jun 28, 2024 · e7b586a · e7b586a
2 parents 31b8d6e + 71822ef
commit e7b586a
Show file tree

Hide file tree

Showing 13 changed files with 160 additions and 148 deletions.
diff --git a/...n/java/org/niis/xroad/securityserver/restapi/converter/CertificateAuthorityConverter.java b/...n/java/org/niis/xroad/securityserver/restapi/converter/CertificateAuthorityConverter.java
@@ -59,7 +59,6 @@ public CertificateAuthority convert(ApprovedCaDto approvedCaDto) {
         ca.setPath(String.join(":", approvedCaDto.getSubjectDnPath()));
         ca.setTopCa(approvedCaDto.isTopCa());
         ca.acmeCapable(approvedCaDto.isAcmeCapable());
-        ca.acmeEabRequired(approvedCaDto.isAcmeEabRequired());
         ca.certificateProfileInfo(approvedCaDto.getCertificateProfileInfo());
         ca.acmeServerIpAddresses(ofNullable(approvedCaDto.getAcmeServerIpAddress())
                 .map(ips -> ips.split(","))

diff --git a/...ce/application/src/main/java/org/niis/xroad/securityserver/restapi/dto/ApprovedCaDto.java b/...ce/application/src/main/java/org/niis/xroad/securityserver/restapi/dto/ApprovedCaDto.java
@@ -49,6 +49,5 @@ public class ApprovedCaDto {
     private final List<String> subjectDnPath;
     private final String certificateProfileInfo;
     private final boolean acmeCapable;
-    private final boolean acmeEabRequired;
     private final String acmeServerIpAddress;
 }
diff --git a/...va/org/niis/xroad/securityserver/restapi/openapi/CertificateAuthoritiesApiController.java b/...va/org/niis/xroad/securityserver/restapi/openapi/CertificateAuthoritiesApiController.java
@@ -182,8 +182,13 @@ public ResponseEntity<Set<CsrSubjectFieldDescription>> getSubjectFieldDescriptio
     public ResponseEntity<AcmeEabCredentialsStatus> hasAcmeExternalAccountBindingCredentials(String caName,
                                                                                              KeyUsageType keyUsageType,
                                                                                              String memberId) {
-        boolean hasAcmeEabCredentials = certificateAuthorityService.hasAcmeExternalAccountBindingCredentials(caName, memberId);
-        return new ResponseEntity<>(new AcmeEabCredentialsStatus(hasAcmeEabCredentials), HttpStatus.OK);
+        try {
+            final var isAcmeEabRequired = certificateAuthorityService.isAcmeExternalAccountBindingRequired(caName);
+            final var hasAcmeEabCredentials = certificateAuthorityService.hasAcmeExternalAccountBindingCredentials(caName, memberId);
+            return new ResponseEntity<>(new AcmeEabCredentialsStatus(isAcmeEabRequired, hasAcmeEabCredentials), HttpStatus.OK);
+        } catch (CertificateAuthorityNotFoundException e) {
+            throw new ResourceNotFoundException(e);
+        }
     }
 
     @Override

diff --git a/.../application/src/main/java/org/niis/xroad/securityserver/restapi/service/AcmeService.java b/.../application/src/main/java/org/niis/xroad/securityserver/restapi/service/AcmeService.java
@@ -235,7 +235,7 @@ private void accountWithEabCredentials(AccountBuilder accountBuilder, KeyUsageIn
     private static Metadata getMetadata(Session session) {
         try {
             return session.getMetadata();
-        } catch (AcmeException e) {
+        } catch (Exception e) {
             throw new AcmeServiceException(FETCHING_METADATA_ERROR, e);
         }
     }

diff --git a/.../main/java/org/niis/xroad/securityserver/restapi/service/CertificateAuthorityService.java b/.../main/java/org/niis/xroad/securityserver/restapi/service/CertificateAuthorityService.java
@@ -54,11 +54,9 @@
 
 import java.security.cert.X509Certificate;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.Optional;
 import java.util.stream.Collectors;
 
 import static org.niis.xroad.restapi.exceptions.DeviationCodes.ERROR_CA_CERT_PROCESSING;
@@ -189,17 +187,7 @@ private ApprovedCaDto buildCertificateAuthorityDto(
         builder.name(approvedCAInfo.getName());
         builder.certificateProfileInfo(approvedCAInfo.getCertificateProfileInfo());
         builder.acmeServerIpAddress(approvedCAInfo.getAcmeServerIpAddress());
-        boolean acmeCapable = approvedCAInfo.getAcmeServerDirectoryUrl() != null;
-        builder.acmeCapable(acmeCapable);
-        if (acmeCapable) {
-            try {
-                builder.acmeEabRequired(
-                        acmeService.isExternalAccountBindingRequired(approvedCAInfo.getAcmeServerDirectoryUrl()));
-            } catch (AcmeServiceException e) {
-                log.warn("Acme Server for {} not reachable: {}", approvedCAInfo.getName(), e.getCause().getMessage());
-                builder.acmeEabRequired(false);
-            }
-        }
+        builder.acmeCapable(approvedCAInfo.getAcmeServerDirectoryUrl() != null);
 
         // properties from X509Certificate
         builder.notAfter(FormatUtils.fromDateToOffsetDateTime(certificate.getNotAfter()));
@@ -241,6 +229,11 @@ List<String> buildPath(X509Certificate certificate,
         return pathElements;
     }
 
+    public boolean isAcmeExternalAccountBindingRequired(String caName) throws CertificateAuthorityNotFoundException {
+        final var acmeUrl = getCertificateAuthorityInfo(caName).getAcmeServerDirectoryUrl();
+        return acmeUrl != null && acmeService.isExternalAccountBindingRequired(acmeUrl);
+    }
+
     public boolean hasAcmeExternalAccountBindingCredentials(String caName, String memberId) {
         return acmeProperties.hasEabCredentials(
                 caName,
@@ -262,7 +255,7 @@ public boolean hasAcmeExternalAccountBindingCredentials(String caName, String me
     public CertificateProfileInfo getCertificateProfile(String caName, KeyUsageInfo keyUsageInfo, ClientId memberId,
                                                         boolean isNewMember)
             throws CertificateAuthorityNotFoundException, CertificateProfileInstantiationException,
-            WrongKeyUsageException, ClientNotFoundException {
+                   WrongKeyUsageException, ClientNotFoundException {
         ApprovedCAInfo caInfo = getCertificateAuthorityInfo(caName);
         if (Boolean.TRUE.equals(caInfo.getAuthenticationOnly()) && KeyUsageInfo.SIGNING == keyUsageInfo) {
             throw new WrongKeyUsageException();
@@ -302,15 +295,11 @@ public CertificateProfileInfo getCertificateProfile(String caName, KeyUsageInfo
      * @throws CertificateAuthorityNotFoundException if matching CA was not found
      */
     public ApprovedCAInfo getCertificateAuthorityInfo(String caName) throws CertificateAuthorityNotFoundException {
-        Collection<ApprovedCAInfo> cas = globalConfService.getApprovedCAsForThisInstance();
-        Optional<ApprovedCAInfo> ca = cas.stream()
+        return globalConfService.getApprovedCAsForThisInstance().stream()
                 .filter(item -> caName.equals(item.getName()))
-                .findFirst();
-        if (ca.isEmpty()) {
-            throw new CertificateAuthorityNotFoundException("certificate authority "
-                    + caName + " not_found");
-        }
-        return ca.get();
+                .findFirst()
+                .orElseThrow(() -> new CertificateAuthorityNotFoundException("certificate authority "
+                        + caName + " not_found"));
     }
 
     /**

diff --git a/src/security-server/admin-service/ui/src/components/wizard/WizardPageCsrDetails.vue b/src/security-server/admin-service/ui/src/components/wizard/WizardPageCsrDetails.vue
@@ -87,7 +87,7 @@
     </div>
     <div class="button-footer">
       <xrd-button outlined data-test="cancel-button" @click="cancel"
-        >{{ $t('action.cancel') }}
+      >{{ $t('action.cancel') }}
       </xrd-button>
 
       <xrd-button
@@ -96,7 +96,7 @@
         class="previous-button"
         data-test="previous-button"
         @click="previous"
-        >{{ $t('action.previous') }}
+      >{{ $t('action.previous') }}
       </xrd-button>
       <xrd-button :disabled="!meta.valid" data-test="save-button" @click="done">
         {{ $t(saveButtonText) }}
@@ -116,6 +116,7 @@ import { useCsr } from '@/store/modules/certificateSignRequest';
 import { defineRule, PublicPathState, useForm } from 'vee-validate';
 import { FieldValidationMetaInfo } from '@vee-validate/i18n';
 import { i18n } from '@/plugins/i18n';
+import { useNotifications } from '@/store/modules/notifications';
 
 defineRule(
   'requiredIfSigning',
@@ -261,15 +262,15 @@ export default defineComponent({
   },
   methods: {
     ...mapActions(useCsr, ['fetchAllMemberIds', 'hasAcmeEabCredentials']),
+    ...mapActions(useNotifications, ['showError']),
     done(): void {
       this.usage = this.values.csr.usage;
       this.csrClient = this.values.csr.client;
       this.certificationService = this.values.csr.certificationService;
       this.csrFormat = this.values.csr.csrFormat;
       this.hasAcmeEabCredentials()
-        .finally(() => {
-          this.$emit('done');
-        });
+        .catch((error) => this.showError(error, true))
+        .finally(() => this.$emit('done'));
     },
     previous(): void {
       this.$emit('previous');

diff --git a/src/security-server/admin-service/ui/src/locales/en.json b/src/security-server/admin-service/ui/src/locales/en.json
@@ -278,6 +278,7 @@
     "csrDetails": "CSR details",
     "csrFormat": "CSR Format",
     "eabCredRequired": "Selected Certification Authority requires external account binding for ACME, but the credentials are missing from the configuration.",
+    "failedFetchAcmeMetadata": "Failed to fetch ACME metadata for selected Certification Authority, this may cause errors on the next steps.",
     "generateCsr": "Generate CSR",
     "helpCertificationService": "Certification Authority (CA) that will issue the certificate.",
     "helpClient": "X-Road member the certificate will be issued for.",
@@ -564,7 +565,8 @@
       "certService": "Certification Service",
       "client": "Client",
       "csrFormat": "CSR Format",
-      "usage": "Usage"
+      "usage": "Usage",
+      "certificationService": "Certification Service"
     },
     "dns": "DNS",
     "keys": {

diff --git a/src/security-server/admin-service/ui/src/store/modules/certificateSignRequest.ts b/src/security-server/admin-service/ui/src/store/modules/certificateSignRequest.ts
@@ -36,7 +36,7 @@ import {
   KeyUsageType,
   KeyWithCertificateSigningRequestId,
   TokenCertificateSigningRequest,
-  TokenType
+  TokenType,
 } from "@/openapi-types";
 import { defineStore } from "pinia";
 import * as api from "@/util/api";
@@ -57,6 +57,7 @@ export interface CsrState {
   memberIds: string[];
   isNewMember: boolean;
   acmeEabCredentialsStatus: AcmeEabCredentialsStatus | undefined;
+  failedFetchAcmeMetadata: boolean;
   acmeOrder: boolean;
 }
 
@@ -77,6 +78,7 @@ export const useCsr = defineStore('csr', {
       memberIds: [],
       isNewMember: false,
       acmeEabCredentialsStatus: undefined,
+      failedFetchAcmeMetadata: false,
       acmeOrder: false,
     };
   },
@@ -92,10 +94,7 @@ export const useCsr = defineStore('csr', {
       )?.acme_capable,
 
     eabRequired: (state) =>
-      state.certificationServiceList.find(
-        (certificateAuthority) =>
-          certificateAuthority.name == state.certificationService,
-      )?.acme_eab_required,
+      state.acmeEabCredentialsStatus?.acme_eab_required,
 
     csrRequestBody(state): CsrGenerate {
       // Creates an object that can be used as body for generate CSR request
@@ -278,7 +277,7 @@ export const useCsr = defineStore('csr', {
       if (!usage) {
         throw new Error('Key usage is missing');
       }
-      return api.post(`/certificate-authorities/${encodePathParameter(caName)}/acme-order`,{
+      return api.post(`/certificate-authorities/${encodePathParameter(caName)}/acme-order`, {
         csr_id: csr.id,
         key_usage_type: usage,
       })
@@ -288,6 +287,8 @@ export const useCsr = defineStore('csr', {
     },
 
     hasAcmeEabCredentials(caName?: string, csrClientId?: string, keyUsage?: KeyUsageType) {
+      this.failedFetchAcmeMetadata = false;
+
       return api
         .get<AcmeEabCredentialsStatus>(
           `/certificate-authorities/${encodePathParameter(
@@ -305,6 +306,7 @@ export const useCsr = defineStore('csr', {
           return res.data;
         })
         .catch((error) => {
+          this.failedFetchAcmeMetadata = error?.response?.data?.error?.code === 'acme.fetching_metadata_error';
           throw error;
         });
     },

diff --git a/src/security-server/admin-service/ui/src/store/modules/notifications.ts b/src/security-server/admin-service/ui/src/store/modules/notifications.ts
@@ -109,12 +109,13 @@ export const useNotifications = defineStore('notifications', {
     },
 
     // Show error notification with axios error object
-    showError(errorObject: unknown): void {
+    showError(errorObject: unknown, asWarning = false): void {
       // Show error using the x-road specific data in an axios error object
       // Don't show errors when the error code is 401 which is usually because of session expiring
       if (axios.isAxiosError(errorObject)) {
         if (errorObject?.response?.status !== 401) {
           const notification = createEmptyNotification(-1);
+          notification.isWarning = asWarning;
 
           // Add validation errors
 

diff --git a/src/security-server/admin-service/ui/src/views/AddClient/AddClient.vue b/src/security-server/admin-service/ui/src/views/AddClient/AddClient.vue
@@ -226,7 +226,7 @@ function csrDetailsReady(): void {
 
   fetchCsrForm()
     .then(() => hasAcmeEabCredentials())
-    .then(() => currentStep.value++)
+    .finally(() => currentStep.value++)
     .catch((error) => showError(error));
 }
 

diff --git a/...n-service/ui/src/views/KeysAndCertificates/SignAndAuthKeys/AcmeOrderCertificateDialog.vue b/...n-service/ui/src/views/KeysAndCertificates/SignAndAuthKeys/AcmeOrderCertificateDialog.vue
@@ -60,6 +60,7 @@ import { useField } from "vee-validate";
 import { mapActions, mapState } from "pinia";
 import { useCsr } from "@/store/modules/certificateSignRequest";
 import { KeyUsageType, TokenCertificateSigningRequest } from "@/openapi-types";
+import { useNotifications } from '@/store/modules/notifications';
 
 export default defineComponent({
   props: {
@@ -103,21 +104,23 @@ export default defineComponent({
   watch: {
     certificateService(newValue: string) {
       const newCA = this.acmeCertificateServices.find(ca => ca.name == newValue);
-      if (newCA?.acme_eab_required) {
+      if (newCA?.acme_capable) {
         this.hasAcmeEabCredentials(newCA.name, this.csr.owner_id, this.keyUsage)
           .then((res) => {
-            this.hasAcmeEabRequiredButNoCredentials = !res.has_acme_external_account_credentials;
+            this.hasAcmeEabRequiredButNoCredentials = res.acme_eab_required && !res.has_acme_external_account_credentials;
             if (this.hasAcmeEabRequiredButNoCredentials) {
               this.setErrors(this.$t('csr.eabCredRequired'));
             }
-          });
+          })
+          .catch((error) => this.showError(error, true));
       } else {
         this.hasAcmeEabRequiredButNoCredentials = false;
       }
     },
   },
   methods: {
     ...mapActions(useCsr, ['hasAcmeEabCredentials']),
+    ...mapActions(useNotifications, ['showError']),
     cancel(): void {
       this.$emit('cancel');
       this.clear();