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.

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

Add checksum validation to library manager #802

Merged
merged 17 commits into from Jan 8, 2018

Conversation

Projects
None yet
4 participants
@momothereal
Member

momothereal commented Jan 5, 2018

This is my take to improve the library manager by adding checksum validation and allowing configuration of repository/lib directory.

#543

I'm open to suggestions or rejection :)

@Pr0methean

Why not signature checking while we're at it?

@momothereal

This comment has been minimized.

Member

momothereal commented Jan 5, 2018

@Pr0methean I am not familiar with Java's SSL API. Do you know if/how the signature is exposed in the HttpsURLConnection API? I'm not sure where to look.

@mastercoms

This comment has been minimized.

Member

mastercoms commented Jan 7, 2018

I feel like it would be better to have the checksums hard coded in, since this is being used for file download integrity.

@momothereal momothereal requested a review from mastercoms Jan 8, 2018

momothereal added some commits Jan 8, 2018

public enum HashAlgorithm {
SHA1(DEFAULT_HASH_FUNCTION, DEFAULT_HASH_FUNCTION_NAME),

This comment has been minimized.

@smartboyathome

smartboyathome Jan 8, 2018

Contributor

One comment here, I don't think DEFAULT_HASH_FUNCTION_NAME and ALTERNATIVE_HASH_FUNCTION_NAME should be static variables, since they're already properties of the enum. In fact, I would argue that ALTERNATIVE_HASH_FUNCTION should be gotten rid of as a static variable as well, since it'll make for some awkward code once a third hash is added.

@@ -52,13 +88,20 @@ public void run() {
}
downloaderService.execute(new LibraryDownloader(
"org.xerial", "sqlite-jdbc", "3.21.0", ""));
"org.xerial", "sqlite-jdbc", "3.21.0",
"347e4d1d3e1dff66d389354af8f0021e62344584", DEFAULT_HASH_ALGORITHM));

This comment has been minimized.

@smartboyathome

smartboyathome Jan 8, 2018

Contributor

I think using DEFAULT_HASH_ALGORITHM here actually makes the code a bit less clear. With that, I have to look up that the default hash algorithm is SHA1, and if we ever need to change that then all these hashes will have to change. If you used HashAlgorithm.SHA1, it is immediately clear what type of hash this is and allows us to switch between hashes without changing all the entries at once.

@@ -412,6 +414,9 @@ private boolean migrate() {
REGION_CACHE_SIZE("advanced.region-file.cache-size", 256),
REGION_COMPRESSION("advanced.region-file.compression", true),
PROFILE_LOOKUP_TIMEOUT("advanced.profile-lookup-timeout", 5),
LIBRARY_CHECKSUM_VALIDATION("advanced.library-checksum-validation", true),
LIBRARY_REPOSITORY_URL("advanced.library-repository-url",
"https://repo.glowstone.net/service/local/repositories/central/content/"),

This comment has been minimized.

@smartboyathome

smartboyathome Jan 8, 2018

Contributor

More of a question than "please change this", but do we want the default library url to really be our repo? Even though it proxies back to Maven Central, thats still using our bandwidth whenever anyone builds. This question becomes more important when custom libraries are introduced, but I wanted to call it out now.

This comment has been minimized.

@mastercoms

mastercoms Jan 8, 2018

Member

The reason why we use our own repo is because Minecraft Forge was asked to stop using Maven Central when they added library downloading in their installer, so we are trying to proactively avoid that issue by using our own repo.

This comment has been minimized.

@smartboyathome

smartboyathome Jan 8, 2018

Contributor

Ah, didn't know about that. Makes sense then.

momothereal added some commits Jan 8, 2018

Doc
} catch (IOException e) {
GlowServer.logger.log(Level.WARNING,
"Failed to download: " + library + ' ' + version, e);
file.delete();
return;
}
} else if (validateChecksum && algorithm != null && checksum != null

This comment has been minimized.

@mastercoms

mastercoms Jan 8, 2018

Member

Is this case for when someone manually puts their own lib in the folder?

This comment has been minimized.

@momothereal

momothereal Jan 8, 2018

Member

Yes, if they overwrite the library (or change its contents) for some reason.

+ "' does not match. "
+ "Restart the server to download it again.");
file.delete();
return;

This comment has been minimized.

@mastercoms

mastercoms Jan 8, 2018

Member

Could we have a retry system that gives up after a second or third try?

This comment has been minimized.

@momothereal

momothereal Jan 8, 2018

Member

Done, looks like this:

13:52:20 [INFO] Downloading sqlite-jdbc 3.21.0...
13:52:20 [INFO] Downloading mysql-connector-java 5.1.44...
13:52:20 [INFO] Downloading log4j-api 2.8.1...
13:52:20 [INFO] Downloading log4j-core 2.8.1...
13:52:20 [INFO] Downloading commons-lang3 3.5...
13:52:21 [INFO] Downloaded log4j-api 2.8.1.
13:52:21 [INFO] Downloaded commons-lang3 3.5.
13:52:22 [INFO] Downloaded mysql-connector-java 5.1.44.
13:52:23 [INFO] Downloaded log4j-core 2.8.1.
13:52:27 [INFO] Downloaded sqlite-jdbc 3.21.0.
13:52:27 [SEVERE] The checksum for the library 'sqlite-jdbc-3.21.0.jar' does not match. Attempting download again (2/2)
13:52:27 [INFO] Downloading sqlite-jdbc 3.21.0...
13:52:32 [INFO] Downloaded sqlite-jdbc 3.21.0.
13:52:32 [SEVERE] The checksum for the library 'sqlite-jdbc-3.21.0.jar' does not match. Restart the server to download it again.

This comment has been minimized.

@mastercoms

mastercoms Jan 8, 2018

Member

Perhaps to be more clear, specify that restarting the server only attempts to download it again and that server owners should check to see what the problem is or wait out connection issues before trying again.

// TODO: actually check checksum
return file.exists();
/**
* Computes and validates the checksum of a file.

This comment has been minimized.

@mastercoms

mastercoms Jan 8, 2018

Member

Add newlines between each paragraph.

This comment has been minimized.

@momothereal

momothereal added some commits Jan 8, 2018

@mastercoms mastercoms merged commit 825e511 into dev Jan 8, 2018

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

@mastercoms mastercoms deleted the libraries branch Jan 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment