[Kerberos] Rest client integration test#32070
[Kerberos] Rest client integration test#32070bizybot merged 9 commits intoelastic:feature/kerberosfrom
Conversation
This commit adds the rest client integration test for Kerberos. This uses existing krb5kdc-fixture, which makes use of MIT Kerberos. Added support to create principals with password in krb5kdc-fixture. The rest test demonstrates the following: - Use of rest client to invoke Elasticsearch APIs authenticating using spnego mechanism, example showing what customizations we need to do to build the rest client. - test for login by keytab for user principal - test for login by username password for user principal
bizybot
added
>feature
:Security/Authentication
|
Pinging @elastic/es-security |
|
@jkakavas Can you have a look at |
tvernum
left a comment
tvernum
left a comment
There was a problem hiding this comment.
LGTM, with a few minor nits
| .field("enabled", true).startObject("rules").startArray("all").startObject().startObject("field") | ||
| .field("realm.name", TEST_KERBEROS_REALM_NAME).endObject().endObject().endArray() // "all" | ||
| .endObject() // "rules" | ||
| .endObject()); |
There was a problem hiding this comment.
I suspect you copied this from code I wrote, but can we lay this out a bit more cleanly? Like one field per line, or something.
There was a problem hiding this comment.
Yes, you are right, I guess it was code formatter. Thank you.
| private static final String TEST_USER_WITH_KEYTAB_KEY = "test.userkt"; | ||
| private static final String TEST_USER_WITH_KEYTAB_PATH_KEY = "test.userkt.keytab"; | ||
| private static final String TEST_USER_WITH_PWD_KEY = "test.userpwd"; | ||
| private static final String TEST_USER_WTIH_PWD_PASSWD_KEY = "test.userpwd.password"; |
| @Before | ||
| public void setupRoleMapping() throws IOException { | ||
| final String json = Strings // top-level | ||
| .toString(XContentBuilder.builder(XContentType.JSON.xContent()).startObject().array("roles", new String[] { "super_user" }) |
There was a problem hiding this comment.
The builtin role is superuser, no _.
If you're not trying to use that role, then just pick something more obvious like "kerb_test"
There was a problem hiding this comment.
I did not realize it and it did not fail for an invalid role. I have updated to use kerb_test role for testing. Thank you.
| integTestCluster { | ||
| setting 'xpack.license.self_generated.type', 'trial' | ||
| setting 'xpack.security.enabled', 'true' | ||
| setting 'xpack.security.http.ssl.enabled', 'false' |
There was a problem hiding this comment.
I do not think we need this setting
| setting 'xpack.security.authc.realms.kerberos.remove_realm_name', 'false' | ||
|
|
||
| Path krb5conf = project(':test:fixtures:krb5kdc-fixture').buildDir.toPath().resolve("conf").resolve("krb5.conf").toAbsolutePath() | ||
| String jvmArgsStr = " -Djava.security.krb5.conf=${krb5conf}" + " -Dsun.security.krb5.debug=true" |
There was a problem hiding this comment.
why do we specify debug in the realm and in the jvm args? Is it because our debug value overrides the system property? If so, we need to change the default of the setting to be the value of the system property.
There was a problem hiding this comment.
There are two flags for debug level logs, -Djava.security.krb5.conf enables debug logs at the Kerberos protocol level. The flag that we take in realm config is only for Krb5LoginModule from JAAS. I will add this information to the debugging and troubleshooting documentation. Thank you.
| systemProperty 'test.userkt.keytab', "${peppaKeytab}" | ||
| systemProperty 'test.userpwd', "george@${realm}" | ||
| systemProperty 'test.userpwd.password', "dino" | ||
| systemProperty 'tests.security.manager', 'false' |
There was a problem hiding this comment.
can you add a comment on why we need to run without the security manager in these tests?
There was a problem hiding this comment.
I was lazy and did want to keep the client simple. But I think it's better to run with the security manager. I have enabled it and also made some changes to make the tests run with the security manager. Thank you.
| * {@link HttpAsyncClientBuilder}.<br> | ||
| * It uses configured credentials either password or keytab for authentication. | ||
| */ | ||
| public class CustomHttpClientConfigCallbackHandler implements HttpClientConfigCallback { |
There was a problem hiding this comment.
I think we should rename this to SpnegoClientConfigCallbackHandler as custom doesn't provide much meaning
There was a problem hiding this comment.
Yes, done. Thank you.
| return httpClientBuilder; | ||
| } | ||
|
|
||
| private void setupSSLSettings(HttpAsyncClientBuilder httpClientBuilder) { |
There was a problem hiding this comment.
ssl is disabled in the test for http. why are we setting this up?
There was a problem hiding this comment.
I initially started to configure SSL for node but later on, decided not to as it was adding more configuration to the tests. I will remove this. Thanks.
|
|
||
| private abstract static class AbstractJaasConf extends Configuration { | ||
| private final String userPrincipalName; | ||
| private final Boolean enableDebugLogs; |
There was a problem hiding this comment.
why is this a Boolean and not a boolean? This could allow someone to pass in null which we do not want
| private final String userPrincipalName; | ||
| private final SecureString password; | ||
| private final String keytabPath; | ||
| private final Boolean enableDebugLogs; |
There was a problem hiding this comment.
do we really need a tri-state boolean here?
There was a problem hiding this comment.
No, we don't need it, corrected this. Thanks.
| * @return {@link LoginContext} | ||
| * @throws LoginException | ||
| */ | ||
| public LoginContext login() throws LoginException { |
There was a problem hiding this comment.
can you make this synchronized? There is no concurrent caller but I don't like having a public API like this since someone could call it concurrently
| } | ||
|
|
||
| private Response execute(final RestClient restClient, final Request request) throws IOException { | ||
| return restClient.performRequest(request); |
There was a problem hiding this comment.
do we really need this method? I think restClient.performRequest is readable
There was a problem hiding this comment.
I think I made this change to make it work as access control context was not available but found a method to pass on current threads access control context. Thank you for noticing this.
- Enable security manager for tests and related modifications to give permissions - Rename HttpClientConfigCallbackHandler - Remove unwanted code.
|
Hi, @jaymode I have addressed your review comments. Please take a look when you get some time. Thank you for reviewing it. |
jaymode
left a comment
jaymode
left a comment
There was a problem hiding this comment.
I left one comment about the file permissions in the test. Otherwise LGTM
| @@ -0,0 +1,5 @@ | |||
| grant { | |||
| permission javax.security.auth.AuthPermission "doAsPrivileged"; | |||
| permission java.io.FilePermission "${test.userkt.keytab}", "read"; | |||
There was a problem hiding this comment.
we should be able to remove this permission if we add the keytab as a resource within gradle?
There was a problem hiding this comment.
Yes, it worked. I have done the changes. Thanks.
4 participants
This commit adds the rest client integration test for Kerberos.
This uses existing krb5kdc-fixture, which makes use of MIT Kerberos.
Added support to create principals with password in krb5kdc-fixture.
The rest test demonstrates the following:
using spnego mechanism, example showing what customizations we
need to do to build the rest client.