Skip to content

Commit

Permalink
Further changes/fixes
Browse files Browse the repository at this point in the history 
- We have no reason to override the transport ssl settings in
AbstractAdLdapRealmTestCase so use what was generated already
- Handle renaming properly in MultipleAdRealmIT
  • Loading branch information
@jkakavas
jkakavas committed Oct 1, 2019
1 parent 8494b38 commit 3bc5d90
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 53 deletions.
54 changes: 3 additions & 51 deletions ...rc/test/java/org/elasticsearch/xpack/security/authc/ldap/AbstractAdLdapRealmTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import org.elasticsearch.client.Client;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.settings.MockSecureSettings;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.set.Sets;
Expand All @@ -25,16 +24,14 @@
import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingResponse;
import org.elasticsearch.xpack.core.security.authc.ldap.ActiveDirectorySessionFactorySettings;
import org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken;
import org.elasticsearch.xpack.core.ssl.VerificationMode;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.UncheckedIOException;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -99,8 +96,6 @@ public abstract class AbstractAdLdapRealmTestCase extends SecurityIntegTestCase
)
};

protected static final String TESTNODE_KEY = "/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.pem";
protected static final String TESTNODE_CERT = "/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.crt";
protected static RealmConfig realmConfig;
protected static List<RoleMappingEntry> roleMappings;

Expand All @@ -121,42 +116,8 @@ public static void cleanupRealm() {
@Override
protected Settings nodeSettings(int nodeOrdinal) {
final RealmConfig realm = AbstractAdLdapRealmTestCase.realmConfig;
final Path nodeCert = getDataPath(TESTNODE_CERT);
final Path nodeKey = getDataPath(TESTNODE_KEY);
Settings.Builder builder = Settings.builder();
// don't use filter since it returns a prefixed secure setting instead of mock!
Settings settingsToAdd = super.nodeSettings(nodeOrdinal);
builder.put(settingsToAdd.filter(k -> k.startsWith("xpack.security.transport.ssl.") == false), false);
MockSecureSettings mockSecureSettings = (MockSecureSettings) Settings.builder().put(settingsToAdd).getSecureSettings();
if (mockSecureSettings != null) {
MockSecureSettings filteredSecureSettings = new MockSecureSettings();
builder.setSecureSettings(filteredSecureSettings);
for (String secureSetting : mockSecureSettings.getSettingNames()) {
if (secureSetting.startsWith("xpack.security.transport.ssl.") == false) {
SecureString secureString = mockSecureSettings.getString(secureSetting);
if (secureString == null) {
final byte[] fileBytes;
try (InputStream in = mockSecureSettings.getFile(secureSetting);
ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream()) {
int numRead;
byte[] bytes = new byte[1024];
while ((numRead = in.read(bytes)) != -1) {
byteArrayOutputStream.write(bytes, 0, numRead);
}
byteArrayOutputStream.flush();
fileBytes = byteArrayOutputStream.toByteArray();
} catch (IOException e) {
throw new UncheckedIOException(e);
}

filteredSecureSettings.setFile(secureSetting, fileBytes);
} else {
filteredSecureSettings.setString(secureSetting, new String(secureString.getChars()));
}
}
}
}
addSslSettingsForKeyPair(builder, nodeKey, "testnode", nodeCert, getNodeTrustedCertificates());
builder.put(super.nodeSettings(nodeOrdinal), true);
builder.put(buildRealmSettings(realm, roleMappings, getNodeTrustedCertificates()));
return builder.build();
}
Expand Down Expand Up @@ -286,15 +247,6 @@ protected static String userHeader(String username, String password) {
return UsernamePasswordToken.basicAuthHeaderValue(username, new SecureString(password.toCharArray()));
}

private void addSslSettingsForKeyPair(Settings.Builder builder, Path key, String keyPassphrase, Path cert,
List<String> certificateAuthorities) {
builder.put("xpack.security.transport.ssl.key", key)
.put("xpack.security.transport.ssl.key_passphrase", keyPassphrase)
.put("xpack.security.transport.ssl.verification_mode", "certificate")
.put("xpack.security.transport.ssl.certificate", cert)
.putList("xpack.security.transport.ssl.certificate_authorities", certificateAuthorities);
}

/**

This comment has been minimized.

Sign in to view

Copy link
@albertzaharovits

albertzaharovits Oct 2, 2019

Contributor

This is unimportant , it did not cause the test to fail, right?

This comment has been minimized.

Sign in to view

Copy link
@jkakavas

jkakavas Oct 2, 2019

Author Member

This caused the 7.x ci builds to fail
* Collects all the certificates that are normally trusted by the node ( contained in testnode.jks )
*/
Expand Down Expand Up @@ -436,7 +388,7 @@ public Settings buildSettings(List<String> certificateAuthorities) {
protected Settings buildSettings(List<String> certificateAuthorities, int order) {
Settings.Builder builder = Settings.builder()
.put("xpack.security.authc.realms." + type + ".external.order", order)
.put("xpack.security.authc.realms." + type + ".external.hostname_verification", false)
.put("xpack.security.authc.realms." + type + ".external.ssl.verification_mode", VerificationMode.CERTIFICATE)
.put("xpack.security.authc.realms." + type + ".external.unmapped_groups_as_roles", mapGroupsAsRoles)
.put(this.settings)
.putList("xpack.security.authc.realms." + type + ".external.ssl.certificate_authorities", certificateAuthorities);
Expand Down
10 changes: 8 additions & 2 deletions ...irectory/src/test/java/org/elasticsearch/xpack/security/authc/ldap/MultipleAdRealmIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import org.apache.logging.log4j.LogManager;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.xpack.core.security.authc.ldap.LdapRealmSettings;
import org.junit.BeforeClass;

import java.io.IOException;
Expand Down Expand Up @@ -49,8 +50,13 @@ protected Settings nodeSettings(int nodeOrdinal) {
final Settings secondarySettings = super.buildRealmSettings(secondaryRealmConfig, secondaryRoleMappings,
getNodeTrustedCertificates());
secondarySettings.keySet().forEach(name -> {
String newName = name.replace(XPACK_SECURITY_AUTHC_REALMS_AD_EXTERNAL, XPACK_SECURITY_AUTHC_REALMS_AD_EXTERNAL + "2");
builder.copy(newName, name, secondarySettings);
final String newname;
if (name.contains(LdapRealmSettings.AD_TYPE)) {
newname = name.replace(XPACK_SECURITY_AUTHC_REALMS_AD_EXTERNAL, XPACK_SECURITY_AUTHC_REALMS_AD_EXTERNAL + "2");
} else {
newname = name.replace(XPACK_SECURITY_AUTHC_REALMS_LDAP_EXTERNAL, XPACK_SECURITY_AUTHC_REALMS_LDAP_EXTERNAL + "2");
}
builder.copy(newname, name, secondarySettings);

This comment has been minimized.

Sign in to view

Copy link
@albertzaharovits

albertzaharovits Oct 2, 2019

Contributor

Do I understand it correctly that this is the reason #47266 has been reverted ? Otherwise can you please explain why the CI failed to catch it.

This comment has been minimized.

Sign in to view

Copy link
@jkakavas

jkakavas Oct 2, 2019

Author Member

Do I understand it correctly that this is the reason #47266 has been reverted ?

Yes. CI didn't catch it because of the randomness in setupSecondaryRealm(). This would work fine if the realm we tried to duplicate is and active directory one, because it would change its name from external to external2 but if it was an ldap one, it wold fail as the replace would do nothing. Makes sense ?

This comment has been minimized.

Sign in to view

Copy link
@albertzaharovits

albertzaharovits Oct 2, 2019

Contributor

Yes, I wanted to make sure.
});

return builder.build();
Expand Down

0 comments on commit 3bc5d90

Please sign in to comment.