SONAR-24023 Handle case where encryption is enabled but signing reque…

…st disabled
SonarSource · Dec 23, 2024 · ff8a50b · ff8a50b
1 parent a2500e7
commit ff8a50b
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 7 deletions.
diff --git a/...h-saml/src/main/java/org/sonar/auth/saml/SonarqubeRelyingPartyRegistrationRepository.java b/...h-saml/src/main/java/org/sonar/auth/saml/SonarqubeRelyingPartyRegistrationRepository.java
@@ -25,6 +25,7 @@
 import java.net.URL;
 import java.security.PrivateKey;
 import java.security.cert.X509Certificate;
+import java.util.Optional;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 import org.springframework.security.saml2.core.Saml2X509Credential;
@@ -68,7 +69,7 @@ public RelyingPartyRegistration findByRegistrationId(String registrationId) {
     return builder.build();
   }
 
-  private static String validateLoginUrl(String url){
+  private static String validateLoginUrl(String url) {
     try {
       return new URL(url).toURI().toString();
     } catch (MalformedURLException | URISyntaxException e) {
@@ -77,16 +78,32 @@ private static String validateLoginUrl(String url){
   }
 
   private void addSignRequestFieldsIfNecessary(RelyingPartyRegistration.Builder builder) {
-    if (!samlSettings.isSignRequestsEnabled()) {
+    //(on SQ) to sign request we need SP private key and certificate
+    //(on IDP) to verify request IDP needs SP public key (certificate)
+
+    //(on IDP) to sign response we need IDP private key (embedded)
+    //(on SQ) to verify response we need IDP public key (certificate) !mandatory!
+
+    //(on IDP) encryption: we need SP public key (certificate)
+    //(on SQ) decryption: we need Service Provide private key and certificate
+    Optional<String> serviceProviderPrivateKey = samlSettings.getServiceProviderPrivateKey();
+
+    if (serviceProviderPrivateKey.isEmpty() || samlSettings.getServiceProviderCertificate() == null) {
+      if (samlSettings.isSignRequestsEnabled()) {
+        throw new IllegalStateException("Sign requests is enabled but SonarQube private key and/or SonarQube certificate is missing");
+      }
       return;
     }
-    String privateKeyString = samlSettings.getServiceProviderPrivateKey().orElseThrow(() -> new IllegalStateException("Sign requests is enabled but private key is missing"));
+
+    String privateKeyString = serviceProviderPrivateKey.get();
     String serviceProviderCertificateString = samlSettings.getServiceProviderCertificate();
     PrivateKey privateKey = samlPrivateKeyConverter.toPrivateKey(privateKeyString);
     X509Certificate spX509Certificate = samlCertificateConverter.toX509Certificate(serviceProviderCertificateString);
-    builder
-      .signingX509Credentials(c -> c.add(Saml2X509Credential.signing(privateKey, spX509Certificate)))
-      .decryptionX509Credentials(c -> c.add(Saml2X509Credential.decryption(privateKey, spX509Certificate)));
+    builder.decryptionX509Credentials(c -> c.add(Saml2X509Credential.decryption(privateKey, spX509Certificate)));
+
+    if (samlSettings.isSignRequestsEnabled()) {
+      builder.signingX509Credentials(c -> c.add(Saml2X509Credential.signing(privateKey, spX509Certificate)));
+    }
   }
 
   @VisibleForTesting

diff --git a/...ml/src/test/java/org/sonar/auth/saml/SonarqubeRelyingPartyRegistrationRepositoryTest.java b/...ml/src/test/java/org/sonar/auth/saml/SonarqubeRelyingPartyRegistrationRepositoryTest.java
@@ -75,6 +75,40 @@ void findByRegistrationId_whenSignRequestsIsDisabledAndAllFieldSet_succeeds() {
     verifyNoInteractions(samlPrivateKeyConverter);
   }
 
+  @Test
+  void findByRegistrationId_whenSignRequestIsDisabledAndPrivateKeyEmptyButSPCertificatePresent_doesNotThrow() {
+    when(samlSettings.getApplicationId()).thenReturn(APPLICATION_ID);
+    when(samlSettings.getProviderId()).thenReturn(PROVIDER_ID);
+    when(samlSettings.getLoginUrl()).thenReturn(SSO_URL);
+    when(samlSettings.getCertificate()).thenReturn(CERTIF_STRING);
+    when(samlSettings.getServiceProviderPrivateKey()).thenReturn(Optional.empty());
+
+    X509Certificate mockedCertificate = mockCertificate();
+    RelyingPartyRegistration registration = findRegistration(CALLBACK_URL);
+
+    assertCommonFields(registration, mockedCertificate, CALLBACK_URL);
+    assertThat(registration.getAssertingPartyMetadata().getWantAuthnRequestsSigned()).isFalse();
+    assertThat(registration.getSigningX509Credentials()).isEmpty();
+    assertThat(registration.getDecryptionX509Credentials()).isEmpty();
+  }
+
+  @Test
+  void findByRegistrationId_whenSignRequestIsDisabledAndPrivateKeyPresentButSPCertificateNull_doesNotThrow() {
+    when(samlSettings.getApplicationId()).thenReturn(APPLICATION_ID);
+    when(samlSettings.getProviderId()).thenReturn(PROVIDER_ID);
+    when(samlSettings.getLoginUrl()).thenReturn(SSO_URL);
+    when(samlSettings.getCertificate()).thenReturn(CERTIF_STRING);
+    when(samlSettings.getServiceProviderPrivateKey()).thenReturn(Optional.of(PRIVATE_KEY));
+
+    X509Certificate mockedCertificate = mockCertificate();
+    RelyingPartyRegistration registration = findRegistration(CALLBACK_URL);
+
+    assertCommonFields(registration, mockedCertificate, CALLBACK_URL);
+    assertThat(registration.getAssertingPartyMetadata().getWantAuthnRequestsSigned()).isFalse();
+    assertThat(registration.getSigningX509Credentials()).isEmpty();
+    assertThat(registration.getDecryptionX509Credentials()).isEmpty();
+  }
+
   @Test
   void findByRegistrationId_whenCallbackUrlIsNull_succeeds() {
     when(samlSettings.getApplicationId()).thenReturn(APPLICATION_ID);
@@ -124,7 +158,7 @@ void findByRegistrationId_whenSignRequestIsEnabledAndPrivateKeyEmpty_throws() {
 
     assertThatIllegalStateException()
       .isThrownBy(() -> findRegistration(CALLBACK_URL))
-      .withMessage("Sign requests is enabled but private key is missing");
+      .withMessage("Sign requests is enabled but SonarQube private key and/or SonarQube certificate is missing");
   }
 
   @Test