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

Issue #5451 - Private Work Dir #5457

Closed
wants to merge 4 commits into from
Closed

Issue #5451 - Private Work Dir #5457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a23b01e
Issue #5451 - Setting ServletContext temp dir to user private
joakime Oct 16, 2020
74d1473
Issue #5451 - Setting ServletContext temp dir to user private
joakime Oct 16, 2020
3cc549c
Issue #5451 - Adding configurable posix perms for ServletContext temp…
joakime Oct 16, 2020
75e6aaf
Issue #5451 - Allowing inversion of ${jetty.base}/work/ persistence
joakime Oct 16, 2020
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
5 changes: 2 additions & 3 deletions jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@

import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.util.ByteArrayOutputStream2;
import org.eclipse.jetty.util.IO;
import org.eclipse.jetty.util.LazyList;
import org.eclipse.jetty.util.MultiException;
import org.eclipse.jetty.util.MultiMap;
Expand Down Expand Up @@ -152,8 +151,8 @@ protected void write(byte[] bytes, int offset, int length) throws IOException

protected void createFile() throws IOException
{
Path parent = MultiPartFormInputStream.this._tmpDir.toPath();
Path tempFile = Files.createTempFile(parent, "MultiPart", "", IO.getUserOnlyFileAttribute(parent));
// Create temp file in Context tempDir (which is user private already)
Path tempFile = Files.createTempFile(MultiPartFormInputStream.this._tmpDir.toPath(), "MultiPart", "");
_file = tempFile.toFile();

OutputStream fos = Files.newOutputStream(tempFile, StandardOpenOption.WRITE);
Expand Down
28 changes: 28 additions & 0 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Server.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ public class Server extends HandlerWrapper implements Attributes
private final ThreadPool _threadPool;
private final List<Connector> _connectors = new CopyOnWriteArrayList<>();
private SessionIdManager _sessionIdManager;
private String _tmpDirPosixPerms = "rwx------";
private boolean _workDirPersistent = true;
private boolean _stopAtShutdown;
private boolean _dumpAfterStart = false;
private boolean _dumpBeforeStop = false;
Expand Down Expand Up @@ -210,6 +212,16 @@ public void setStopAtShutdown(boolean stop)
_stopAtShutdown = stop;
}

public String getTempDirectoryPosixPermissions()
{
return _tmpDirPosixPerms;
}

public boolean isWorkDirectoryPersistent()
{
return _workDirPersistent;
}

/**
* @return Returns the connectors.
*/
Expand Down Expand Up @@ -583,6 +595,22 @@ public void setSessionIdManager(SessionIdManager sessionIdManager)
_sessionIdManager = sessionIdManager;
}

/**
* Set the POSIX permission string used for the Temp Directory creation for all webapps deployed on the server.
*
* @param perms the string for temp directory permissions
* @see java.nio.file.attribute.PosixFilePermissions#fromString(String)
*/
public void setTempDirectoryPosixPermissions(String perms)
{
_tmpDirPosixPerms = perms;
}

public void setWorkDirectoryPersistent(boolean flag)
{
_workDirPersistent = flag;
}

/*
* @see org.eclipse.util.AttributesMap#clearAttributes()
*/
Expand Down
78 changes: 0 additions & 78 deletions jetty-util/src/main/java/org/eclipse/jetty/util/IO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,8 @@
import java.nio.ByteBuffer;
import java.nio.channels.GatheringByteChannel;
import java.nio.charset.Charset;
import java.nio.file.FileStore;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.DosFileAttributeView;
import java.nio.file.attribute.FileAttribute;
import java.nio.file.attribute.PosixFileAttributeView;
import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.PosixFilePermissions;
import java.util.HashSet;
import java.util.Objects;

import org.eclipse.jetty.util.log.Log;
import org.eclipse.jetty.util.log.Logger;
Expand All @@ -57,24 +49,6 @@ public class IO
{
private static final Logger LOG = Log.getLogger(IO.class);

private static final FileAttribute<?>[] NO_FILE_ATTRIBUTES = new FileAttribute[0];
private static final FileAttribute<?>[] USER_ONLY_POSIX_FILE_ATTRIBUTES =
new FileAttribute[]{
PosixFilePermissions.asFileAttribute(
new HashSet<PosixFilePermission>()
{
{
add(PosixFilePermission.OWNER_EXECUTE);
add(PosixFilePermission.OWNER_READ);
add(PosixFilePermission.OWNER_WRITE);
// we don't add GROUP or OTHER write perms here.
add(PosixFilePermission.GROUP_READ);
add(PosixFilePermission.OTHERS_READ);
}
}
)
};

public static final String
CRLF = "\r\n";

Expand Down Expand Up @@ -462,58 +436,6 @@ public static void close(Writer writer)
close((Closeable)writer);
}

