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.

Sign up for GitHub

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

Jump to bottom

fix: (hashicorp-vault) token self-lookup path as segments #4512

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: (hashicorp-vault) token self-lookup path as segments #4512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fc43d08
#4508 interpret token self-lookup path as several segments, not a sin…
drcgjung Sep 30, 2024
38c5ba1
feature: introduce/clone integration test to run against hashicorps B…
drcgjung Oct 4, 2024
ce072cc
Merge remote-tracking branch 'origin/main' into fix/4508-hashicorp-va…
drcgjung Oct 4, 2024
44592f5
fix: stay with static container/test initialization not to risk multi…
drcgjung Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...-hashicorp/src/main/java/org/eclipse/edc/vault/hashicorp/client/HashicorpVaultClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public Result<Void> doHealthCheck() {
public Result<Boolean> isTokenRenewable() {
var uri = settings.url()
.newBuilder()
.addPathSegment(TOKEN_LOOK_UP_SELF_PATH)
.addPathSegments(TOKEN_LOOK_UP_SELF_PATH)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't there a way to justify this change through an automated test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched HashicorpVaultClientIntegrationTest from "vault/1.9.6" to "hashicorp/vault:1.17.3" (and higher) -> lookUpToken_whenTokenNotExpired_shouldSucceed will fail without this patch.

docker stopped to update "vault" from 1.14 on - the newer "vault" implementations all come from hashicorp as a verified publisher.

I know the license problem why many people froze the reference implementation, but its a kind of bug anway if the reference implementation interprets its own spec now more literally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drcgjung ok, but in the changelog there's no version update, please change the docker image version accordingly, there shouldn't be any licensing issue.

Copy link
Member

@paullatzelsperger paullatzelsperger Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a bit of context: HashiCorp changed their licensing in August 2023, mainly to prevent companies from offering competing services that are based on Vault (and/or other HashiCorp offerings).

Since the EDC project neither sells nor embeds HashiCorp community software, nor does it offer a product competitive to a HashiCorp product, I think we are in the clear.

However, if there is a breaking API change between the freely available version (1.9.6) and the "commercial" version (1.17.3), that cannot be solved with a separate code path in the vault client, then this becomes a topic for the EDC Technical Committee.

Generally, we should always use latest software, but we cannot require the use of non-OSS software. there always has to be an OSS alternative available.

@ndr-brt there might not be a licensing issue for EDC, but there could be awkward situations when our HCV impl. requires the use of a non-free version of HCV.

@drcgjung Are you saying that there is a breaking API change between 1.9.6 and 1.17.3?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paullatzelsperger we already have libraries that deal with proprietary software (see all the technology repos), we are not distributing hashicorp code.
License wise I see no issue here.

If a new version brought a breaking change I think we should provide either two versions of the module or a config to being able to make the EDC being able to interact with both "pre" and "post" breaking change, until the "pre" will get out of support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drcgjung Are you saying that there is a breaking API change between 1.9.6 and 1.17.3?

No, I am saying that AFAICS from version 1.17.3 on, the vault API does not tolerate/interpret url-encoded slashes in path prefixes as normal slashes anymore (which is IMHO quite a difference from the W3C spec perspective). So the older versions seems to have been more "tolerant" wrt the interpretation of the same API.

My patch works with ALL versions, only the integration test currently needs to select a single reference - maybe it could be subclassed/doubled to test integration with both versions?

Copy link
Member

@paullatzelsperger paullatzelsperger Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndr-brt my point exactly. We (=EDC) are in the clear, legally. My point is that we can't really recommend publicly to use vault 1.17.3, because that is not strictly OSS anymore and that would be a problem from an EF perspective I think. Yes, we have implementations for other non-free software, but there always has to be an OSS alternative.

From what I gather there is no breaking change now, so we don't need to duplicate or config anything.

Just to be sure and to guard against regression, a good middle ground would be to test against 1.17.3 AND the last-known-free version 1.9.6, as @drcgjung suggested. That way we can guarantee that our code works with "an open-source variant".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure and to guard against regression, a good middle ground would be to test against 1.17.3 AND the last-known-free version 1.9.6, as @drcgjung suggested. That way we can guarantee that our code works with "an open-source variant".

definitely agree

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure and to guard against regression, a good middle ground would be to test against 1.17.3 AND the last-known-free version 1.9.6, as @drcgjung suggested. That way we can guarantee that our code works with "an open-source variant".

definitely agree

second integration test implemented.

.build();
var request = httpGet(uri);

Expand Down
237 changes: 149 additions & 88 deletions ...test/java/org/eclipse/edc/vault/hashicorp/client/HashicorpVaultClientIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.eclipse.edc.junit.annotations.ComponentTest;
import org.eclipse.edc.spi.monitor.ConsoleMonitor;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.testcontainers.junit.jupiter.Container;
import org.testcontainers.junit.jupiter.Testcontainers;
Expand All @@ -35,98 +36,158 @@
import static org.eclipse.edc.junit.assertions.AbstractResultAssert.assertThat;
import static org.testcontainers.shaded.org.awaitility.Awaitility.await;

@ComponentTest
@Testcontainers
class HashicorpVaultClientIntegrationTest {
@Container
static final VaultContainer<?> VAULT_CONTAINER = new VaultContainer<>("vault:1.9.6")
.withVaultToken(UUID.randomUUID().toString());

private static final String HTTP_URL_FORMAT = "http://%s:%s";
private static final String HEALTH_CHECK_PATH = "/health/path";
private static final String CLIENT_TOKEN_KEY = "client_token";
private static final String AUTH_KEY = "auth";
private static final long CREATION_TTL = 6L;
private static final long TTL = 5L;
private static final long RENEW_BUFFER = 4L;
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ConsoleMonitor MONITOR = new ConsoleMonitor();

private static HashicorpVaultClient client;

@BeforeEach
void beforeEach() throws IOException, InterruptedException {
assertThat(CREATION_TTL).isGreaterThan(TTL);
client = new HashicorpVaultClient(
testHttpClient(),
OBJECT_MAPPER,
MONITOR,
getSettings()
);
}

@Test
void lookUpToken_whenTokenNotExpired_shouldSucceed() {
var tokenLookUpResult = client.isTokenRenewable();

assertThat(tokenLookUpResult).isSucceeded().isEqualTo(true);
}

@Test
void lookUpToken_whenTokenExpired_shouldFail() {
await()
.pollDelay(CREATION_TTL, TimeUnit.SECONDS)
.atMost(CREATION_TTL + 1, TimeUnit.SECONDS)
.untilAsserted(() -> {
var tokenLookUpResult = client.isTokenRenewable();
assertThat(tokenLookUpResult).isFailed();
assertThat(tokenLookUpResult.getFailureDetail()).isEqualTo("Token look up failed with status 403");
});
}

@Test
void renewToken_whenTokenNotExpired_shouldSucceed() {
var tokenRenewResult = client.renewToken();
class HashicorpVaultClientIntegrationTest {

assertThat(tokenRenewResult).isSucceeded().satisfies(ttl -> assertThat(ttl).isEqualTo(TTL));
@ComponentTest
@Testcontainers
@Nested
abstract static class Tests {

protected static final String HTTP_URL_FORMAT = "http://%s:%s";
protected static final String HEALTH_CHECK_PATH = "/health/path";
protected static final String CLIENT_TOKEN_KEY = "client_token";
protected static final String AUTH_KEY = "auth";
protected static final long CREATION_TTL = 6L;
protected static final long TTL = 5L;
protected static final long RENEW_BUFFER = 4L;
protected HashicorpVaultClient client;
protected final ObjectMapper mapper = new ObjectMapper();
protected final ConsoleMonitor monitor = new ConsoleMonitor();

@BeforeEach
void beforeEach() throws IOException, InterruptedException {
assertThat(CREATION_TTL).isGreaterThan(TTL);
}

@Test
void lookUpToken_whenTokenNotExpired_shouldSucceed() {
var tokenLookUpResult = client.isTokenRenewable();

assertThat(tokenLookUpResult).isSucceeded().isEqualTo(true);
}

@Test
void lookUpToken_whenTokenExpired_shouldFail() {
await()
.pollDelay(CREATION_TTL, TimeUnit.SECONDS)
.atMost(CREATION_TTL + 1, TimeUnit.SECONDS)
.untilAsserted(() -> {
var tokenLookUpResult = client.isTokenRenewable();
assertThat(tokenLookUpResult).isFailed();
assertThat(tokenLookUpResult.getFailureDetail()).isEqualTo("Token look up failed with status 403");
});
}

@Test
void renewToken_whenTokenNotExpired_shouldSucceed() {
var tokenRenewResult = client.renewToken();

assertThat(tokenRenewResult).isSucceeded().satisfies(ttl -> assertThat(ttl).isEqualTo(TTL));
}

@Test
void renewToken_whenTokenExpired_shouldFail() {
await()
.pollDelay(CREATION_TTL, TimeUnit.SECONDS)
.atMost(CREATION_TTL + 1, TimeUnit.SECONDS)
.untilAsserted(() -> {
var tokenRenewResult = client.renewToken();
assertThat(tokenRenewResult).isFailed();
assertThat(tokenRenewResult.getFailureDetail()).isEqualTo("Token renew failed with status: 403");
});
}
}

@Test
void renewToken_whenTokenExpired_shouldFail() {
await()
.pollDelay(CREATION_TTL, TimeUnit.SECONDS)
.atMost(CREATION_TTL + 1, TimeUnit.SECONDS)
.untilAsserted(() -> {
var tokenRenewResult = client.renewToken();
assertThat(tokenRenewResult).isFailed();
assertThat(tokenRenewResult.getFailureDetail()).isEqualTo("Token renew failed with status: 403");
});
@ComponentTest
@Testcontainers
@Nested
class LastKnownFoss extends Tests {
@Container
static final VaultContainer<?> VAULT_CONTAINER = new VaultContainer<>("vault:1.9.6")
.withVaultToken(UUID.randomUUID().toString());

public static HashicorpVaultSettings getSettings() throws IOException, InterruptedException {
var execResult = VAULT_CONTAINER.execInContainer(
"vault",
"token",
"create",
"-policy=root",
"-ttl=%d".formatted(CREATION_TTL),
"-format=json");

var jsonParser = Json.createParser(new StringReader(execResult.getStdout()));
jsonParser.next();
var auth = jsonParser.getObjectStream().filter(e -> e.getKey().equals(AUTH_KEY))
.map(Map.Entry::getValue)
.findFirst()
.orElseThrow()
.asJsonObject();
var clientToken = auth.getString(CLIENT_TOKEN_KEY);

return HashicorpVaultSettings.Builder.newInstance()
.url(HTTP_URL_FORMAT.formatted(VAULT_CONTAINER.getHost(), VAULT_CONTAINER.getFirstMappedPort()))
.healthCheckPath(HEALTH_CHECK_PATH)
.token(clientToken)
.ttl(TTL)
.renewBuffer(RENEW_BUFFER)
.build();
}

@BeforeEach
void beforeEach() throws IOException, InterruptedException {
client = new HashicorpVaultClient(
testHttpClient(),
mapper,
monitor,
getSettings()
);
}
}

public static HashicorpVaultSettings getSettings() throws IOException, InterruptedException {
var execResult = VAULT_CONTAINER.execInContainer(
"vault",
"token",
"create",
"-policy=root",
"-ttl=%d".formatted(CREATION_TTL),
"-format=json");

var jsonParser = Json.createParser(new StringReader(execResult.getStdout()));
jsonParser.next();
var auth = jsonParser.getObjectStream().filter(e -> e.getKey().equals(AUTH_KEY))
.map(Map.Entry::getValue)
.findFirst()
.orElseThrow()
.asJsonObject();
var clientToken = auth.getString(CLIENT_TOKEN_KEY);

return HashicorpVaultSettings.Builder.newInstance()
.url(HTTP_URL_FORMAT.formatted(VAULT_CONTAINER.getHost(), VAULT_CONTAINER.getFirstMappedPort()))
.healthCheckPath(HEALTH_CHECK_PATH)
.token(clientToken)
.ttl(TTL)
.renewBuffer(RENEW_BUFFER)
.build();
@ComponentTest
@Testcontainers
@Nested
class Latest extends Tests {
@Container
static final VaultContainer<?> VAULT_CONTAINER = new VaultContainer<>("hashicorp/vault:1.17.3")
.withVaultToken(UUID.randomUUID().toString());

public static HashicorpVaultSettings getSettings() throws IOException, InterruptedException {
var execResult = VAULT_CONTAINER.execInContainer(
"vault",
"token",
"create",
"-policy=root",
"-ttl=%d".formatted(CREATION_TTL),
"-format=json");

var jsonParser = Json.createParser(new StringReader(execResult.getStdout()));
jsonParser.next();
var auth = jsonParser.getObjectStream().filter(e -> e.getKey().equals(AUTH_KEY))
.map(Map.Entry::getValue)
.findFirst()
.orElseThrow()
.asJsonObject();
var clientToken = auth.getString(CLIENT_TOKEN_KEY);

return HashicorpVaultSettings.Builder.newInstance()
.url(HTTP_URL_FORMAT.formatted(VAULT_CONTAINER.getHost(), VAULT_CONTAINER.getFirstMappedPort()))
.healthCheckPath(HEALTH_CHECK_PATH)
.token(clientToken)
.ttl(TTL)
.renewBuffer(RENEW_BUFFER)
.build();
}

@BeforeEach
void beforeEach() throws IOException, InterruptedException {
client = new HashicorpVaultClient(
testHttpClient(),
mapper,
monitor,
getSettings()
);
}
}
}
}
Loading