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.

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

feat: implement automatic hashicorp token renewal #3782

Merged

Conversation

hamidonos
Copy link
Contributor

@hamidonos hamidonos commented Jan 17, 2024

What this PR changes/adds

This PR adds an automatic token renewal mechanism to the Hashicorp Vault extension.

Why it does that

Currently the EDC uses the root token which is not suitable for production stage.

Further notes

Note that the Vault token does not rotate. It is renewed.

A token has a TTL. The TTL defines the interval in which the token has to be renewed, otherwise it will be invalidated forever. If a token expires for any reason the EDC will go into a unhealthy state. Currently there is no other way to recover from a invalid token other than restarting the EDC with a fresh token since the token is provided in the boot configurations.

Brief explanation of new configurations added:

  • VAULT_TOKEN_TTL: Defines the TTL of the token which is passed to the Vault upon renewal. The TTL might not be honored by the Vault (e.g. in the case of periodic tokens). Therefore the TTL value returned by the Vault should be respected for each renewal. Vault requires a minimum TTL of 5 seconds.
  • VAULT_TOKEN_RENEW_BUFFER: Buffer before a renewal is triggered to account for network issues etc. Has to be smaller than the TTL.
  • VAULT_TOKEN_SCHEDULED_RENEWAL_ENABLED: Whether the automatic renewal feature added by this PR is active or not. Defaults to true. This should be only set to false for development or testing cases.

Upon start the EDC validates the token by calling the Vault API. For non valid tokens a error message will be logged and the Connector will enter a unhealthy state. If the token is renewable it will be renewed instantly. If the renewal was successful a scheduled one-shot task is submitted. The task executes after a delay defined as TTL - VAULT_TOKEN_RENEW_BUFFER. If the renewal was successful the next task is scheduled again at TTL - VAULT_TOKEN_RENEW_BUFFER and so on. Http requests are retried if they are retryable. See Hashicorp status codes

For non-renewable but valid tokens the EDC will stay healthy until the token expires.

Tokens have a max TTL even if they're renewable (unless they're periodic). After the max ttl expires the token will be invalidated for ever. Further renewals will fail.
The max TTL can either be a system default config on Vault side, or a explicit max TTL set by the admin at token creation.

Periodic tokens can be renewed forever as long they're renewed within the TTL.

Linked Issue(s)

Closes #3610


Hamid Ahmetovic[email protected], Mercedes-Benz Tech Innovation GmbH, legal info/Impressum

hamidonos and others added 26 commits November 14, 2023 14:05
# Conflicts:
#	spi/common/core-spi/src/main/java/org/eclipse/edc/spi/monitor/Monitor.java
#	spi/common/core-spi/src/main/java/org/eclipse/edc/spi/monitor/PrefixMonitor.java
…t_add_token_rotation