/**
* Get the array of {@link FileAttribute} values for the provided path
* that will set the path to Full Read/Write for the user running Jetty,
* but Readonly for other users.
* <p>
* For Unix, that's means {@link java.nio.file.attribute.PosixFileAttributes}
* where the World and Other groups have their read / write flags removed.
* </p>
* <p>
* For Windows / Dos, that means {@link java.nio.file.attribute.DosFileAttributes}
* </p>
*/
public static FileAttribute<?>[] getUserOnlyFileAttribute(Path path)
{
FileStore fileStore = null;
try
{
// Obtain a reference to the FileStore to know what kind of read-only we are capable of.
fileStore = Files.getFileStore(Objects.requireNonNull(path));

if (fileStore == null)
{
// Not on a properly implemented FileStore (seen with 3rd party FileStore implementations)
// We cannot do anything in this case, so just return.
return NO_FILE_ATTRIBUTES;
}

if (fileStore.supportsFileAttributeView(DosFileAttributeView.class))
{
// We are on a Windows / DOS filesystem.
// It might support ACL, but we don't attempt to support that here.
return NO_FILE_ATTRIBUTES;
}

if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class))
{
// We are on a Unix / Linux / OSX system
return USER_ONLY_POSIX_FILE_ATTRIBUTES;
}

// If we reached this point, we have a Path on a FileSystem / FileStore that we cannot control.
// So skip the attempt to set readable.
}
catch (IOException e)
{
if (LOG.isDebugEnabled())
LOG.debug("Unable to determine attribute types on path: {}", path, e);
}

return NO_FILE_ATTRIBUTES;
}

