Skip to content

Added permission check to the known_host file #216

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

Merged
merged 1 commit into from
Aug 22, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@
import java.security.cert.X509Certificate;
import javax.net.ssl.X509TrustManager;

import org.neo4j.driver.v1.Logger;
import org.neo4j.driver.internal.util.BytePrinter;
import org.neo4j.driver.v1.Logger;

import static java.lang.String.format;
import static org.neo4j.driver.internal.util.CertificateTool.X509CertToString;

/**
Expand Down Expand Up @@ -77,6 +78,8 @@ private void load() throws IOException
return;
}

assertKnownHostFileReadable();

BufferedReader reader = new BufferedReader( new FileReader( knownHosts ) );
String line;
while ( (line = reader.readLine()) != null )
Expand Down Expand Up @@ -107,12 +110,38 @@ private void saveTrustedHost( String fingerprint ) throws IOException
logger.warn( "Adding %s as known and trusted certificate for %s.", fingerprint, serverId );
createKnownCertFileIfNotExists();

assertKnownHostFileWritable();
BufferedWriter writer = new BufferedWriter( new FileWriter( knownHosts, true ) );
writer.write( serverId + " " + this.fingerprint );
writer.newLine();
writer.close();
}


private void assertKnownHostFileReadable() throws IOException
{
if( !knownHosts.canRead() )
{
throw new IOException( format(
"Failed to load certificates from file %s as you have no read permissions to it.\n" +
"Try configuring the Neo4j driver to use a file system location you do have read permissions to.",
knownHosts.getAbsolutePath()
) );
}
}

private void assertKnownHostFileWritable() throws IOException
{
if( !knownHosts.canWrite() )
{
throw new IOException( format(
"Failed to write certificates to file %s as you have no write permissions to it.\n" +
"Try configuring the Neo4j driver to use a file system location you do have write permissions to.",
knownHosts.getAbsolutePath()
) );
}
}

/*
* Disallow all client connection to this client
*/
Expand Down Expand Up @@ -140,7 +169,7 @@ public void checkServerTrusted( X509Certificate[] chain, String authType )
}
catch ( IOException e )
{
throw new CertificateException( String.format(
throw new CertificateException( format(
"Failed to save the server ID and the certificate received from the server to file %s.\n" +
"Server ID: %s\nReceived cert:\n%s",
knownHosts.getAbsolutePath(), serverId, X509CertToString( cert ) ), e );
Expand All @@ -150,7 +179,7 @@ public void checkServerTrusted( X509Certificate[] chain, String authType )
{
if ( !this.fingerprint.equals( cert ) )
{
throw new CertificateException( String.format(
throw new CertificateException( format(
"Unable to connect to neo4j at `%s`, because the certificate the server uses has changed. " +
"This is a security feature to protect against man-in-the-middle attacks.\n" +
"If you trust the certificate the server uses now, simply remove the line that starts with " +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,17 @@
import org.junit.rules.TemporaryFolder;

import java.io.File;
import java.io.IOException;
import java.io.PrintWriter;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.util.Scanner;

import org.neo4j.driver.v1.Logger;

import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -141,4 +144,54 @@ private String nextLine( Scanner reader )
while ( line.trim().startsWith( "#" ) );
return line;
}

@Test
public void shouldThrowMeaningfulExceptionIfHasNoReadPermissionToKnownHostFile() throws Throwable
{
// Given
File knownHostFile = mock( File.class );
when( knownHostFile.canRead() ).thenReturn( false );
when( knownHostFile.exists() ).thenReturn( true );

// When & Then
try
{
new TrustOnFirstUseTrustManager( null, -1, knownHostFile, null );
fail( "Should have failed in load certs" );
}
catch( IOException e )
{
assertThat( e.getMessage(), containsString( "you have no read permissions to it" ) );
}
catch( Exception e )
{
fail( "Should not get any other error besides no permission to read" );
}
}

@Test
public void shouldThrowMeaningfulExceptionIfHasNoWritePermissionToKnownHostFile() throws Throwable
{
// Given
File knownHostFile = mock( File.class );
when( knownHostFile.exists() ).thenReturn( false /*skip reading*/, true );
when( knownHostFile.canWrite() ).thenReturn( false );

// When & Then
try
{
TrustOnFirstUseTrustManager manager =
new TrustOnFirstUseTrustManager( null, -1, knownHostFile, mock( Logger.class ) );
manager.checkServerTrusted( new X509Certificate[]{ knownCertificate}, null );
fail( "Should have failed in write to certs" );
}
catch( CertificateException e )
{
assertThat( e.getCause().getMessage(), containsString( "you have no write permissions to it" ) );
}
catch( Exception e )
{
fail( "Should not get any other error besides no permission to write" );
}
}
}