fix: (hashicorp-vault) token self-lookup path as segments #4512
Conversation
…, not a single segment which encodes the path slashes.
drcgjung
changed the title
drcgjung
changed the title
| @@ -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) | |||
There was a problem hiding this comment.
isn't there a way to justify this change through an automated test?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@drcgjung Are you saying that there is a breaking API change between
1.9.6and1.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?
There was a problem hiding this comment.
@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".
There was a problem hiding this comment.
Just to be sure and to guard against regression, a good middle ground would be to test against
1.17.3AND the last-known-free version1.9.6, as @drcgjung suggested. That way we can guarantee that our code works with "an open-source variant".
definitely agree
There was a problem hiding this comment.
Just to be sure and to guard against regression, a good middle ground would be to test against
1.17.3AND the last-known-free version1.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.
There was a problem hiding this comment.
I get why you changed it to non-static, but there should be a better way to do this without blowing up test run times.
[edit]: this could be a rough PoC. we follow the same patterns in other places too.
| class HashicorpVaultClientIntegrationTest { | ||
| @Container | ||
| static final VaultContainer<?> VAULT_CONTAINER = new VaultContainer<>("vault:1.9.6") | ||
| final VaultContainer<?> VAULT_CONTAINER = new VaultContainer<>(getVaultImage()) |
There was a problem hiding this comment.
please do not change that. it will spin up a separate container for each test method, which needlessly blows up test runtimes. test methods should be (and currently are) designed to be orthogonal.
...rc/test/java/org/eclipse/edc/vault/hashicorp/client/HashicorpVaultClientIntegrationTest.java
Outdated
Show resolved
Hide resolved
…ple vault instances. Courtesy of @paullatzelsperger.
What this PR changes/adds
HashciCorpClient should interpret token self-lookup path as several segments.
Why it does that
Currently, the path is interpreted as a single segment (which encodes the path slashes into %2F). The resulting endpoint will hit the hashicorp routing logic and will be redirected to "ui" and return an html page instead of the expected json response.
Further notes
Linked Issue(s)
Closes #4508 <-- insert Issue number if one exists
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.