Skip to content
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 typed security configuration #15651

Merged
merged 4 commits into from Oct 8, 2019
Merged

Conversation

@kwart
Copy link
Contributor

kwart commented Sep 30, 2019

EE part: hazelcast/hazelcast-enterprise#3193

Group password removed and authentication and identity representation configuration moved to a new security config type: RealmConfig

<security>
    <realms>
        <realm name="default">
            <authentication>
                <jaas>
                    <login-module class-name="com.hazelcast.examples.MyRequiredLoginModule" usage="REQUIRED"/>
                </jaas>
<!--
                <tls roleAttribute="cn" />
                <ldap>
                    <url>ldap://server.example.com</url>
                </ldap>
-->             
            </authentication>
            <identity>
                <username-password username="hazelcast" password="Z7vn4vNU9qL7H5xM"/>
<!--
                <credentialsFactory></credentialsFactory>
                <token>token_value</token>
-->
            </identity>
        </realm>
    </realms>
    <member-authentication realm="default"/>
    <client-authentication realm="default"/>
</security>

The realm keeps security-related configuration for one area (e.g. authentication for one Hazelcast protocol). Member and client protocols are mapped to the realms in the first implementation. The plan is to map also the management REST endpoints and data REST endpoints.

Realms can be also used for defining additional identities in the future. For instance a Kerberos identity of the member.

Client name configuration

As the cluster-name is not fitting well into the client configuration, this PR is renaming it to client-name. It's a name under which the client authenticates to the Hazelcast cluster. The default implementation compares it to the cluster name, but there can be other implementation employed and then the cluster-name is misleading.

Reverting group-name in Azure configuration

This PR reverts a small part of PR #15540. There is a configuration related to Azure resource group name within Azure discovery config. This was accidentally renamed from group-name (referencing Azure resource group) to a cluster-name (referencing Hazelcast cluster).

