diff --git a/src/main/java/org/italiangrid/voms/ac/VOMSValidationResult.java b/src/main/java/org/italiangrid/voms/ac/VOMSValidationResult.java index 5c32e51..1048b70 100644 --- a/src/main/java/org/italiangrid/voms/ac/VOMSValidationResult.java +++ b/src/main/java/org/italiangrid/voms/ac/VOMSValidationResult.java @@ -92,4 +92,14 @@ public String toString() { + attributes + "]"; } + + public static VOMSValidationResult success( + VOMSAttribute attributes, List validationErrors) { + return new VOMSValidationResult(attributes, true, validationErrors); + } + + public static VOMSValidationResult failure( + VOMSAttribute attributes, List validationErrors) { + return new VOMSValidationResult(attributes, false, validationErrors); + } } diff --git a/src/main/java/org/italiangrid/voms/ac/impl/DefaultVOMSValidationStrategy.java b/src/main/java/org/italiangrid/voms/ac/impl/DefaultVOMSValidationStrategy.java index df18b4a..74f094c 100644 --- a/src/main/java/org/italiangrid/voms/ac/impl/DefaultVOMSValidationStrategy.java +++ b/src/main/java/org/italiangrid/voms/ac/impl/DefaultVOMSValidationStrategy.java @@ -11,6 +11,7 @@ import static org.italiangrid.voms.error.VOMSValidationErrorCode.acNotValidAtCurrentTime; import static org.italiangrid.voms.error.VOMSValidationErrorCode.canlError; import static org.italiangrid.voms.error.VOMSValidationErrorCode.emptyAcCertsExtension; +import static org.italiangrid.voms.error.VOMSValidationErrorCode.fqanDoesntMatchVo; import static org.italiangrid.voms.error.VOMSValidationErrorCode.invalidAaCert; import static org.italiangrid.voms.error.VOMSValidationErrorCode.invalidAcCert; import static org.italiangrid.voms.error.VOMSValidationErrorCode.localhostDoesntMatchAcTarget; @@ -81,19 +82,16 @@ private boolean checkACHolder( X500Principal chainHolder = ProxyUtils.getOriginalUserDN(chain); - boolean holderDoesMatch = chainHolder.equals(attributes.getHolder()); - - if (!holderDoesMatch) { - - String acHolderSubject = X500NameUtils.getReadableForm(attributes.getHolder()); - String certChainSubject = X500NameUtils.getReadableForm(chainHolder); - - validationErrors.add( - VOMSValidationErrorMessage.newErrorMessage( - acHolderDoesntMatchCertChain, acHolderSubject, certChainSubject)); + if (chainHolder.equals(attributes.getHolder())) { + return true; } - return holderDoesMatch; + String acHolderSubject = X500NameUtils.getReadableForm(attributes.getHolder()); + String certChainSubject = X500NameUtils.getReadableForm(chainHolder); + validationErrors.add( + VOMSValidationErrorMessage.newErrorMessage( + acHolderDoesntMatchCertChain, acHolderSubject, certChainSubject)); + return false; } private boolean checkACValidity( @@ -101,17 +99,16 @@ private boolean checkACValidity( Date now = new Date(); - boolean valid = attributes.validAt(now); - - if (!valid) { - VOMSValidationErrorMessage m = - VOMSValidationErrorMessage.newErrorMessage( - acNotValidAtCurrentTime, attributes.getNotBefore(), attributes.getNotAfter(), now); - - validationErrors.add(m); + if (attributes.validAt(now)) { + return true; } - return valid; + VOMSValidationErrorMessage m = + VOMSValidationErrorMessage.newErrorMessage( + acNotValidAtCurrentTime, attributes.getNotBefore(), attributes.getNotAfter(), now); + + validationErrors.add(m); + return false; } private boolean checkLocalAACertSignature( @@ -132,16 +129,15 @@ private boolean checkLocalAACertSignature( return false; } - boolean signatureValid = verifyACSignature(attributes, localAACert); - - if (!signatureValid) { - String readableSubject = X500NameUtils.getReadableForm(localAACert.getSubjectX500Principal()); - validationErrors.add( - VOMSValidationErrorMessage.newErrorMessage( - aaCertFailsSignatureVerification, readableSubject)); + if (verifyACSignature(attributes, localAACert)) { + return true; } - return signatureValid; + String readableSubject = X500NameUtils.getReadableForm(localAACert.getSubjectX500Principal()); + validationErrors.add( + VOMSValidationErrorMessage.newErrorMessage( + aaCertFailsSignatureVerification, readableSubject)); + return false; } private boolean checkLSCSignature( @@ -191,11 +187,8 @@ private boolean checkLSCSignature( private boolean checkSignature( VOMSAttribute attributes, List validationErrors) { - boolean valid = checkLSCSignature(attributes, validationErrors); - - if (!valid) valid = checkLocalAACertSignature(attributes, validationErrors); - - return valid; + return checkLSCSignature(attributes, validationErrors) + || checkLocalAACertSignature(attributes, validationErrors); } private boolean checkTargets( @@ -305,58 +298,37 @@ private boolean checkUnhandledCriticalExtensions( public VOMSValidationResult validateAC(VOMSAttribute attributes) { - boolean valid = true; List validationErrors = new ArrayList(); - // Check temporal validity - valid = checkACValidity(attributes, validationErrors); + if (checkACValidity(attributes, validationErrors) + && checkSignature(attributes, validationErrors) + && checkTargets(attributes, validationErrors) + && checkAuthorityKeyIdentifierExtension(attributes, validationErrors) + && checkNoRevAvailExtension(attributes, validationErrors) + && checkUnhandledCriticalExtensions(attributes, validationErrors) + && checkFQANs(attributes, validationErrors)) { - if (valid) - // Verify signature on AC checking LSC file or local AA certificate - valid = checkSignature(attributes, validationErrors); - - if (valid) - // Check targets - valid = checkTargets(attributes, validationErrors); - - // AC extension checking to be compliant with rfc 3281 - if (valid) valid = checkAuthorityKeyIdentifierExtension(attributes, validationErrors); - - if (valid) valid = checkNoRevAvailExtension(attributes, validationErrors); - - if (valid) valid = checkUnhandledCriticalExtensions(attributes, validationErrors); - - return new VOMSValidationResult(attributes, valid, validationErrors); + return VOMSValidationResult.success(attributes, validationErrors); + } + return VOMSValidationResult.failure(attributes, validationErrors); } public VOMSValidationResult validateAC(VOMSAttribute attributes, X509Certificate[] chain) { - boolean valid = true; List validationErrors = new ArrayList(); - // Check temporal validity - valid = checkACValidity(attributes, validationErrors); + if (checkACValidity(attributes, validationErrors) + && checkSignature(attributes, validationErrors) + && checkACHolder(attributes, chain, validationErrors) + && checkTargets(attributes, validationErrors) + && checkAuthorityKeyIdentifierExtension(attributes, validationErrors) + && checkNoRevAvailExtension(attributes, validationErrors) + && checkUnhandledCriticalExtensions(attributes, validationErrors) + && checkFQANs(attributes, validationErrors)) { - if (valid) - // Verify signature on AC checking LSC file or local AA certificate - valid = checkSignature(attributes, validationErrors); - - if (valid) - // Check AC holder - valid = checkACHolder(attributes, chain, validationErrors); - - if (valid) - // Check targets - valid = checkTargets(attributes, validationErrors); - - // AC extension checking to be compliant with rfc 3281 - if (valid) valid = checkAuthorityKeyIdentifierExtension(attributes, validationErrors); - - if (valid) valid = checkNoRevAvailExtension(attributes, validationErrors); - - if (valid) valid = checkUnhandledCriticalExtensions(attributes, validationErrors); - - return new VOMSValidationResult(attributes, valid, validationErrors); + return VOMSValidationResult.success(attributes, validationErrors); + } + return VOMSValidationResult.failure(attributes, validationErrors); } private boolean validateCertificate( @@ -390,4 +362,38 @@ private boolean verifyACSignature(VOMSAttribute attributes, X509Certificate cert throw new VOMSError("Error verifying AC signature: " + e.getMessage(), e); } } + + /** + * Checks that every FQAN has the AC's VO name as its root group. + * + * @return false if any FQAN belongs to a different VO + */ + private boolean checkFQANs( + VOMSAttribute attributes, List validationErrors) { + + String vo = attributes.getVO(); + + if (vo == null || vo.isEmpty()) { + validationErrors.add(newErrorMessage(other, "Missing VO name in AC")); + return false; + } + + List fqans = attributes.getFQANs(); + + if (fqans == null) { + validationErrors.add(newErrorMessage(other, "Missing FQAN list in AC")); + return false; + } + + String root = "/" + vo; + + for (String fqan : fqans) { + if (fqan == null || !(fqan.equals(root) || fqan.startsWith(root + "/"))) { + validationErrors.add(newErrorMessage(fqanDoesntMatchVo, fqan, vo)); + return false; + } + } + + return true; + } } diff --git a/src/main/java/org/italiangrid/voms/error/VOMSValidationErrorCode.java b/src/main/java/org/italiangrid/voms/error/VOMSValidationErrorCode.java index 415d765..0df6a8b 100644 --- a/src/main/java/org/italiangrid/voms/error/VOMSValidationErrorCode.java +++ b/src/main/java/org/italiangrid/voms/error/VOMSValidationErrorCode.java @@ -35,6 +35,9 @@ public enum VOMSValidationErrorCode { // CAnL errors canlError, + // FQAN checks + fqanDoesntMatchVo, + // Other other; } diff --git a/src/main/resources/org/italiangrid/voms/error/validationErrors.properties b/src/main/resources/org/italiangrid/voms/error/validationErrors.properties index c78f21d..de5d2fb 100644 --- a/src/main/resources/org/italiangrid/voms/error/validationErrors.properties +++ b/src/main/resources/org/italiangrid/voms/error/validationErrors.properties @@ -35,4 +35,5 @@ acHolderDoesntMatchCertChain = AC holder check failed: AC holder {0} does not ma ## Targets check localhostDoesntMatchAcTarget = AC target check failed: local host {0} is not in the AC target list {1}. - +## FQAN +fqanDoesntMatchVo=FQAN "{0}" does not belong to VO "{1}". diff --git a/src/test/java/org/italiangrid/voms/test/ac/TestFQANVOValidation.java b/src/test/java/org/italiangrid/voms/test/ac/TestFQANVOValidation.java new file mode 100644 index 0000000..a71aaf8 --- /dev/null +++ b/src/test/java/org/italiangrid/voms/test/ac/TestFQANVOValidation.java @@ -0,0 +1,192 @@ +// SPDX-FileCopyrightText: 2026 Istituto Nazionale di Fisica Nucleare +// +// SPDX-License-Identifier: Apache-2.0 + +package org.italiangrid.voms.test.ac; + +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import eu.emi.security.authn.x509.ValidationResult; +import eu.emi.security.authn.x509.X509CertChainValidatorExt; +import eu.emi.security.authn.x509.impl.PEMCredential; +import eu.emi.security.authn.x509.proxy.ProxyCertificate; +import java.security.cert.X509Certificate; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import org.italiangrid.voms.VOMSAttribute; +import org.italiangrid.voms.ac.VOMSValidationResult; +import org.italiangrid.voms.ac.impl.DefaultVOMSACParser; +import org.italiangrid.voms.ac.impl.DefaultVOMSValidationStrategy; +import org.italiangrid.voms.ac.impl.DefaultVOMSValidator; +import org.italiangrid.voms.error.VOMSValidationErrorMessage; +import org.italiangrid.voms.store.impl.DefaultVOMSTrustStore; +import org.italiangrid.voms.test.utils.Fixture; +import org.italiangrid.voms.test.utils.Utils; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Regression tests for issue #42: every FQAN root group must equal the AC's VO name. + * + *

ACs are generated, signed and parsed using the existing fixtures. Only PKIX certificate chain + * validation is stubbed to avoid dependence on fixture certificate and CRL expiry. AC signature + * verification, LSC matching and holder validation use the real implementation. + */ +public class TestFQANVOValidation implements Fixture { + + private PEMCredential holder; + private DefaultVOMSValidationStrategy strategy; + + @BeforeEach + public void setUp() throws Exception { + holder = Utils.getTestUserCredential(); + X509CertChainValidatorExt certValidator = mock(X509CertChainValidatorExt.class); + when(certValidator.validate(any(X509Certificate[].class))) + .thenReturn(new ValidationResult(true)); + strategy = + new DefaultVOMSValidationStrategy( + new DefaultVOMSTrustStore(Collections.singletonList(vomsdir)), certValidator); + } + + /** Accepts a root-only FQAN equal to the declared VO in both validation overloads. */ + @Test + public void acceptsMatchingRoot() throws Exception { + assertAccepted("/test.vo"); + } + + /** Accepts subgroups and role/capability components while preserving FQAN order. */ + @Test + public void acceptsMatchingGroupsAndRoles() throws Exception { + assertAccepted( + "/test.vo/analysis/Role=production/Capability=NULL", + "/test.vo", + "/test.vo/analysis", + "/test.vo/analysis/subgroup/Role=NULL/Capability=NULL"); + } + + /** Rejects a correctly signed AC containing a FQAN from another VO. */ + @Test + public void rejectsDifferentVo() throws Exception { + assertRejected("/other.vo/Role=production"); + } + + /** Requires a group boundary after the VO name, not merely a matching string prefix. */ + @Test + public void rejectsVoNamePrefixCollision() throws Exception { + assertRejected("/test.vo-other/Role=production"); + } + + /** Does not treat a longer dotted VO name as a subgroup of the declared VO. */ + @Test + public void rejectsLongerDottedVoName() throws Exception { + assertRejected("/test.vo.other"); + } + + /** Compares VO names exactly, without case folding. */ + @Test + public void rejectsDifferentCase() throws Exception { + assertRejected("/TEST.VO/Role=production"); + } + + /** Rejects a FQAN without the leading group separator. */ + @Test + public void rejectsMissingLeadingSlash() throws Exception { + assertRejected("test.vo/Role=production"); + } + + /** Checks secondary FQANs even when the primary FQAN belongs to the declared VO. */ + @Test + public void rejectsForeignSecondaryFqan() throws Exception { + assertRejected("/other.vo/Role=production", "/test.vo", "/other.vo/Role=production"); + } + + /** Rejects a foreign primary FQAN instead of promoting a later valid FQAN to primary. */ + @Test + public void rejectsForeignPrimaryFqan() throws Exception { + assertRejected("/other.vo", "/other.vo", "/test.vo"); + } + + /** Ensures the public validator does not expose a rejected AC as validated attributes. */ + @Test + public void publicValidatorExcludesMixedVoAc() throws Exception { + ProxyCertificate proxy = createProxy(Arrays.asList("/test.vo", "/other.vo")); + DefaultVOMSValidator validator = + new DefaultVOMSValidator.Builder().validationStrategy(strategy).build(); + + List results = validator.validateWithResult(proxy.getCertificateChain()); + assertEquals(1, results.size()); + assertMismatch(results.get(0), "/other.vo"); + assertTrue(validator.validate(proxy.getCertificateChain()).isEmpty()); + } + + private ProxyCertificate createProxy(List fqans) throws Exception { + return Utils.getVOMSAA().createVOMSProxy(holder, fqans); + } + + private VOMSAttribute parse(ProxyCertificate proxy, List fqans) { + List attributes = new DefaultVOMSACParser().parse(proxy.getCertificateChain()); + assertEquals(1, attributes.size()); + VOMSAttribute attribute = attributes.get(0); + assertEquals(defaultVO, attribute.getVO()); + assertEquals(fqans, attribute.getFQANs()); + return attribute; + } + + private void assertAccepted(String... fqans) throws Exception { + List expected = Arrays.asList(fqans); + ProxyCertificate proxy = createProxy(expected); + VOMSAttribute attribute = parse(proxy, expected); + + assertAll( + "Both validation overloads accept matching FQANs", + () -> assertValid(strategy.validateAC(attribute)), + () -> assertValid(strategy.validateAC(attribute, proxy.getCertificateChain()))); + assertEquals(expected, attribute.getFQANs()); + assertEquals(expected.get(0), attribute.getPrimaryFQAN()); + } + + private void assertRejected(String fqan) throws Exception { + assertRejected(fqan, new String[] {fqan}); + } + + private void assertRejected(String offendingFqan, String... fqans) throws Exception { + List expected = Arrays.asList(fqans); + ProxyCertificate proxy = createProxy(expected); + VOMSAttribute attribute = parse(proxy, expected); + + assertAll( + "Both validation overloads reject inconsistent FQANs", + () -> assertMismatch(strategy.validateAC(attribute), offendingFqan), + () -> + assertMismatch( + strategy.validateAC(attribute, proxy.getCertificateChain()), offendingFqan)); + // Validation must reject the AC without filtering or reordering its signed attributes. + assertEquals(expected, attribute.getFQANs()); + assertEquals(expected.get(0), attribute.getPrimaryFQAN()); + } + + private void assertValid(VOMSValidationResult result) { + assertTrue(result.isValid(), () -> result.getValidationErrors().toString()); + assertTrue(result.getValidationErrors().isEmpty()); + } + + private void assertMismatch(VOMSValidationResult result, String offendingFqan) { + assertFalse(result.isValid(), "An AC with a foreign FQAN must be rejected"); + assertEquals(1, result.getValidationErrors().size()); + VOMSValidationErrorMessage error = result.getValidationErrors().get(0); + // Compare names so this regression suite also compiles before the new enum value is added. + assertEquals("fqanDoesntMatchVo", error.getErrorCode().name()); + assertEquals(offendingFqan, error.getParameters()[0]); + assertEquals(defaultVO, error.getParameters()[1]); + assertEquals( + "FQAN \"" + offendingFqan + "\" does not belong to VO \"" + defaultVO + "\".", + error.getMessage()); + } +}