Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect paths in mutable packages which are not writable by the service #14

Merged
merged 3 commits into from
Mar 19, 2021
Merged

Detect paths in mutable packages which are not writable by the service #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da7e9b3
Detect paths in mutable packages which are not writable by the service
kwin Mar 19, 2021
df2c295
improve readme
kwin Mar 19, 2021
f7d3b57
improve readme part2
kwin Mar 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,30 @@ The following options are supported apart from the default settings mentioned in

Option | Mandatory | Description | Default Value | Since Version
--- | --- | --- | --- | ---
`allowVarNodeOutsideContainer` | no | `true` means `/var` nodes should be allowed in content packages which do not contain other packages (i.e. are no containers). Otherwise `var` nodes are not even allowed in standalone packages. | `true` | 1.0.0
`allowReadOnlyMutablePathsOutsideContainers` (or `allowVarNodeOutsideContainer` deprecated) | no | `true` means read-only paths (i.e. paths to which the service session used for mutable package installation on publish does not have write permission) should be allowed. Otherwise those won't only be allowed in author-only container packages. | `true` | 1.2.0 (1.0.0 for deprecated `allowVarNodeOutsideContainer`)
`allowLibsNode` | no | `true` means that `libs` nodes are allowed in content packages. *Only set this to `true` when building packages which are part of the AEM product.* | `false` | 1.2.0

# Included Checks

## Prevent using `/var` in content package
## Prevent using certain paths in mutable content packages

