From 7ffefd03586e8f2487d9a37a3ea4ecde6ac1e7c8 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 | 53 +++++---- .../AppRoleAuthenticationOptions.java | 108 ++++++++++-------- ...AppRoleAuthenticationIntegrationTests.java | 24 ++++ .../AppRoleAuthenticationUnitTests.java | 40 ++++--- .../asciidoc/reference/authentication.adoc | 9 +- 5 files changed, 144 insertions(+), 90 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 531596e6..47d97b9d 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; @@ -40,6 +42,7 @@ import org.springframework.web.client.RestOperations; * {@link AppRoleAuthenticationOptions#getSecretId()}. * * @author Mark Paluch + * @author Vincent Le Nair * @see AppRoleAuthenticationOptions * @see RestOperations * @see Auth Backend: @@ -70,7 +73,8 @@ public class AppRoleAuthentication implements ClientAuthentication { this.restOperations = restOperations; } - @Override public VaultToken login() { + @Override + public VaultToken login() { return createTokenUsingAppRole(); } @@ -82,9 +86,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()); logger.debug("Login successful using AppRole authentication"); @@ -92,55 +95,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); } @@ -148,9 +155,11 @@ public class AppRoleAuthentication implements ClientAuthentication { Map login = new HashMap(); login.put("role_id", roleId); + if (secretId != null) { login.put("secret_id", secretId); } + return login; } } 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 fef3d135..9cdde66f 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 @@ -17,6 +17,7 @@ package org.springframework.vault.authentication; import org.springframework.util.Assert; import org.springframework.util.StringUtils; +import org.springframework.vault.support.VaultToken; /** * Authentication options for {@link AppRoleAuthentication}. @@ -26,6 +27,7 @@ import org.springframework.util.StringUtils; * this class are immutable once constructed. * * @author Mark Paluch + * @author Vincent Le Nair * @see AppRoleAuthentication * @see #builder() */ @@ -34,7 +36,7 @@ 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; @@ -54,11 +56,12 @@ public class AppRoleAuthenticationOptions { private final String appRole; /** - * Token associated to the roleName. + * Token associated for pull mode (retrieval of secretId/roleId). */ - private final String initialToken; + private final VaultToken initialToken; - private AppRoleAuthenticationOptions(String path, String roleId, String secretId, String appRole, String initialToken) { + private AppRoleAuthenticationOptions(String path, String roleId, String secretId, + String appRole, VaultToken initialToken) { this.path = path; this.roleId = roleId; @@ -97,15 +100,17 @@ public class AppRoleAuthenticationOptions { /** * @return the bound AppRole. + * @since 1.1 */ 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() { + public VaultToken getInitialToken() { return initialToken; } @@ -116,14 +121,14 @@ public class AppRoleAuthenticationOptions { private String path = DEFAULT_APPROLE_AUTHENTICATION_PATH; - private String appRole; - - private String initialToken; - private String roleId; private String secretId; + private String appRole; + + private VaultToken initialToken; + AppRoleAuthenticationOptionsBuilder() { } @@ -142,34 +147,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. * @@ -198,10 +175,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}. */ @@ -209,19 +216,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 784d7bd5..17d83f09 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 @@ -27,6 +27,7 @@ import org.springframework.vault.core.RestOperationsCallback; 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 org.springframework.web.client.RestOperations; import static org.assertj.core.api.Assertions.assertThat; @@ -93,6 +94,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 640c3d6a..55d79656 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 @@ -28,7 +28,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; @@ -36,6 +39,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 { @@ -80,23 +84,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)) @@ -115,16 +117,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 2adc00b5..483210d2 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) <