@kwart kwart added this to the 4.0 milestone Sep 30, 2019
@kwart kwart requested a review from hazelcast/clients as a code owner Sep 30, 2019
@kwart kwart self-assigned this Sep 30, 2019
@kwart kwart force-pushed the kwart:security-typed-config branch from 3c956ed to e9dff27 Oct 1, 2019
@petrpleshachkov petrpleshachkov self-requested a review Oct 1, 2019
@kwart kwart force-pushed the kwart:security-typed-config branch from e9dff27 to da160b4 Oct 1, 2019
@@ -118,7 +118,7 @@ public void smokeAzureConfig() {
assertEquals("TENANT_ID", azure.getProperty("tenant-id"));
assertEquals("SUB_ID", azure.getProperty("subscription-id"));
assertEquals("HZLCAST001", azure.getProperty("cluster-id"));
assertEquals("CLUSTER-NAME", azure.getProperty("cluster-name"));
assertEquals("RESOURCE-GROUP-NAME", azure.getProperty("group-name"));

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 1, 2019

Contributor

Why would we reintroduce group-name here? It may cause more confusion. Maybe to use RESOURCE_CLUSTER_NAME?

This comment has been minimized.

Copy link
@kwart

kwart Oct 1, 2019

Author Contributor

The Azure group-name is not Hazelcast cluster name, but Azure resource group name.

azure

@@ -827,7 +825,7 @@ private void assertAzureConfig(AzureConfig azure) {
assertEquals("TENANT_ID", azure.getProperty("tenant-id"));
assertEquals("SUB_ID", azure.getProperty("subscription-id"));
assertEquals("HZLCAST001", azure.getProperty("cluster-id"));
assertEquals("CLUSTER-NAME", azure.getProperty("cluster-name"));
assertEquals("RESOURCE-GROUP-NAME", azure.getProperty("group-name"));

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 1, 2019

Contributor

The same.

This comment has been minimized.

Copy link
@kwart

kwart Oct 4, 2019

Author Contributor

The Azure group-name is not our Hazelcast cluster name, but it's Azure own structure named "resource group name".

@@ -76,7 +74,7 @@
tenant-id="TENANT_ID"
subscription-id="SUB_ID"
cluster-id="HZLCAST001"
cluster-name="CLUSTER-NAME"/>
group-name="RESOURCE-GROUP-NAME"/>

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 1, 2019

Contributor

Why not to keep cluster-name here?

This comment has been minimized.

Copy link
@kwart

kwart Oct 4, 2019

Author Contributor

The Azure group-name is not our Hazelcast cluster name, but it's Azure own structure named "resource group name".

@@ -801,6 +729,15 @@ public ClientConfig setInstanceName(String instanceName) {
return this;
}

public String getClientName() {

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 1, 2019

Contributor

Javadoc would be helpful.

This comment has been minimized.

Copy link
@kwart

kwart Oct 4, 2019

Author Contributor

👍 will add

.close();
gen.open(tag);
for (LoginModuleConfig lm : loginModuleConfigs) {
List<String> attrs = new ArrayList<String>();

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 1, 2019

Contributor

new ArrayList<> ?

This comment has been minimized.

Copy link
@kwart

kwart Oct 4, 2019

Author Contributor

👍 will fix

@@ -115,4 +108,21 @@ public int hashCode() {
result = 31 * result + (properties != null ? properties.hashCode() : 0);
return result;
}

@Override
public ICredentialsFactory asCredentialsFactory(ClassLoader cl) {

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 1, 2019

Contributor

Can this method been called from multiple threads concurrently?

This comment has been minimized.

Copy link
@kwart

kwart Oct 5, 2019

Author Contributor

It probably can. 👍
I'll make the fields volatile.

}

private void setIfConfigured(Properties props, String propertyName, String value) {
if (!isNullOrEmpty(value)) {

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 1, 2019

Contributor

Is there a real scenario when the value is null?

This comment has been minimized.

Copy link
@kwart

kwart Oct 5, 2019

Author Contributor

Yes, most of the properties in the login module is optional.

@@ -342,7 +338,7 @@
<tenant-id>TENANT_ID</tenant-id>
<subscription-id>SUB_ID</subscription-id>
<cluster-id>HZLCAST001</cluster-id>
<cluster-name>CLUSTER-NAME</cluster-name>
<group-name>RESOURCE-GROUP-NAME</group-name>

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 1, 2019

Contributor

Why not to keep cluster-name?

This comment has been minimized.

Copy link
@kwart

kwart Oct 5, 2019

Author Contributor

Azure resource.

&& isCompatible(c1.getLdapAuthenticationConfig(), c2.getLdapAuthenticationConfig())
&& isCompatible(c1.getUsernamePasswordIdentityConfig(), c2.getUsernamePasswordIdentityConfig())
&& isCompatible(c1.getTokenIdentityConfig(), c2.getTokenIdentityConfig())
;

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 1, 2019

Contributor

Minor: move comma one line up? :-)

This comment has been minimized.

Copy link
@kwart

kwart Oct 5, 2019

Author Contributor

matter of taste :)

@vojtechtoman vojtechtoman self-requested a review Oct 3, 2019
identityConfig = identityType.getConstructor(identityType).newInstance(securityConfig.identityConfig);
} catch (InstantiationException | IllegalAccessException | IllegalArgumentException | InvocationTargetException
| NoSuchMethodException | SecurityException e) {
identityConfig = securityConfig.identityConfig;
Comment on lines 45 to 48

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Oct 3, 2019

Contributor

Minor: have you considered introducing a simple copy method or perhaps implementing Cloneable? (But of course, the above code is a thing of beauty.)

This comment has been minimized.

Copy link
@kwart

kwart Oct 5, 2019

Author Contributor

Good point. I'll use a copy() method.

credentialsFactory.configure(cs -> {
});
Comment on lines 91 to 95

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Oct 3, 2019

Contributor

Is this necessary? Is the idea to maybe reset the default behavior or just to make sure the configure gets called (because the impl may depend on that)?

This comment has been minimized.

Copy link
@kwart

kwart Oct 5, 2019

Author Contributor

The configure() should be always called for factory implementation. There is no specific callback supported currently on the client-side so such an empty handler should be OK, but probably throwing the UnsupportedCallbackException will work better here (instead of the empty statement).

# * "credentials-factory":
# Specifies the name and properties of your class that you developed by implementing Hazelcast's Credentials
# interface.
# This element has a mandatory "class-name" attribute where you should define the factory class implementing
# ICredentialsFactory used to create Credentials objects.
# With the "properties" sub-element, you can define properties for the factory class. The properties can be
# defined as a mapping where the key is the name of the property.
# * "username-password":

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Oct 3, 2019

Contributor

Documentation missing? Also, what about token?

This comment has been minimized.

Copy link
@kwart

kwart Oct 5, 2019

Author Contributor

👍

@@ -2130,22 +2124,6 @@
It has the following attributes and sub-elements:

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Oct 3, 2019

Contributor

The list of sub-elements hasn't been updated

This comment has been minimized.

Copy link
@kwart

kwart Oct 5, 2019

Author Contributor

👍

@@ -2128,18 +2128,6 @@ hazelcast:
# It has the following attributes and sub-elements:

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Oct 3, 2019

Contributor

The list of sub-elements hasn't been updated.

This comment has been minimized.

Copy link
@kwart

kwart Oct 5, 2019

Author Contributor

👍

return false;
}
for (String realmName: c1.keySet()) {
if (! isCompatible(c1.get(realmName), c2.get(realmName))) {

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Oct 3, 2019

Contributor

whitespace after !

This comment has been minimized.

Copy link
@kwart

kwart Oct 4, 2019

Author Contributor

Whitespace approved by our checkstyle rules is always a correct one. Isn't it? :)

for (Node child : childElements(node)) {
String nodeName = cleanNodeName(child);
if ("realm".equals(nodeName)) {
realms.put(getAttribute(child, "name"), handleRealm(child, securityConfigBuilder));

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Oct 3, 2019

Contributor

Wouldn't a bean reference be more Spring-y? (Not sure if it makes sense here, just asking because we seem to do that where possible.)

This comment has been minimized.

Copy link
@kwart

kwart Oct 5, 2019

Author Contributor

I would leave it this way, as it's not a top-level configuration, but part of the security configuration.

@kwart

This comment has been minimized.

Copy link
Contributor Author

kwart commented Oct 4, 2019

Thanks a lot for reviews, @vojtechtoman & @petrpleshachkov. I'll go through your comments and try to fix or explain.

@kwart kwart force-pushed the kwart:security-typed-config branch from da160b4 to 6cdff48 Oct 5, 2019
@kwart kwart force-pushed the kwart:security-typed-config branch from 6cdff48 to e02b97b Oct 5, 2019
@kwart kwart requested review from petrpleshachkov and vojtechtoman Oct 5, 2019
@kwart

This comment has been minimized.

Copy link
Contributor Author

kwart commented Oct 5, 2019

The comments should be now resolved or explained. Could you have a second look, please?

Copy link
Contributor

vojtechtoman left a comment

Looks good to me, only minor remarks.

@kwart kwart force-pushed the kwart:security-typed-config branch 2 times, most recently from 36f90be to 6af5716 Oct 5, 2019
@kwart kwart force-pushed the kwart:security-typed-config branch from 6af5716 to dd60774 Oct 5, 2019
} else {
try {
ICredentialsFactory credentialsFactory = ClassLoaderUtil.newInstance(cl, className);
credentialsFactory.init(properties);

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 7, 2019

Contributor

Is it expected that we will create credentialsFactory every time? Maybe we should cache it?

Copy link
Member

asimarslan left a comment

client only

@asimarslan asimarslan requested a review from gurbuzali Oct 8, 2019
@kwart kwart merged commit d2967c0 into hazelcast:master Oct 8, 2019
1 check failed
1 check failed
default Test `FAIL`ed. http://jenkins.hazelcast.com/job/Hazelcast-pr-builder/3671/
Details
@kwart kwart deleted the kwart:security-typed-config branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.