From de22a51b48ffeaede6b7c9556cf13b580840a0e2 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Thu, 7 Sep 2017 16:46:15 +0200 Subject: [PATCH] Polishing. Fetch SecretId if no secretId is configured but an initial token is provided instead of relying on a configured role name. Use configured AppRole mount path instead of static literal. Reorder methods, add since and author tags. Reduce tests to AppRoleAuthenticationOptions code. Add further test cases. Add integration tests. Formatting, fix typos. Original pull request: gh-133. Related ticket: gh-132. --- .../authentication/AppRoleAuthentication.java | 52 ++++---- .../AppRoleAuthenticationOptions.java | 116 ++++++++++-------- ...AppRoleAuthenticationIntegrationTests.java | 24 ++++ .../AppRoleAuthenticationUnitTests.java | 40 +++--- .../asciidoc/reference/authentication.adoc | 9 +- 5 files changed, 150 insertions(+), 91 deletions(-) diff --git a/spring-vault-core/src/main/java/org/springframework/vault/authentication/AppRoleAuthentication.java b/spring-vault-core/src/main/java/org/springframework/vault/authentication/AppRoleAuthentication.java index 77a1d084..1a54279e 100644 --- a/spring-vault-core/src/main/java/org/springframework/vault/authentication/AppRoleAuthentication.java +++ b/spring-vault-core/src/main/java/org/springframework/vault/authentication/AppRoleAuthentication.java @@ -17,8 +17,10 @@ package org.springframework.vault.authentication; import java.util.HashMap; import java.util.Map; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; + import org.springframework.http.HttpEntity; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; @@ -42,6 +44,7 @@ import org.springframework.web.client.RestOperations; * {@link AppRoleAuthenticationOptions#getSecretId()}. * * @author Mark Paluch + * @author Vincent Le Nair * @see AppRoleAuthenticationOptions * @see RestOperations * @see Auth Backend: @@ -91,7 +94,8 @@ public class AppRoleAuthentication implements ClientAuthentication, .login("auth/{mount}/login", options.getPath()); } - @Override public VaultToken login() { + @Override + public VaultToken login() { return createTokenUsingAppRole(); } @@ -108,9 +112,8 @@ public class AppRoleAuthentication implements ClientAuthentication, Map login = getAppRoleLogin(roleId, secretId); try { - VaultResponse response = restOperations - .postForObject("auth/{mount}/login", login, VaultResponse.class, - options.getPath()); + VaultResponse response = restOperations.postForObject("auth/{mount}/login", + login, VaultResponse.class, options.getPath()); Assert.state(response != null && response.getAuth() != null, "Auth field must not be null"); @@ -121,55 +124,59 @@ public class AppRoleAuthentication implements ClientAuthentication, } catch (HttpStatusCodeException e) { throw new VaultException(String.format("Cannot login using AppRole: %s", - VaultResponses.getError(e.getResponseBodyAsString()))); + VaultResponses.getError(e.getResponseBodyAsString()))); } } private String getRoleId() { + String roleId = options.getRoleId(); - if (StringUtils.isEmpty(roleId) && !StringUtils.isEmpty(options.getAppRole())) { + + if (StringUtils.isEmpty(roleId)) { + try { - ResponseEntity response = restOperations - .exchange("auth/approle/role/{role}/role-id", HttpMethod.GET, + ResponseEntity response = restOperations.exchange( + "auth/{mount}/role/{role}/role-id", HttpMethod.GET, createHttpEntityWithToken(), VaultResponse.class, - options.getAppRole()); + options.getPath(), options.getAppRole()); roleId = (String) response.getBody().getData().get("role_id"); } catch (HttpStatusCodeException e) { - throw new VaultException(String - .format("Cannot get Role id using AppRole: %s", + throw new VaultException(String.format( + "Cannot get Role id using AppRole: %s", VaultResponses.getError(e.getResponseBodyAsString()))); } } - return roleId; + return roleId; } private String getSecretId() { + String secretId = options.getSecretId(); - if (StringUtils.isEmpty(secretId) && !StringUtils.isEmpty(options.getAppRole())) { + + if (StringUtils.isEmpty(secretId) && options.getInitialToken() != null) { try { - VaultResponse response = restOperations - .postForObject("auth/approle/role/{role}/secret-id", + VaultResponse response = restOperations.postForObject( + "auth/{mount}/role/{role}/secret-id", createHttpEntityWithToken(), VaultResponse.class, - options.getAppRole()); + options.getPath(), options.getAppRole()); secretId = (String) response.getData().get("secret_id"); } catch (HttpStatusCodeException e) { - throw new VaultException(String - .format("Cannot get Secret id using AppRole: %s", + throw new VaultException(String.format( + "Cannot get Secret id using AppRole: %s", VaultResponses.getError(e.getResponseBodyAsString()))); } } - return secretId; + return secretId; } private HttpEntity createHttpEntityWithToken() { + HttpHeaders headers = new HttpHeaders(); - if (options.getInitialToken() != null) { - headers.set("X-Vault-Token", options.getInitialToken()); - } + headers.set("X-Vault-Token", options.getInitialToken().getToken()); return new HttpEntity(null, headers); } @@ -179,6 +186,7 @@ public class AppRoleAuthentication implements ClientAuthentication, Map login = new HashMap<>(); login.put("role_id", roleId); + if (secretId != null) { login.put("secret_id", secretId); } diff --git a/spring-vault-core/src/main/java/org/springframework/vault/authentication/AppRoleAuthenticationOptions.java b/spring-vault-core/src/main/java/org/springframework/vault/authentication/AppRoleAuthenticationOptions.java index 49be9170..61bceeff 100644 --- a/spring-vault-core/src/main/java/org/springframework/vault/authentication/AppRoleAuthenticationOptions.java +++ b/spring-vault-core/src/main/java/org/springframework/vault/authentication/AppRoleAuthenticationOptions.java @@ -18,6 +18,7 @@ package org.springframework.vault.authentication; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.StringUtils; +import org.springframework.vault.support.VaultToken; /** * Authentication options for {@link AppRoleAuthentication}. @@ -27,6 +28,7 @@ import org.springframework.util.StringUtils; * this class are immutable once constructed. * * @author Mark Paluch + * @author Vincent Le Nair * @see AppRoleAuthentication * @see #builder() */ @@ -35,13 +37,14 @@ public class AppRoleAuthenticationOptions { public static final String DEFAULT_APPROLE_AUTHENTICATION_PATH = "approle"; /** - * Path of the apprile authentication backend mount. + * Path of the approle authentication backend mount. */ private final String path; /** * The RoleId. */ + @Nullable private final String roleId; /** @@ -53,15 +56,18 @@ public class AppRoleAuthenticationOptions { /** * Role name used to get roleId and secretID */ + @Nullable private final String appRole; /** - * Token associated to the roleName. + * Token associated for pull mode (retrieval of secretId/roleId). */ - private final String initialToken; + @Nullable + private final VaultToken initialToken; - private AppRoleAuthenticationOptions(String path, String roleId, - @Nullable String secretId, String appRole, String initialToken) { + private AppRoleAuthenticationOptions(String path, @Nullable String roleId, + @Nullable String secretId, @Nullable String appRole, + @Nullable VaultToken initialToken) { this.path = path; this.roleId = roleId; @@ -87,6 +93,7 @@ public class AppRoleAuthenticationOptions { /** * @return the RoleId. */ + @Nullable public String getRoleId() { return roleId; } @@ -101,15 +108,19 @@ public class AppRoleAuthenticationOptions { /** * @return the bound AppRole. + * @since 1.1 */ + @Nullable public String getAppRole() { return appRole; } /** - * @return the bound InitialToken. + * @return the initial token for roleId/secretId retrieval in pull mode. + * @since 1.1 */ - public String getInitialToken() { + @Nullable + public VaultToken getInitialToken() { return initialToken; } @@ -120,16 +131,16 @@ public class AppRoleAuthenticationOptions { private String path = DEFAULT_APPROLE_AUTHENTICATION_PATH; - private String appRole; - - private String initialToken; - @Nullable private String roleId; @Nullable private String secretId; + private String appRole; + + private VaultToken initialToken; + AppRoleAuthenticationOptionsBuilder() { } @@ -148,34 +159,6 @@ public class AppRoleAuthenticationOptions { return this; } - /** - * Configure a {@code appRole}. - * - * @param appRole must not be empty or {@literal null}. - * @return {@code this} {@link AppRoleAuthenticationOptionsBuilder}. - */ - public AppRoleAuthenticationOptionsBuilder appRole(String appRole) { - - Assert.hasText(appRole, "AppRole must not be empty"); - - this.appRole = appRole; - return this; - } - - /** - * Configure a {@code initialToken}. - * - * @param initialToken must not be empty or {@literal null}. - * @return {@code this} {@link AppRoleAuthenticationOptionsBuilder}. - */ - public AppRoleAuthenticationOptionsBuilder initialToken(String initialToken) { - - Assert.hasText(initialToken, "InitialToken must not be empty"); - - this.initialToken = initialToken; - return this; - } - /** * Configure the RoleId. * @@ -204,10 +187,40 @@ public class AppRoleAuthenticationOptions { return this; } + /** + * Configure a {@code appRole}. + * + * @param appRole must not be empty or {@literal null}. + * @return {@code this} {@link AppRoleAuthenticationOptionsBuilder}. + * @since 1.1 + */ + public AppRoleAuthenticationOptionsBuilder appRole(String appRole) { + + Assert.hasText(appRole, "AppRole must not be empty"); + + this.appRole = appRole; + return this; + } + + /** + * Configure a {@code initialToken}. + * + * @param initialToken must not be empty or {@literal null}. + * @return {@code this} {@link AppRoleAuthenticationOptionsBuilder}. + * @since 1.1 + */ + public AppRoleAuthenticationOptionsBuilder initialToken(VaultToken initialToken) { + + Assert.notNull(initialToken, "InitialToken must not be null"); + + this.initialToken = initialToken; + return this; + } + /** * Build a new {@link AppRoleAuthenticationOptions} instance. Requires - * {@link #roleId(String)} for Push Mode or {@link #appRole(String)} and - * {@link #initialToken(String)} for pull Mode to be configured. + * {@link #roleId(String)} for push mode or {@link #appRole(String)} and + * {@link #initialToken(VaultToken)} for pull mode to be configured. * * @return a new {@link AppRoleAuthenticationOptions}. */ @@ -215,19 +228,26 @@ public class AppRoleAuthenticationOptions { Assert.hasText(path, "Path must not be empty"); - //Role ID is required in order to use push mode (no appRole and initialToken) - if (StringUtils.isEmpty(appRole) && StringUtils.isEmpty(initialToken)) { - Assert.notNull(roleId, "RoleId must not be null"); + // Role ID is required in order to use push mode (no appRole and initialToken) + + if (StringUtils.isEmpty(roleId) && StringUtils.isEmpty(appRole) + && initialToken == null) { + throw new IllegalArgumentException( + "Either roleId (push mode) or appRole/initialToken (pull mode) must be configured for AppRole authentication"); } - //AppRole and InitialToken are required in order to use pull mode (no roleId) + // AppRole and InitialToken are required in order to use pull mode (no roleId) if (StringUtils.isEmpty(roleId)) { - Assert.notNull(appRole, "AppRole must not be null"); - Assert.notNull(initialToken, "InitialToken must not be null"); + + Assert.notNull(appRole, + "AppRole authentication configured for pull mode. AppRole must not be null."); + Assert.notNull( + initialToken, + "AppRole authentication configured for pull mode. InitialToken must not be null (pull mode)"); } return new AppRoleAuthenticationOptions(path, roleId, secretId, appRole, - initialToken); + initialToken); } } } diff --git a/spring-vault-core/src/test/java/org/springframework/vault/authentication/AppRoleAuthenticationIntegrationTests.java b/spring-vault-core/src/test/java/org/springframework/vault/authentication/AppRoleAuthenticationIntegrationTests.java index f77a0469..def0e690 100644 --- a/spring-vault-core/src/test/java/org/springframework/vault/authentication/AppRoleAuthenticationIntegrationTests.java +++ b/spring-vault-core/src/test/java/org/springframework/vault/authentication/AppRoleAuthenticationIntegrationTests.java @@ -26,6 +26,7 @@ import org.springframework.vault.VaultException; import org.springframework.vault.core.VaultOperations; import org.springframework.vault.support.VaultResponse; import org.springframework.vault.util.IntegrationTestSupport; +import org.springframework.vault.util.Settings; import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.CoreMatchers.anyOf; @@ -88,6 +89,29 @@ public class AppRoleAuthenticationIntegrationTests extends IntegrationTestSuppor assertThat(authentication.login()).isNotNull(); } + @Test + public void shouldAuthenticateWithFullPullMode() { + + AppRoleAuthenticationOptions options = AppRoleAuthenticationOptions.builder() + .appRole("with-secret-id").initialToken(Settings.token()).build(); + AppRoleAuthentication authentication = new AppRoleAuthentication(options, + prepare().getRestTemplate()); + + assertThat(authentication.login()).isNotNull(); + } + + @Test + public void shouldAuthenticateWithPullMode() { + + AppRoleAuthenticationOptions options = AppRoleAuthenticationOptions.builder() + .roleId(getRoleId("with-secret-id")).appRole("with-secret-id") + .initialToken(Settings.token()).build(); + AppRoleAuthentication authentication = new AppRoleAuthentication(options, + prepare().getRestTemplate()); + + assertThat(authentication.login()).isNotNull(); + } + @Test public void shouldAuthenticatePullModeWithGeneratedSecretId() { diff --git a/spring-vault-core/src/test/java/org/springframework/vault/authentication/AppRoleAuthenticationUnitTests.java b/spring-vault-core/src/test/java/org/springframework/vault/authentication/AppRoleAuthenticationUnitTests.java index 8de25f84..ae785993 100644 --- a/spring-vault-core/src/test/java/org/springframework/vault/authentication/AppRoleAuthenticationUnitTests.java +++ b/spring-vault-core/src/test/java/org/springframework/vault/authentication/AppRoleAuthenticationUnitTests.java @@ -30,7 +30,10 @@ import org.springframework.vault.support.VaultToken; import org.springframework.web.client.RestTemplate; import static org.assertj.core.api.Assertions.assertThat; -import static org.springframework.test.web.client.match.MockRestRequestMatchers.*; +import static org.springframework.test.web.client.match.MockRestRequestMatchers.header; +import static org.springframework.test.web.client.match.MockRestRequestMatchers.jsonPath; +import static org.springframework.test.web.client.match.MockRestRequestMatchers.method; +import static org.springframework.test.web.client.match.MockRestRequestMatchers.requestTo; import static org.springframework.test.web.client.response.MockRestResponseCreators.withServerError; import static org.springframework.test.web.client.response.MockRestResponseCreators.withSuccess; @@ -38,6 +41,7 @@ import static org.springframework.test.web.client.response.MockRestResponseCreat * Unit tests for {@link AppRoleAuthentication}. * * @author Mark Paluch + * @author Vincent Le Nair */ public class AppRoleAuthenticationUnitTests { @@ -82,23 +86,21 @@ public class AppRoleAuthenticationUnitTests { public void loginShouldPullRoleIdAndSecretId() throws Exception { AppRoleAuthenticationOptions options = AppRoleAuthenticationOptions.builder() - .appRole("app_role") - .initialToken("initial_token") - .build(); + .appRole("app_role").initialToken(VaultToken.of("initial_token")).build(); mockRest.expect(requestTo("/auth/approle/role/app_role/role-id")) .andExpect(method(HttpMethod.GET)) .andExpect(header("X-Vault-token", "initial_token")) - .andRespond(withSuccess().contentType(MediaType.APPLICATION_JSON).body( - "{\"data\": {\"role_id\": \"hello\"}}" - )); + .andRespond( + withSuccess().contentType(MediaType.APPLICATION_JSON).body( + "{\"data\": {\"role_id\": \"hello\"}}")); mockRest.expect(requestTo("/auth/approle/role/app_role/secret-id")) .andExpect(method(HttpMethod.POST)) .andExpect(header("X-Vault-token", "initial_token")) - .andRespond(withSuccess().contentType(MediaType.APPLICATION_JSON).body( - "{\"data\": {\"secret_id\": \"world\"}}" - )); + .andRespond( + withSuccess().contentType(MediaType.APPLICATION_JSON).body( + "{\"data\": {\"secret_id\": \"world\"}}")); mockRest.expect(requestTo("/auth/approle/login")) .andExpect(method(HttpMethod.POST)) @@ -117,16 +119,18 @@ public class AppRoleAuthenticationUnitTests { } @Test(expected = IllegalArgumentException.class) - public void loginShouldFailIfPullModeButNoToken() throws Exception { + public void optionsShouldRequireTokenOrRoleIdIfNothingIsSet() { + AppRoleAuthenticationOptions.builder().build(); + } - AppRoleAuthenticationOptions options = AppRoleAuthenticationOptions.builder() - .appRole("app_role") - .build(); - - AppRoleAuthentication sut = new AppRoleAuthentication(options, restTemplate); - - sut.login(); + @Test(expected = IllegalArgumentException.class) + public void optionsShouldRequireTokenOrRoleIdIfTokenIsSet() { + AppRoleAuthenticationOptions.builder().initialToken(VaultToken.of("foo")).build(); + } + @Test(expected = IllegalArgumentException.class) + public void optionsShouldRequireTokenOrRoleIdIfAppRoleIdIsSet() { + AppRoleAuthenticationOptions.builder().appRole("app_role").build(); } @Test diff --git a/src/main/asciidoc/reference/authentication.adoc b/src/main/asciidoc/reference/authentication.adoc index 450c5f26..74ec9072 100644 --- a/src/main/asciidoc/reference/authentication.adoc +++ b/src/main/asciidoc/reference/authentication.adoc @@ -196,7 +196,8 @@ authentication, like the deprecated (since Vault 0.6.1) <