public static byte[] readBytes(InputStream in)
throws IOException
{
Expand Down
4 changes: 2 additions & 2 deletions jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,8 @@ protected void write(byte[] bytes, int offset, int length)
protected void createFile()
throws IOException
{
Path parent = MultiPartInputStreamParser.this._tmpDir.toPath();
Path tempFile = Files.createTempFile(parent, "MultiPart", "", IO.getUserOnlyFileAttribute(parent));
// Create temp file in Context tempDir (which is user private already)
Path tempFile = Files.createTempFile(MultiPartInputStreamParser.this._tmpDir.toPath(), "MultiPart", "");
_file = tempFile.toFile();

OutputStream fos = Files.newOutputStream(tempFile, StandardOpenOption.WRITE);
Expand Down
24 changes: 24 additions & 0 deletions jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import org.eclipse.jetty.util.AttributesMap;
import org.eclipse.jetty.util.Loader;
import org.eclipse.jetty.util.MultiException;
import org.eclipse.jetty.util.StringUtil;
import org.eclipse.jetty.util.URIUtil;
import org.eclipse.jetty.util.annotation.ManagedAttribute;
import org.eclipse.jetty.util.annotation.ManagedObject;
Expand Down Expand Up @@ -177,6 +178,7 @@ public class WebAppContext extends ServletContextHandler implements WebAppClassL

private File _tmpDir;
private boolean _persistTmpDir = false;
private String _tmpDirPosixPerms;

private String _war;
private String _extraClasspath;
Expand Down Expand Up @@ -517,6 +519,11 @@ protected void doStart() throws Exception
{
try
{
if (StringUtil.isBlank(_tmpDirPosixPerms))
{
_tmpDirPosixPerms = getServer().getTempDirectoryPosixPermissions();
}

_metadata.setAllowDuplicateFragmentNames(isAllowDuplicateFragmentNames());
Boolean validate = (Boolean)getAttribute(MetaData.VALIDATE_XML);
_metadata.setValidateXml((validate != null && validate));
Expand Down Expand Up @@ -1298,12 +1305,29 @@ public void setTempDirectory(File dir)
setAttribute(TEMPDIR, _tmpDir);
}

/**
* Set the POSIX permission string used for the Temp Directory for this specific webapp.
*
* @param perms the string for temp directory permissions
* @see java.nio.file.attribute.PosixFilePermissions#fromString(String)
*/
public void setTempDirectoryPosixPermissions(String perms)
{
this._tmpDirPosixPerms = perms;
}

@ManagedAttribute(value = "temporary directory location", readonly = true)
public File getTempDirectory()
{
return _tmpDir;
}

@ManagedAttribute(value = "temporary directory perms", readonly = true)
public String getTempDirectoryPosixPermissions()
{
return _tmpDirPosixPerms;
}

/**
* If true the temp directory for this
* webapp will be kept when the webapp stops. Otherwise,
Expand Down
23 changes: 20 additions & 3 deletions jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,12 @@
import java.net.URISyntaxException;
import java.net.URL;
import java.net.URLClassLoader;
import java.nio.file.FileStore;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.PosixFileAttributeView;
import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.PosixFilePermissions;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -456,7 +460,10 @@ public void resolveTempDirectory(WebAppContext context)
File work = new File(jettyBase, "work");
if (work.exists() && work.isDirectory() && work.canWrite())
{
context.setPersistTempDirectory(true);
if (context.getServer().isWorkDirectoryPersistent())
{
context.setPersistTempDirectory(true);
}
Comment on lines +463 to +466
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should also do this with an attribute so we don't commit to a forever API:

Suggested change
if (context.getServer().isWorkDirectoryPersistent())
{
context.setPersistTempDirectory(true);
}
Object workNotPersistent = context.getServer().getAttribute(WORK_DIRECTORY_NOT_PERSISTENT);
if (workNotPersistent != null && Boolean.valueOf(workNotPersistent))
context.setPersistTempDirectory(true);

makeTempDirectory(work, context);
return;
}
Expand Down Expand Up @@ -507,8 +514,18 @@ public void makeTempDirectory(File parent, WebAppContext context)
}
else
{
//ensure file will always be unique by appending random digits
tmpDir = Files.createTempDirectory(parent.toPath(), temp).toFile();
Path parentPath = parent.toPath();
FileStore fileStore = Files.getFileStore(parentPath);
if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class))
{
String workDirPerms = context.getTempDirectoryPosixPermissions();
Set<PosixFilePermission> permSet = PosixFilePermissions.fromString(workDirPerms);
tmpDir = Files.createTempDirectory(parentPath, temp, PosixFilePermissions.asFileAttribute(permSet)).toFile();
}
Comment on lines +519 to +524
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need to create a lot of new API for this. Can we just do it with attributes: context, server then system

Suggested change
if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class))
{
String workDirPerms = context.getTempDirectoryPosixPermissions();
Set<PosixFilePermission> permSet = PosixFilePermissions.fromString(workDirPerms);
tmpDir = Files.createTempDirectory(parentPath, temp, PosixFilePermissions.asFileAttribute(permSet)).toFile();
}
if (fileStore.supportsFileAttributeView(PosixFileAttributeView.class))
{
String workDirPerms = (String) context.getAttribute(TEMP_DIRECTORY_POSIX_PERMISSIONS);
if (workDirPerms == null)
workDirPerms = (String) context.getServer().getAttribute(TEMP_DIRECTORY_POSIX_PERMISSIONS);
if (workDirPerms == null)
workDirPerms = (String) System.getProperty(TEMP_DIRECTORY_POSIX_PERMISSIONS, "rwx------");
Set<PosixFilePermission> permSet = PosixFilePermissions.fromString(workDirPerms);
tmpDir = Files.createTempDirectory(parentPath, temp, PosixFilePermissions.asFileAttribute(permSet)).toFile();
}

We've changed tmp dir stuff a number of times... once a thing is said in API it is forever! So I think we just lean towards to loose attributes rather than API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the String you see for TEMP_DIRECTORY_POSIX_PERMISSIONS ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The biggest issue with attributes is making them friendly for start modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, where does TEMP_DIRECTORY_POSIX_PERMISSIONS exist?
Not the WebAppContext, how about IO ? or the Server?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is closed, but will answer so record my thoughts.

I see these attributes as hints to the implementation, so the string should be "org.eclipse.jetty.webapp.WebInfConfiguration.TEMP_DIRECTORY_POSIX_PERMISSIONS" and defined as a constant in WebInfConfiguration.

It is easy to make friendly for start modules, because we write the XML for those modules, thus we write the XML like:

  <Call name="setAttribute">
    <Arg>org.eclipse.jetty.webapp.WebInfConfiguration.TEMP_DIRECTORY_POSIX_PERMISSIONS</arg>
    <Arg><Property name="jetty.webapp.tmpDirPosixPermissions"/></Arg>
  </Call>

and then we just add to the ini-template of webapp.mod:

## Set the permissions of temporary directory on POSIX file systems
# jetty.webapp.tmpDirPosixPermissions=rwx------

Ultimately, this is no different at the start module level than if we had dedicated APIs

else
{
tmpDir = Files.createTempDirectory(parentPath, temp).toFile();
}
}
configureTempDirectory(tmpDir, context);

Expand Down