Including `/var` in content packages being deployed to publish instances must be prevented, as it causes deployment failures. The system user which takes care of installing the packages on publish (named `sling-distribution-importer`) does not have `write` permission in `/var`. Further details at <https://experienceleague.adobe.com/docs/experience-manager-learn/cloud-service/debugging/debugging-aem-as-a-cloud-service/build-and-deployment.html?lang=en#including-%2Fvar-in-content-package>.
Including `/var` and `tmp` and some others in content packages being deployed to publish instances must be prevented, as it causes deployment failures. The [system session](https://sling.apache.org/documentation/the-sling-engine/service-authentication.html#slingrepository) which takes care of installing the packages on publish does not have `jcr:write` permission to those locations. Further details at <https://experienceleague.adobe.com/docs/experience-manager-learn/cloud-service/debugging/debugging-aem-as-a-cloud-service/build-and-deployment.html?lang=en#including-%2Fvar-in-content-package>.

As this restriction technically only affects publish instances it is still valid to have those nodes in [author-only containers](https://experienceleague.adobe.com/docs/experience-manager-cloud-service/implementing/developing/aem-project-content-package-structure.html#embeddeds).
As a *temporary workaround* you can also [extend the privileges of the `sling-distribution-importer` user via a custom repoinit configuration](https://helpx.adobe.com/in/experience-manager/kb/cm/cloudmanager-deploy-fails-due-to-sling-distribution-aem.html). Here is the full list of default permissions of the system session extracted from AEM 2021.2.4887.20210204T154817Z.
All the following principals are mapped via the service user mapping for `org.apache.sling.distribution.journal:importer` on publish

Principal | Permissions
--- | ---
`sling-distribution-importer` | allow `jcr:modifyAccessControl,jcr:readAccessControl` on `/content`<br/>allow `jcr:modifyAccessControl,jcr:readAccessControl` on `/conf`<br/>allow `jcr:modifyAccessControl,jcr:readAccessControl` on `/etc`<br/>allow `jcr:nodeTypeDefinitionManagement,rep:privilegeManagement` on `:repository`
`sling-distribution` | allow `jcr:read,rep:write` on `/var/sling/distribution`
`content-writer-service` | allow `jcr:read,rep:write,jcr:versionManagement` on `/content`
`repository-reader-service` | allow `jcr:read` on `/`
`version-manager-service` | allow `jcr:read,rep:write,jcr:versionManagement` on `/conf`</br/>allow `jcr:read,rep:write,jcr:versionManagement` on `/etc`
`group-administration-service` | allow `jcr:all` on `/home/groups`
`user-administration-service` | allow `jcr:all` on `/home/users`
`namespace-mgmt-service` | allow `jcr:namespaceManagement` on `:repository`

As this restriction technically only affects publish instances it is still valid to have `/var` nodes in author-only containers.
As a *temporary workaround* you can also [extend the privileges of the `sling-distribution-importer` via a custom repoinit configuration](https://helpx.adobe.com/in/experience-manager/kb/cm/cloudmanager-deploy-fails-due-to-sling-distribution-aem.html).

## Prevent using `/libs` in content package

Expand Down
58 changes: 38 additions & 20 deletions src/main/java/biz/netcentric/filevault/validator/aem/cloud/AemCloudValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@

import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
import java.util.regex.Pattern;

import org.apache.jackrabbit.vault.packaging.PackageType;
Expand All @@ -38,9 +36,9 @@

public class AemCloudValidator implements NodePathValidator, MetaInfPathValidator, DocumentViewXmlValidator {

static final String VIOLATION_MESSAGE_VAR_NODES_CONDITION_CONTAINER = "only allowed in author-specific packages";
static final String VIOLATION_MESSAGE_VAR_NODES_CONDITION_OVERALL = "not allowed";
static final String VIOLATION_MESSAGE_VAR_NODES = "Using nodes below '/var' is %s. Consider to use repoinit scripts instead or move that content to another location. Further details at https://experienceleague.adobe.com/docs/experience-manager-learn/cloud-service/debugging/debugging-aem-as-a-cloud-service/build-and-deployment.html?lang=en#including-%%2Fvar-in-content-package";
static final String VIOLATION_MESSAGE_CONDITION_AUTHOR_ONLY_CONTAINER = "only allowed in author-specific packages";
static final String VIOLATION_MESSAGE_CONDITION_OVERALL = "not allowed";
static final String VIOLATION_MESSAGE_READONLY_MUTABLE_PATH = "Using mutable nodes in this repository location is %s as it is not writable by the underlying service user on publish. Consider to use repoinit scripts instead or move that content to another location. Further details at https://experienceleague.adobe.com/docs/experience-manager-learn/cloud-service/debugging/debugging-aem-as-a-cloud-service/build-and-deployment.html?lang=en#including-%%2Fvar-in-content-package";
static final String VIOLATION_MESSAGE_INSTALL_HOOK_IN_MUTABLE_PACKAGE = "Using install hooks in mutable content packages leads to deployment failures as the underlying service user on the publish does not have the right to execute those.";
static final String VIOLATION_MESSAGE_INVALID_INDEX_DEFINITION_NODE_NAME = "All Oak index definition node names must end with '-custom-<integer>' but found name '%s'. Further details at https://experienceleague.adobe.com/docs/experience-manager-cloud-service/operations/indexing.html?lang=en#how-to-use";
static final String VIOLATION_MESSAGE_LIBS_NODES = "Nodes below '/libs' may be overwritten by future product upgrades. Rather use '/apps'. Further details at https://experienceleague.adobe.com/docs/experience-manager-cloud-service/implementing/developing/full-stack/overlays.html?lang=en#developing";
Expand All @@ -51,13 +49,20 @@ public class AemCloudValidator implements NodePathValidator, MetaInfPathValidato
// this path is relative to META-INF
private static final Path INSTALL_HOOK_PATH = Paths.get(Constants.VAULT_DIR, Constants.HOOKS_DIR);
private static final Pattern INDEX_DEFINITION_NAME_PATTERN = Pattern.compile(".*-custom-\\d++");
private static final Set<String> IMMUTABLE_PATH_PREFIXES = new HashSet<>(Arrays.asList("/apps", "/libs", "/oak:index"));
private static final Collection<String> IMMUTABLE_PATH_PREFIXES = Arrays.asList("/apps", "/libs", "/oak:index");
private static final Collection<String> WRITABLE_PATHS_BY_DISTRIBUTION_IMPORTER = Arrays.asList(
"/content", // access provided by system user content-writer-service and sling-distribution-importer
"/etc", // access provided by system user version-manager-service and sling-distribution-importer
"/conf", // access provided by system user version-manager-service and sling-distribution-importer
"/home/users", // access provided by system user user-administration-service
"/home/groups" // access provided by system user group-administration-service
);

private final @NotNull ValidationMessageSeverity defaultSeverity;
private final ValidationContext containerValidationContext;
private final PackageType packageType;
private boolean hasMutableNodes;
private final boolean allowVarNodesOutsideContainers;
private final boolean allowReadOnlyMutablePaths;
private final boolean allowLibsNode;
private boolean hasInstallHooks;
private boolean hasImmutableNodes;
Expand All @@ -67,10 +72,10 @@ public class AemCloudValidator implements NodePathValidator, MetaInfPathValidato
private int numLibNodeViolations = 0;
private int numMutableNodeViolations = 0;

public AemCloudValidator(boolean allowVarNodesOutsideContainers, boolean allowLibsNode, @Nullable PackageType packageType,
public AemCloudValidator(boolean allowReadOnlyMutablePaths, boolean allowLibsNode, @Nullable PackageType packageType,
@Nullable ValidationContext containerValidationContext, @NotNull ValidationMessageSeverity defaultSeverity) {
super();
this.allowVarNodesOutsideContainers = allowVarNodesOutsideContainers;
this.allowReadOnlyMutablePaths = allowReadOnlyMutablePaths;
this.allowLibsNode = allowLibsNode;
this.packageType = packageType;
this.containerValidationContext = containerValidationContext;
Expand All @@ -83,20 +88,20 @@ public AemCloudValidator(boolean allowVarNodesOutsideContainers, boolean allowLi
@Override
public Collection<ValidationMessage> validate(@NotNull String path) {
Collection<ValidationMessage> messages = new ArrayList<>();
if (numVarNodeViolations < MAX_NUM_VIOLATIONS_PER_TYPE && path.startsWith("/var/")) {
// check if package itself is only used on author
if (!allowVarNodesOutsideContainers || !isContainedInAuthorOnlyPackage(containerValidationContext)) {
// only emit once per package
messages.add(new ValidationMessage(defaultSeverity, String.format(
VIOLATION_MESSAGE_VAR_NODES, allowVarNodesOutsideContainers ? VIOLATION_MESSAGE_VAR_NODES_CONDITION_CONTAINER
: VIOLATION_MESSAGE_VAR_NODES_CONDITION_OVERALL)));
numVarNodeViolations++;
}
}
// skip root node for mutable/immutable path classification
if (!"/".equals(path)) {
if (isMutablePath(path)) {
hasMutableNodes = true;
if (numVarNodeViolations < MAX_NUM_VIOLATIONS_PER_TYPE && !isPathWritableByDistributionJournalImporter(path)) {
// check if package itself is only used on author
if (!allowReadOnlyMutablePaths || !isContainedInAuthorOnlyPackage(containerValidationContext)) {
// only emit once per package
messages.add(new ValidationMessage(defaultSeverity, String.format(
VIOLATION_MESSAGE_READONLY_MUTABLE_PATH, allowReadOnlyMutablePaths ? VIOLATION_MESSAGE_CONDITION_AUTHOR_ONLY_CONTAINER
: VIOLATION_MESSAGE_CONDITION_OVERALL)));
numVarNodeViolations++;
}
}
if (numMutableNodeViolations < MAX_NUM_VIOLATIONS_PER_TYPE && PackageType.MIXED.equals(packageType)) {
messages.add(new ValidationMessage(defaultSeverity, VIOLATION_MESSAGE_MUTABLE_NODES_IN_MIXED_PACKAGE));
numMutableNodeViolations++;
Expand All @@ -112,7 +117,7 @@ public Collection<ValidationMessage> validate(@NotNull String path) {
return messages;
}

private boolean isMutablePath(String path) {
static boolean isMutablePath(String path) {
for (String immutablePathPrefix : IMMUTABLE_PATH_PREFIXES) {
if (path.startsWith(immutablePathPrefix+"/") || path.equals(immutablePathPrefix)) {
return false;
Expand All @@ -121,6 +126,19 @@ private boolean isMutablePath(String path) {
return true;
}

/**
*
* @return {@code true} in case the given mutable path is writable by the service user used by the Content Distribution Journal Importer used on AEMaaCS publish
*/
static boolean isPathWritableByDistributionJournalImporter(String path) {
for (String writablePath : WRITABLE_PATHS_BY_DISTRIBUTION_IMPORTER) {
if (path.startsWith(writablePath + "/") || path.equals(writablePath)) {
return true;
}
}
return false;
}

/** @param containerValidationContext the container validation context of the package to be validated.
* @return {@code true} in case this package has been included in a "author" run mode specific folder, otherwise {@code false} */
private boolean isContainedInAuthorOnlyPackage(ValidationContext containerValidationContext) {
Expand Down
18 changes: 14 additions & 4 deletions src/main/java/biz/netcentric/filevault/validator/aem/cloud/AemCloudValidatorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,27 +17,37 @@
import org.apache.jackrabbit.vault.validation.spi.Validator;
import org.apache.jackrabbit.vault.validation.spi.ValidatorFactory;
import org.apache.jackrabbit.vault.validation.spi.ValidatorSettings;
import org.apache.jackrabbit.vault.validation.spi.impl.AdvancedFilterValidatorFactory;
import org.jetbrains.annotations.NotNull;
import org.kohsuke.MetaInfServices;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;


@MetaInfServices
public class AemCloudValidatorFactory implements ValidatorFactory {

private static final String OPTION_ALLOW_VAR_NODE_OUTSIDE_CONTAINERS = "allowVarNodeOutsideContainer";
private static final String OPTION_ALLOW_READONLY_MUTABLE_PATHS = "allowReadOnlyMutablePaths";
private static final String OPTION_ALLOW_LIBS_NODE = "allowLibsNode";

private static final Logger LOGGER = LoggerFactory.getLogger(AemCloudValidatorFactory.class);

@Override
public Validator createValidator(@NotNull ValidationContext context, @NotNull ValidatorSettings settings) {
boolean allowVarNodesOutsideContainers = true;
if (settings.getOptions().containsKey(OPTION_ALLOW_VAR_NODE_OUTSIDE_CONTAINERS)) {
allowVarNodesOutsideContainers = Boolean.parseBoolean(settings.getOptions().get(OPTION_ALLOW_VAR_NODE_OUTSIDE_CONTAINERS));
boolean allowReadOnlyMutablePathsOutsideContainers = true;
if (settings.getOptions().containsKey(OPTION_ALLOW_READONLY_MUTABLE_PATHS)) {
allowReadOnlyMutablePathsOutsideContainers = Boolean.parseBoolean(settings.getOptions().get(OPTION_ALLOW_READONLY_MUTABLE_PATHS));
// deprecated option
} else if (settings.getOptions().containsKey(OPTION_ALLOW_VAR_NODE_OUTSIDE_CONTAINERS)) {
allowReadOnlyMutablePathsOutsideContainers = Boolean.parseBoolean(settings.getOptions().get(OPTION_ALLOW_VAR_NODE_OUTSIDE_CONTAINERS));
LOGGER.warn("Using deprecated option 'allowVarNodeOutsideContainer', please use 'allowReadOnlyMutablePaths' instead");
}
boolean allowLibsNode = false;
if (settings.getOptions().containsKey(OPTION_ALLOW_LIBS_NODE)) {
allowLibsNode = Boolean.parseBoolean(settings.getOptions().get(OPTION_ALLOW_LIBS_NODE));
}
return new AemCloudValidator(allowVarNodesOutsideContainers, allowLibsNode, context.getProperties().getPackageType(), context.getContainerValidationContext(), settings.getDefaultSeverity());
return new AemCloudValidator(allowReadOnlyMutablePathsOutsideContainers, allowLibsNode, context.getProperties().getPackageType(), context.getContainerValidationContext(), settings.getDefaultSeverity());
}

@Override
Expand Down
18 changes: 18 additions & 0 deletions src/test/java/biz/netcentric/filevault/validator/aem/cloud/AemCloudValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,22 @@ void testIsPackagePathInstalledConditionally() {
Assertions.assertTrue(AemCloudValidator.isPackagePathInstalledConditionally("author", Paths.get("/apps/install.author/container.zip")));
Assertions.assertTrue(AemCloudValidator.isPackagePathInstalledConditionally("author", Paths.get("/apps/install.author/subfolder/container.zip")));
}

@Test
void testIsMutablePath() {
Assertions.assertTrue(AemCloudValidator.isMutablePath("/"));
Assertions.assertTrue(AemCloudValidator.isMutablePath("/conf"));
Assertions.assertTrue(AemCloudValidator.isMutablePath("/conf/test"));
Assertions.assertFalse(AemCloudValidator.isMutablePath("/libs"));
Assertions.assertFalse(AemCloudValidator.isMutablePath("/libs/test"));
}

@Test
void testIsPathWritableByDistributionJournalImporter() {
Assertions.assertTrue(AemCloudValidator.isPathWritableByDistributionJournalImporter("/content"));
Assertions.assertTrue(AemCloudValidator.isPathWritableByDistributionJournalImporter("/content/test"));
Assertions.assertFalse(AemCloudValidator.isPathWritableByDistributionJournalImporter("/tmp"));
Assertions.assertFalse(AemCloudValidator.isPathWritableByDistributionJournalImporter("/var"));
Assertions.assertFalse(AemCloudValidator.isPathWritableByDistributionJournalImporter("/var/subnode/myfile"));
}
}