# Conflicts:
#	extensions/common/vault/vault-hashicorp/src/main/java/org/eclipse/edc/vault/hashicorp/HashicorpVaultExtension.java
#	extensions/common/vault/vault-hashicorp/src/test/java/org/eclipse/edc/vault/hashicorp/HashicorpVaultExtensionTest.java
@hamidonos hamidonos self-assigned this Jan 17, 2024
@hamidonos hamidonos added the enhancement New feature or request label Jan 17, 2024
Copy link

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale Open for x days with no activity label Feb 16, 2024
@ndr-brt ndr-brt removed the stale Open for x days with no activity label Feb 16, 2024
return Result.success(isRenewable);
} catch (IllegalArgumentException e) {
var errMsgFormat = "Failed to parse renewable flag from token look up response %s with reason: %s";
monitor.warning(errMsgFormat.formatted(map, e.getStackTrace()));

Check notice

Code scanning / CodeQL

Implicit conversion from array to string Note

Implicit conversion from Array to String.
return Result.success(ttl);
} catch (IllegalArgumentException e) {
var errMsgFormat = "Failed to parse ttl from token renewal response %s with reason: %s";
monitor.warning(errMsgFormat.formatted(map, e.getStackTrace()));

Check notice

Code scanning / CodeQL

Implicit conversion from array to string Note

Implicit conversion from Array to String.
Copy link
Member

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

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

just a minor naming nit, but once that is resolved I think we can merge this PR.

Copy link

github-actions bot commented Mar 6, 2024

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale Open for x days with no activity label Mar 6, 2024
@ndr-brt ndr-brt removed the stale Open for x days with no activity label Mar 6, 2024
@paullatzelsperger
Copy link
Member

@hamidonos is any work being done on this? Seeing as the latest comments from @jimmarino are 2 weeks old.

@paullatzelsperger
Copy link
Member

Notice: should this PR not receive any updates by Friday March 15 2024 5pm CET, I will close, adopt and re-submit it under my authorship.
The Copyright will remain with the original author.

@DominikPinsel
Copy link
Contributor

Notice: should this PR not receive any updates by Friday March 15 2024 5pm CET, I will close, adopt and re-submit it under my authorship. The Copyright will remain with the original author.

We kindly request reconsideration of the one-day response time, as your team member, Hamid, will be out of the office until Friday. Adjusting the response timeframe would greatly aid in maintaining workflow efficiency.

@paullatzelsperger
Copy link
Member

paullatzelsperger commented Mar 14, 2024

@DominikPinsel this PR has been open forever, and no activity was visible for two weeks after the latest comments. We have a release scheduled for March 27 which should have this feature in it.

If it is a matter of a few days, then I'm sure we can accomodate, but we shouldn't wait too long. Shall we say +1week, i.e. Friday March 22nd then.

@paullatzelsperger paullatzelsperger added this to the Milestone 14 milestone Mar 14, 2024
@DominikPinsel
Copy link
Contributor

@DominikPinsel this PR has been open forever, and no activity was visible for two weeks after the latest comments. We have a release scheduled for March 27 which should have this feature in it.

If it is a matter of a few days, then I'm sure we can accomodate, but we shouldn't wait too long. Shall we say +1week, i.e. Friday March 22nd then.

This would be great! :)

@ndr-brt
Copy link
Member

ndr-brt commented Mar 19, 2024

hey @hamidonos we had some changes that conflicted with this PR, please solve conflicts and then we'll be ready to merge

…t_add_token_rotation

# Conflicts:
#	extensions/common/vault/vault-hashicorp/src/main/java/org/eclipse/edc/vault/hashicorp/HashicorpVaultClientConfig.java
#	extensions/common/vault/vault-hashicorp/src/main/java/org/eclipse/edc/vault/hashicorp/HashicorpVaultConfig.java
#	extensions/common/vault/vault-hashicorp/src/main/java/org/eclipse/edc/vault/hashicorp/HashicorpVaultExtension.java
#	extensions/common/vault/vault-hashicorp/src/main/java/org/eclipse/edc/vault/hashicorp/HashicorpVaultHealthExtension.java
#	extensions/common/vault/vault-hashicorp/src/test/java/org/eclipse/edc/vault/hashicorp/HashicorpVaultExtensionTest.java
#	extensions/common/vault/vault-hashicorp/src/test/java/org/eclipse/edc/vault/hashicorp/HashicorpVaultIntegrationTest.java
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 90.07937% with 25 lines in your changes missing coverage. Please review.

Project coverage is 73.96%. Comparing base (7f20ba5) to head (f379945).
Report is 533 commits behind head on main.

Files with missing lines Patch % Lines
...ipse/edc/vault/hashicorp/HashicorpVaultClient.java 84.34% 11 Missing and 7 partials ⚠️
.../vault/hashicorp/HashicorpVaultTokenRenewTask.java 89.47% 1 Missing and 3 partials ⚠️
...vault/hashicorp/HashicorpVaultHealthExtension.java 81.81% 1 Missing and 1 partial ⚠️
...lipse/edc/spi/system/health/HealthCheckResult.java 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3782      +/-   ##
==========================================
+ Coverage   71.74%   73.96%   +2.22%     
==========================================
  Files         919      984      +65     
  Lines       18457    20008    +1551     
  Branches     1037     1127      +90     
==========================================
+ Hits        13242    14799    +1557     
+ Misses       4756     4726      -30     
- Partials      459      483      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ndr-brt ndr-brt merged commit 3cde990 into eclipse-edc:main Mar 20, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HashiCorp Vault: Add token rotation mechanism
6 participants