Issue #6114 - hot deploy of symlink results in symlink name

[joakime]

• Using getAbsolutePath not canonical (too many corner cases with it to be reliable)

Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com

[joakime]

This PR also addresses a disabled testcase in Issue #5684

[janbartel]

I'm questioning the need for this change.

The javadoc for File.getCanonicalPath says:

A canonical pathname is both absolute and unique. The precise definition of canonical form is system-dependent. This method first converts this pathname to absolute form if necessary, as if by invoking the getAbsolutePath() method, and then maps it to its unique form in a system-dependent way. This typically involves removing redundant names such as "." and ".." from the pathname, resolving symbolic links (on UNIX platforms), and converting drive letters to a standard case (on Microsoft Windows platforms).

Therefore a canonical path is an absolute path.

[joakime]

There are 2 major reasons.

Canonical path requires filesystem permissions for all paths necessary to create the canonical path and it follows symlinks.

Example:

C:\sites\customer1\webapp\

Where C:\sites is setup to be no permissions to the running server user.
But the C:\sites\customer1 has permissions for the running server user.
If you attempt to new File("C:\sites\customer1\webapp\").getCanonicalPath() it will fail.
If you use new File("C:\sites\customer1\webapp\").getAbsolutePath() it will work.

The use of getCanonicalPath above will result in either an IOException or a more specific FileSystemException.
With getAbsolutePath it will just work.

Absolute path does not require extra filesystem permissions, and does not follow links, it merely resolves them (as a directory or a file in place).

This gets even more complicated on filesystems that are different from the default, such as network shares, virtual filesystems (git/gitlab/github/perforce/docker to name a few), and even alternate filesystem implementations (seen in increasing frequency in data centers for example)

An alternative would be Path.toRealPath(LinkOption...) as that would allow for better control over symlink behavior (follow vs resolve) and even detects filesystem loops (FileSystemLoopException) with symlinks, and even invalid symlinks (NotLinkException).

The Scanner operating on getCanonicalPath breaks the ability to have symlinks in /webapps/ directory and deploy as the name on the symlink, it forces the name to be the canonical name (what we don't want).

In short, getCanonicalPath is a bad API for ..

• users that care about filesystem permissions on servers
• users that would like to use symlink deployment as the symlink name (be it a xml deployable, a war, or a directory)
• users that have filesystems that are not default for the OS and/or local

[janbartel]

I'm cautious about making this change at all. It seems more secure to be both absolute and canonical for the majority of operating systems. If some operating systems have different behaviours with canonicalization what is the risk? That a webapp won't be deployed? That a webapp will be redeployed multiple times for every scan?

BTW the Scanner has always worked on canonical paths. That being the case, how was it possible for the usecase with multiple symlinks in the /webapps directory to have produced multiple deployments, or is this a new usecase?

[joakime]

The testcase for this PR isn't new, it was created back when we underwent the Scanner > PathWatcher > Scanner changes a few years ago.
It was disabled when the Scanner was rewritten during the change back to Scanner.
We used to support proper symlink behavior, the getCanonicalPath change broke it.
The scanInfoMap in the Scanner was added around this time to detect filesystem loops and "have we seen this" behaviors, something that the PathWatcher (and the nio.Files/nio.Paths) already support built-in.

getCanonicalPath (and getCanonicalFile) has a history with this project were we remove it when it becomes a problem (either reported, or discovered), it's been done in the WebAppClassloader already, it's been done in the Resource layer already, it's been done in jetty-start already.

Do we have usages of getCanonicalPath still around? Yes, but the vast majority is our own test cases (where we don't have to worry about the variety of environments it runs on).

The knowledge of the canonical path in the Scanner has no value, so why break legit usage by insisting on it.
Absolute is sufficient, and causes less problems.

[gregw]

I'm also cautious of this change. The removal of getCanonicalPath has already caused one significant CVE.
If we do remove it, I think using toRealPath would be better as it allows control over symlink handling.

Most importantly, the handling must be the same in the Scanner as it is in WebAppProvider, but the Scanner is used elsewhere, so changing it may break other things.

Thus I think we need to do is create an interface like

    public interface Normalizer
    {
        String normalize(File f) throws IOException;
    }

Which can optionally be passed into the constructor of Scanner. If not passed, then it is initialised to File::getCanonicalPath).
This keeps the Scanner exactly the same for existing uses. Then the WebAppProvider can have some configuration to say if it uses canonical or not, and/or if it follows symlinks. This config can be used the select a Normalizer, which will then be used both by the Scanner and the WebAppProvider.

[gregw]

see previous comment

[joakime]

The Normalizer would then need to be OS and FileSystem specific.
There would be no good "default" that could be documented.

There would just be one with getCanonicalPath for users on Linux with ext3 or ext4.
And getAbsolutePath for everyone else (Windows, OSX, docker, zfs, etc)

[gregw]

@joakime, it may not even be that. It might be a normalizer with getCanonicalPath for any other direct usages of the Scanner and a normalizer with getAbsolutePath for using the scanner in the DeployerProvider.

It's not about different OS's. It's about not changing the Scanner just so it syncs with the Deployer. By having it pluggable the deployer can ensure that the same impl is used and not be vulnerable if the Scanner is subsequently changed. It is about non fragile code.

[janbartel]

@joakime when we resolve this issue, can you also take a look at 2 @Disabled tests that touch on this very subject at https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-deploy/src/test/java/org/eclipse/jetty/deploy/providers/WebAppProviderTest.java#L104 and https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-deploy/src/test/java/org/eclipse/jetty/deploy/providers/WebAppProviderTest.java#L121 and then tick them off the list at #5684

[joakime]

This will need to be reworked, in light of the changes in #6317