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

Supplemental changes for PR #1357 #1363

Merged
merged 13 commits into from Aug 21, 2015

Conversation

Projects
None yet
3 participants
@joschi
Contributor

joschi commented Aug 20, 2015

Some supplemental changes to PR #1357 to speed up things due to the upcoming code freeze.

@joschi joschi added this to the 1.2.0 milestone Aug 20, 2015

@bernd bernd self-assigned this Aug 20, 2015

CK_TLS_NEED_CLIENT_AUTH,
"TLS Need Client Auth",
false,
"TLS Need Client Auth")

This comment has been minimized.

@bernd

bernd Aug 21, 2015

Member

I think it would be good to have a small description what "Need Client Auth" (and also "Want Client Auth") actually means.

This comment has been minimized.

@bernd

bernd Aug 21, 2015

Member

Also setting "need client auth" does not have any effect. I can still send with a client that has no client certificates configured. Only "want client auth" seems to make a difference. If it is checked, the connection fails if no client certs are configured.

Can someone explain why we need both? @gbu-censhare

This comment has been minimized.

@gbu-censhare

gbu-censhare Aug 21, 2015

Contributor

From JavaDoc it comes like this:

setNeedClientAuth: Configures the engine to require client authentication. This option is only useful for engines in the server mode. An engine's client authentication setting is one of the following: client authentication required, client authentication requested no client authentication desired

setWantClientAuth: Configures the engine to request client authentication. This option is only useful for engines in the server mode. An engine's client authentication setting is one of the following: client authentication required, client authentication requested, no client authentication desired

so "want" asks if possible, "need" requires one from documentation, "want" is optional in my option, i added it for completeness.

This comment has been minimized.

@bernd

bernd Aug 21, 2015

Member

Okay, the confusion is solved.

The "want" option accepts clients without any cert and clients with a valid cert. Clients with invalid certs are not accepted.

The "need" option only accepts clients with a valid certificate.

The code needs to be changed to only set one of the options because they overwrite each other. @joschi is currently working on that.

final char[] password = Strings.nullToEmpty(tlsKeyPassword).toCharArray();
ks.setKeyEntry("key", privateKey, password, certChain.toArray(new Certificate[certChain.size()]));
LOG.info("KeyStore: {}, aliases: {}", ks, join(ks.aliases()));

This comment has been minimized.

@bernd

bernd Aug 21, 2015

Member

This logs the following for each new tcp connection:

2015-08-21 12:51:23,336 INFO : org.graylog2.plugin.inputs.transports.util.KeyUtil - KeyStore: java.security.KeyStore@2fda2c1, aliases: key

Please change that to debug.

This comment has been minimized.

@joschi

joschi Aug 21, 2015

Contributor

Hm, this also seems to be a quite expensive operation to do on each new connection. Maybe we can cache this somehow.

This comment has been minimized.

@bernd

bernd Aug 21, 2015

Member

Is it more expensive than the SslContext operation we did before?

This comment has been minimized.

@bernd

bernd Aug 21, 2015

Member

Good thing not caching it is that you can just replace certificates at runtime.

This comment has been minimized.

@gbu-censhare

gbu-censhare Aug 21, 2015

Contributor

it has to be more expensive, if client certificates are involved, it simply has to parse more files.

live replacing could be achieved by filesystem watchers, e.g.

engine.setUseClientMode(false);
engine.setNeedClientAuth(tlsNeedClientAuth);
engine.setNeedClientAuth(tlsWantClientAuth);

This comment has been minimized.

@bernd

bernd Aug 21, 2015

Member

This should be engine.setWantClientAuth(tlsWantClientAuth);.

final SSLContext instance = SSLContext.getInstance("TLS");
TrustManager[] initTrustStore = new TrustManager[0];
if ((tlsWantClientAuth || tlsNeedClientAuth)) {

This comment has been minimized.

@bernd

bernd Aug 21, 2015

Member

Unneeded parens.

@gbu-censhare

This comment has been minimized.

Contributor

gbu-censhare commented Aug 21, 2015

nice, thanks, for spotting my mistakes.
i have an additional switch built in

to enable tcp compression, so if you're interessted i can create a pull request for this:
https://github.com/gbu-censhare/graylog2-server/commit/60e1de7c1acca65317630b85ecec9ec64f1e6598

@bernd

This comment has been minimized.

Member

bernd commented Aug 21, 2015

LGTM and tested with several clients. 👍

bernd added a commit that referenced this pull request Aug 21, 2015

Merge pull request #1363 from Graylog2/pr-1357
Supplemental changes for PR #1357

@bernd bernd merged commit 0ebd940 into master Aug 21, 2015

3 checks passed

ci Jenkins build graylog2-server-integration-pr 167 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bernd bernd deleted the pr-1357 branch Aug 21, 2015

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