Skip to content

Commit

Permalink
Apply PR suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@edmocosta
edmocosta committed Mar 31, 2023
1 parent c0af983 commit b2a56ef
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 38 deletions.
23 changes: 15 additions & 8 deletions lib/logstash/inputs/beats.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,12 +178,19 @@ class LogStash::Inputs::Beats < LogStash::Inputs::Base
'cipher_suites',
].freeze

SSL_CLIENT_AUTH_NONE = 'none'.freeze
SSL_CLIENT_AUTH_OPTIONAL = 'optional'.freeze
SSL_CLIENT_AUTH_REQUIRED = 'required'.freeze

SSL_VERIFY_MODE_TO_CLIENT_AUTHENTICATION_MAP = {
'none' => 'none',
'peer' => 'optional',
'force_peer' => 'required'
'none' => SSL_CLIENT_AUTH_NONE,
'peer' => SSL_CLIENT_AUTH_OPTIONAL,
'force_peer' => SSL_CLIENT_AUTH_REQUIRED
}.freeze

private_constant :SSL_CLIENT_AUTH_NONE
private_constant :SSL_CLIENT_AUTH_OPTIONAL
private_constant :SSL_CLIENT_AUTH_REQUIRED
private_constant :NON_PREFIXED_SSL_CONFIGS
private_constant :SSL_VERIFY_MODE_TO_CLIENT_AUTHENTICATION_MAP

Expand Down Expand Up @@ -279,15 +286,15 @@ def client_authentication_metadata?
end

def client_authentication_required?
@ssl_client_authentication && @ssl_client_authentication.downcase == "required"
@ssl_client_authentication && @ssl_client_authentication.downcase == SSL_CLIENT_AUTH_REQUIRED
end

def client_authentication_optional?
@ssl_client_authentication && @ssl_client_authentication.downcase == "optional"
@ssl_client_authentication && @ssl_client_authentication.downcase == SSL_CLIENT_AUTH_OPTIONAL
end

def client_authentication_none?
@ssl_client_authentication && @ssl_client_authentication.downcase == 'none'
@ssl_client_authentication && @ssl_client_authentication.downcase == SSL_CLIENT_AUTH_NONE
end

def require_certificate_authorities?
Expand Down Expand Up @@ -323,12 +330,12 @@ def validate_ssl_config!
end

if client_authentication_metadata? && !require_certificate_authorities?
config_name, optional, required = provided_client_authentication_config(%w[optional required])
config_name, optional, required = provided_client_authentication_config([SSL_CLIENT_AUTH_OPTIONAL, SSL_CLIENT_AUTH_REQUIRED])
configuration_error "Configuring ssl_peer_metadata => true requires #{config_name} => to be configured with '#{optional}' or '#{required}'"
end

if original_params.include?('ssl_client_authentication') && certificate_authorities_configured? && !require_certificate_authorities?
configuration_error "Configuring ssl_certificate_authorities requires ssl_client_authentication => to be configured with 'optional' or 'required'"
configuration_error "Configuring ssl_certificate_authorities requires ssl_client_authentication => to be configured with '#{SSL_CLIENT_AUTH_OPTIONAL}' or '#{SSL_CLIENT_AUTH_REQUIRED}'"
end
end

Expand Down
3 changes: 1 addition & 2 deletions spec/inputs/beats_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,11 +302,10 @@
end
end

context "and ssl_ settings provided" do
context "and `ssl_` settings provided" do
let(:config) { { "port" => 0, "ssl_enabled" => false, "ssl_certificate" => certificate.ssl_cert, "ssl_client_authentication" => "none", "cipher_suites" => ["FOO"] } }

it "should warn about not using the configs" do
puts config
plugin = LogStash::Inputs::Beats.new(config)
expect( plugin.logger ).to receive(:warn).with('Configured SSL settings are not used when `ssl_enabled` is set to `false`: ["ssl_certificate", "ssl_client_authentication", "cipher_suites"]')

Expand Down
31 changes: 13 additions & 18 deletions src/main/java/org/logstash/netty/SslContextBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,18 @@
public class SslContextBuilder {

public enum SslClientVerifyMode {
NONE,
OPTIONAL,
REQUIRED;
NONE(ClientAuth.NONE),
OPTIONAL(ClientAuth.OPTIONAL),
REQUIRED(ClientAuth.REQUIRE);

private final ClientAuth clientAuth;

SslClientVerifyMode(ClientAuth clientAuth) {
this.clientAuth = clientAuth;
}

public ClientAuth toClientAuth() {
switch (this) {
case NONE:
return ClientAuth.NONE;
case OPTIONAL:
return ClientAuth.OPTIONAL;
case REQUIRED:
return ClientAuth.REQUIRE;
default:
throw new IllegalArgumentException(
String.format("Unsupported SSL client authentication mode `%s`", this.name().toLowerCase())
);
}
return clientAuth;
}
}

Expand Down Expand Up @@ -173,8 +168,8 @@ public static boolean isUnlimitedJCEAvailable() {

public SslContext buildContext() throws Exception {
if (logger.isDebugEnabled()) {
logger.debug(String.format("Available ciphers: %s", SUPPORTED_CIPHERS));
logger.debug(String.format("Ciphers: %s", Arrays.toString(ciphers)));
logger.debug("Available ciphers: {}", SUPPORTED_CIPHERS);
logger.debug("Ciphers: {}", Arrays.toString(ciphers));
}

io.netty.handler.ssl.SslContextBuilder builder = io.netty.handler.ssl.SslContextBuilder
Expand All @@ -184,7 +179,7 @@ public SslContext buildContext() throws Exception {

if (isClientAuthenticationEnabled(verifyMode)) {
if (logger.isDebugEnabled()) {
logger.debug(String.format("Certificate Authorities: %s", Arrays.toString(certificateAuthorities)));
logger.debug("Certificate Authorities: {}", Arrays.toString(certificateAuthorities));
}

builder.clientAuth(verifyMode.toClientAuth())
Expand Down
27 changes: 17 additions & 10 deletions src/test/java/org/logstash/netty/SslContextBuilderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,28 @@
import io.netty.handler.ssl.ClientAuth;
import io.netty.handler.ssl.SslContext;
import org.hamcrest.core.Every;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

import static org.hamcrest.Matchers.*;
import static org.hamcrest.core.Is.is;
import static org.junit.Assert.*;
import static org.logstash.netty.SslContextBuilder.*;

import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLServerSocketFactory;
import java.util.Arrays;
import java.util.List;

import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLServerSocketFactory;
import static org.hamcrest.Matchers.arrayWithSize;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.isIn;
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.core.Is.is;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.logstash.netty.SslContextBuilder.SUPPORTED_CIPHERS;
import static org.logstash.netty.SslContextBuilder.SslClientVerifyMode;
import static org.logstash.netty.SslContextBuilder.getDefaultCiphers;

/**
* Unit test for {@link SslContextBuilder}
Expand Down Expand Up @@ -136,14 +143,14 @@ private void assertSetClientAuthenticationThrowWithNoCerts(SslClientVerifyMode m

try {
sslContextBuilder.setClientAuthentication(mode, new String[0]);
Assert.fail("No exception was thrown");
fail("No exception was thrown");
} catch (IllegalArgumentException e) {
assertThat(e.getMessage(), is(expectedMessage));
}

try {
sslContextBuilder.setClientAuthentication(mode, null);
Assert.fail("No exception was thrown");
fail("No exception was thrown");
} catch (IllegalArgumentException e) {
assertThat(e.getMessage(), is(expectedMessage));
}
Expand Down

0 comments on commit b2a56ef

Please sign in to comment.